more fixes for image consolidation / bazel caching #6713

BenTheElder · 2018-02-08T08:34:03Z

the first commit is necessary for clean test-infra logs
the second commit is cleanup
the third commit tracks GCC and python versions when caching with bazel (in theory)
- see: remote/correctness: Bazel doesn't track system libraries. bazelbuild/bazel#4558

BenTheElder · 2018-02-08T08:35:11Z

I'm still committed to refactoring how we make the bazel cache RCs once (if?) we move nursery out of experimental, but for now yes: more bash 🙃

BenTheElder · 2018-02-08T08:36:09Z

/area images
/area bazel

ixdy · 2018-02-08T18:49:54Z

experiment/nursery/create_bazel_cache_rcs.sh

+    GCC_VERSION=$(package_version gcc)
+    echo "build --action_env=__CACHE_GCC_VERSION__=${GCC_VERSION}" >> $1
+    PYTHON_VERSION=$(package_version python)
+    echo "build --action_env=__CACHE_PYTHON_VERSION__=${PYTHON_VERSION}" >> $1


FYI if you want to stop writing echo ... >> $1 all the time, you can probably do something like

cat >$1 <<EOF # this is the default for recent releases but we set it explicitly # since this is the only hash our cache supports startup --host_jvm_args=-Dbazel.DigestFunction=sha256 # use remote caching for all the things build --experimental_remote_spawn_cache # point it at our http cache ... build --remote_http_cache=${CACHE_HOST} ... EOF

hmm the downside to this is that I have to deindent the block being written and the whole block highlights as a string 🤷‍♂️

other alternatives:

wrap everything in a subshell ( ), e.g.

( echo "blah" ) > $1

Rather than having make_bazel_rc take an argument, just pipe it to a file below, e.g.

make_bazel_rc > "${HOME}/.bazelrc" make_bazel_rc > /etc/bazel.bazelrc

(don't consider these comments blocking, though)

Thanks! I'll go with one of these. Blocking this myself on verifying that I can disrupt the cache with GCC version locally and then fix it with these args. Will do one of these first.

ixdy · 2018-02-08T18:50:45Z

what do you mean by "clean test-infra logs"?

BenTheElder · 2018-02-08T19:08:52Z

what do you mean by "clean test-infra logs"?

We get a lot of spam in the new builds: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/test-infra/6713/pull-test-infra-bazel/12990/?log#log which I suspect is due to missing tooling, not much else has changed so my best bet right now is outdated python tools.

krzyzacy

/lgtm
/hold

k8s-ci-robot · 2018-02-08T19:25:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, krzyzacy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [BenTheElder,krzyzacy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

BenTheElder · 2018-02-08T23:28:46Z

experiment/nursery/create_bazel_cache_rcs.sh

@@ -19,6 +19,10 @@
 # TODO(bentheelder): verify that this works and move it into the images
 CACHE_HOST="http://bazel-cache.default:8080"

+package_version () {
+    dpkg-query --showformat='${Version}' --show $1


This is correct for getting a package version but not sufficient for tracking what $CC actually points to if we install other versions etc. updating in a bit with something smarter based on intentionally breaking the cache with planter and a local instance of the remote cache.

maybe $CC --version? contains spaces and strings, so needs quoting.

I'm also curious how stdlib includes are accounted for in caching.

also curious: does the caching take into account the --crosstool and --cpu flags?

BenTheElder · 2018-02-09T00:12:13Z

experiment/nursery/create_bazel_cache_rcs.sh

+    GCC_VERSION=$(package_version gcc)
+    echo "build --action_env=__CACHE_GCC_VERSION__=${GCC_VERSION}" >> $1
+    PYTHON_VERSION=$(package_version python)
+    echo "build --action_env=__CACHE_PYTHON_VERSION__=${PYTHON_VERSION}" >> $1


I think you also need build --experimental_strict_action_env for these to take effect.. https://docs.bazel.build/versions/master/command-line-reference.html

BenTheElder · 2018-02-09T06:35:08Z

Today I proved locally that if you switch GCC versions system-wide inside planter and try to reuse the remote cache you get poisoned results because bazel doesn't have any way to track the host binaries that aren't actually registered with bazel currently.

The proposed work around of --action_env=SOME_BOGUS_ENV=<tool versioning> almost works but some actions don't seem to pass any env (tracked with bazel build //... -s to output the calls) so test-infra will incorrectly cache and choke on missing headers while compiling protobuf cc files.

My next thought is to use the tooling tracking to create a meta "cache version" key we insert into bazelrc like build --remote_http_cache=http://${CACHE_DOMAIN}:${CACHE_PORT}/${CACHE_VERSION}. The docs for the protocol suggest that prefixing the cache HTTP path is normal / supported.

https://docs.bazel.build/versions/master/remote-caching.html#http-caching-protocol

BenTheElder · 2018-02-09T06:37:39Z

/hold cancel
current changes should go in anyhow

BenTheElder · 2018-02-09T07:03:38Z

Following up with @buchgr on this idea, will experiment locally a bit.

add python dev packages to bootstrap images

b990f57

BenTheElder requested review from ixdy and krzyzacy as code owners February 8, 2018 08:34

k8s-ci-robot assigned ixdy and krzyzacy Feb 8, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 8, 2018

k8s-ci-robot added area/images area/bazel labels Feb 8, 2018

remove unecessary BUILD

3c32cda

BenTheElder force-pushed the consolidation branch 2 times, most recently from 262f704 to b4a4707 Compare February 8, 2018 08:45

ixdy reviewed Feb 8, 2018

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 8, 2018

krzyzacy approved these changes Feb 8, 2018

View reviewed changes

BenTheElder commented Feb 8, 2018

View reviewed changes

BenTheElder commented Feb 9, 2018

View reviewed changes

make bazel cache track host tools

9c795fb

BenTheElder force-pushed the consolidation branch from b4a4707 to 9c795fb Compare February 9, 2018 06:36

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 9, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2018

k8s-ci-robot merged commit 303e45f into kubernetes:master Feb 9, 2018

BenTheElder deleted the consolidation branch February 9, 2018 07:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

more fixes for image consolidation / bazel caching #6713

more fixes for image consolidation / bazel caching #6713

BenTheElder commented Feb 8, 2018 •

edited

Loading

BenTheElder commented Feb 8, 2018

BenTheElder commented Feb 8, 2018

ixdy Feb 8, 2018

BenTheElder Feb 8, 2018

ixdy Feb 8, 2018

ixdy Feb 8, 2018

BenTheElder Feb 8, 2018

BenTheElder Feb 9, 2018

ixdy commented Feb 8, 2018

BenTheElder commented Feb 8, 2018

krzyzacy left a comment

k8s-ci-robot commented Feb 8, 2018

BenTheElder Feb 8, 2018

ixdy Feb 8, 2018

ixdy Feb 8, 2018

BenTheElder Feb 9, 2018

BenTheElder Feb 9, 2018

BenTheElder commented Feb 9, 2018 •

edited

Loading

BenTheElder commented Feb 9, 2018

BenTheElder commented Feb 9, 2018

more fixes for image consolidation / bazel caching #6713

more fixes for image consolidation / bazel caching #6713

Conversation

BenTheElder commented Feb 8, 2018 • edited Loading

BenTheElder commented Feb 8, 2018

BenTheElder commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixdy commented Feb 8, 2018

BenTheElder commented Feb 8, 2018

krzyzacy left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Feb 9, 2018 • edited Loading

BenTheElder commented Feb 9, 2018

BenTheElder commented Feb 9, 2018

BenTheElder commented Feb 8, 2018 •

edited

Loading

BenTheElder commented Feb 9, 2018 •

edited

Loading