use per WORKSPACE / host tools keyed bazel cache(s) #6767

BenTheElder · 2018-02-09T21:46:58Z

We can solve a lot of problems by just using one cache per (WORKSPACE [org/repo], (Image Tool Versions [gcc, python, etc.]))
We do this by setting:

CACHE_URL="http://${CACHE_HOST}:${CACHE_PORT}/${CACHE_ID}"
echo "build --remote_http_cache=${CACHE_URL}"

Where CACHE_ID currently comes from CACHE_ID="${WORKSPACE_NAME},$(hash_toolchains)"

Simple per repo cache
No incorrect cache sharing (in theory, since we key each setup to it's own storage)
If somehow incorrect cache sharing happens by accident, we just update the cache keying logic to be different so a new cache is used, no mucking with the cache node required
If we add a new host toolchain we can add it to the cache keying trivially
Doesn't depend on any special bazel features

Right now I've implemented the keying and tested it against nginx with webdav enabled locally.
TODO:

update nursery deployment to use something else that supports storing multiple directories
- evaluate caches:
  - hazelcast is more complex than what we want, nginx with webdav looks a bit simplistic..., etc. nothing seems to support many repos while being operationally minimal (IE not hazelcast, we don't need a huge complex distributed cache, JVM tuning etc..)
- implement our own server that meets our needs (mainly multiple/arbitrary individual caches)

Follow-ups:

start using experimentally for test-infra
Track cache usage / add metrics
Implement eviction for stale / unused entries
integrate bazelrcs into image(s)

BenTheElder · 2018-02-09T21:47:06Z

/cc @ixdy @krzyzacy

ixdy · 2018-02-09T22:33:29Z

experiment/nursery/create_bazel_cache_rcs.sh

-    echo "build --action_env=CACHE_PYTHON_VERSION=${PYTHON_VERSION}"
+    # point it at our http cache ...
+    # NOTE our cache is versioned by the first path segment
+    # TODO(bentheelder): update the nursery deployment to something that supports this


what does this mean?

the cache server deployment "nursery" is actually https://github.com/buchgr/bazel-remote currently which only supports a single cache directory

this is done now

ixdy · 2018-02-09T22:34:56Z

experiment/nursery/create_bazel_cache_rcs.sh

+    # point it at our http cache ...
+    # NOTE our cache is versioned by the first path segment
+    # TODO(bentheelder): update the nursery deployment to something that supports this
+    WORKSPACE_NAME="${REPO_NAME:-$(basename "$PWD")}"


is reponame just like test-infra or is it kubernetes/test-infra?

this is just test-infra https://github.com/kubernetes/test-infra/tree/master/prow#job-environment-variables

It's probably better to use owner as well, though implementing the bash fallback for CI jobs may be a bit messy.

I fixed this and cleaned up the bash a bit

ixdy · 2018-02-09T22:41:49Z

another thought: most of our images include an IMAGE environment variable which is updated each time we push a new image. could we use that for hashing instead?

it'd result in slightly less caching, since we probably don't change most of the underlying dependencies that often. OTOH it should be pretty safe.

BenTheElder · 2018-02-09T22:44:11Z

another thought: most of our images include an IMAGE environment variable which is updated each time we push a new image. could we use that for hashing instead?

We can, but I'm not really concerned if we can track GCC versions correctly (this part is easy) and the images are pushed fairly frequently for other changes (kubetest).

Either way we need to key off the repo/WORKSPACE as well.

BenTheElder · 2018-02-09T22:45:07Z

Since we're based on debian jessie now host tools like python and gcc are unlikely to change until we switch the base image to something else someday (stretch?).

BenTheElder · 2018-02-09T22:47:55Z

Also if we do run into an issue with the toolchain hashing this we can switch out the cache identifier generation logic to use $IMAGE instead as long as the backend supports arbitrary URL/file system safe names for separate caches. I'd like to maximize the actual caching if we can though. 🤷‍♂️

BenTheElder · 2018-02-10T01:31:50Z

the server should work now, need to clean up the logging and test it some more then package it up for deployment.

BenTheElder · 2018-02-10T01:44:45Z

To test the server locally, add this to .bazelrc in test-infra:

startup --host_jvm_args=-Dbazel.DigestFunction=sha256
build --spawn_strategy=remote --genrule_strategy=remote
build --strategy=Javac=remote --strategy=Closure=remote
build --remote_local_fallback
build --remote_http_cache=http://localhost:8080/k8s.io/test-infra

build and run:

bazel build //experiment/nursery
bazel-bin/experiment/nursery/darwin_amd64_stripped/nursery --dir=$HOME/bazel-remote-cache

build something in another shell:

bazel build //...

BenTheElder · 2018-02-10T08:50:53Z

/test pull-test-infra-bazel-canary
/test pull-test-infra-bazel
[using new nursery deployment]

BenTheElder · 2018-02-12T21:55:59Z

/area bazel

cjwagner · 2018-02-12T22:27:55Z

experiment/nursery/diskcache/cache.go

+}
+
+// Get provides your readHandler with the contents at key
+func (c *Cache) Get(key string, readHandler ReadHandler) error {


Instead of using a ReadHandler you could just return the ReadSeeker and an error. That would be more idiomatic.

Yes, but this allows me to track who currently holds a cache key which may be very useful in the future.

cjwagner · 2018-02-12T22:46:41Z

experiment/nursery/main.go

+}
+
+func main() {
+	// TODO(bentheelder): bound cache size / convert to LRU


This should be completed before we use this anywhere so that the cache doesn't grow uncontrollably.

It only takes up about 4gb to host test infra, the disk is 375gb. We won't use this in prod yet, just experimentally. I'm going to collect some more data from experimental usage before deciding on an eviction strategy.

Filling the disk doesn't cause failures though either, you just won't be able to insert new things into the cache.

Also worth noting that this runs on a dedicated node, so it doesn't affect the rest of CI. A previous iteration of this already runs and is used by the pull-.*bazel.*-canary jobs.

cjwagner · 2018-02-12T22:51:29Z

experiment/nursery/main.go

+				}
+				// unknown error
+				log.WithError(err).Error("error getting key")
+				http.Error(w, err.Error(), http.StatusNotFound)


This should be a 5xx status code instead of a 404.

sure, I didn't pay too much attention to the error codes since the only client should be bazel which should only concerned with 200 or not

cjwagner · 2018-02-12T22:55:36Z

experiment/nursery/main.go

+		// handle unsupported methods...
+		default:
+			log.Warn("received an invalid request method: %v", r.Method)
+			http.Error(w, "unsupported method", http.StatusBadRequest)


405 is more specific than 400.

cjwagner · 2018-02-12T23:02:10Z

experiment/nursery/main.go

+		case http.MethodPut:
+			// only hash CAS, not action cache
+			// the action cache is hash -> metadata
+			// the CAS is well, a CAS, which we can hash...


I don't understand what the action cache (ac) variant is supposed to do. Where is the hash -> metadata mapping mentioned in the comment and how is it used?

See the protocol linked at the top of the file and the comment below: https://docs.bazel.build/versions/master/remote-caching.html#http-caching-protocol

Bazel does PUT $HTTP_CACHE_URL/ac/$HASH (bytes of PUT are action metadata) and GET the same way
It also does PUT $HTTP_CACHE_URL/cas/$HASH (bytes of PUT are content-address object storage) and GET the same way.

cjwagner · 2018-02-12T23:19:40Z

experiment/nursery/main.go

+			if acOrCAS != "cas" {
+				hash = ""
+			}
+			err := cache.Put(r.URL.Path, r.Body, hash)


I'm a little confused about the file retrieval process.
If we do a PUT to /org/repo/cas/15e2b0d and create a file cache-dir/org/repo/cas/15e2b0d we wont be able to GET the file without knowing its content digest (15e2b0d) which we would never know unless we already had the file... Is this where action caches come into play?

Yes. Caching works like:
Bazel computes the entire build as a graph of Actions, each Action has inputs etc and is hashed to some action-cache key. The action cache key is then looked up in the cache for metadata stored from actually executing an action. If there is a cache hit, then the objects are looked up in the CAS from this metadata.

The CAS is just a content addressed store of files used / produced by actions.

We can't hash the actions because they are computed each time and map to metadata from execution instead of the action itself, which is much cheaper to compute. The caches are something like:

Action Cache (../ac/..): Hash(Action.serialize()) -> Action.execute().metadata()

Action Metadata -> CAS Keys

CAS (.../cas...): Hash(OutputFile.getBytes()) -> OutputFile.getBytes()

cjwagner · 2018-02-12T23:22:24Z

experiment/nursery/main.go

+
+func cacheHandler(cache *diskcache.Cache) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// parse and validate path


It might be helpful to create a logrus.Entry here that has a path and a method field and use that to log warnings and errors throughout this function.

done, also set "component"

cjwagner · 2018-02-12T23:55:44Z

Why aren't we just using GCS as the cache (https://docs.bazel.build/versions/master/remote-caching.html#google-cloud-storage)?

BenTheElder · 2018-02-12T23:59:02Z

@cjwagner because this way the bandwidth is all in-cluster which should be faster and cheaper, we also don't want to provide jobs with unrestricted access to stuff data in GCS. This way the storage is private because it can only be accessed within the cluster and our costs are bound to the cost of the cache node.

BenTheElder · 2018-02-13T00:04:41Z

Serving files from a local SSD in cluster is very fast, pull-test-infra-bazel-canary jobs are now bound to the cost of the pip install and pylint, the time to bazel build / test is otherwise neglible using the cache node.

krzyzacy · 2018-02-13T00:16:58Z

I'm fine with experimenting this with a few canary jobs to start with, as the performance improvement already looks promising. punt to @cjwagner if he has more comments.

cjwagner

Thanks for explaining everything. Maybe add a short comment explaining why we have our own bazel cache implementation instead of using an existing one.
/lgtm
/hold

k8s-ci-robot · 2018-02-13T00:20:41Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,cjwagner]

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

ixdy · 2018-02-13T00:22:41Z

experiment/nursery/create_bazel_cache_rcs.sh

@@ -26,15 +27,38 @@ package_to_version () {

 # look up a binary with which and return the debian package it belongs to
 command_to_package () {
-    BINARY_PATH=$(readlink -f $(which $1))
+    local BINARY_PATH=$(readlink -f $(which $1))


nit: per https://google.github.io/styleguide/shell.xml?showone=Variable_Names#Variable_Names, variable names should be lowercase.

done, and elsewhere

krzyzacy · 2018-02-13T00:24:02Z

/lint
/joke

k8s-ci-robot · 2018-02-13T00:24:03Z

@krzyzacy: What do you call a cow with two legs? Lean beef.

In response to this:

/lint
/joke

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot

@krzyzacy: 1 warning.

In response to this:

/lint
/joke

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2018-02-13T00:24:14Z

experiment/nursery/diskcache/cache.go

+limitations under the License.
+*/
+
+// cache implements disk backed cache storage for use in nursery


Golint comments: package comment should be of the form "Package diskcache ...". More info.

BenTheElder · 2018-02-13T00:24:58Z

Thanks for explaining everything. Maybe add a short comment explaining why we have our own bazel cache implementation instead of using an existing one.

👍

If everything goes well with the next round of canary experiments I plan to:

add an eviction strategy
rename to greenhouse, add a more detailed README, and graduate it from "experiment/"
integrate creating the bazelrcs into our images (behind some feature gate)
instrument / add metrics, create a velodrome dashboard, alert on low disk

BenTheElder · 2018-02-13T00:59:14Z

/hold cancel

Will continue to improve this in other PRs, this is already XL. Thanks for all the review @cjwagner @krzyzacy @ixdy 😄

BenTheElder · 2018-02-13T02:21:08Z

Small follow-up:
8m11s: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/test-infra/5137/pull-test-infra-bazel-canary/115/
After the cache is warm:
3m6s: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/test-infra/5137/pull-test-infra-bazel-canary/117/

bentheelder@gke-prow-bazel-cache-70c4c5ed-61kg ~ $ df -h /mnt/disks/ssd0
Filesystem      Size  Used Avail Use% Mounted on
/dev/sdb        369G  2.1G  348G   1% /mnt/disks/ssd0
bentheelder@gke-prow-bazel-cache-70c4c5ed-61kg ~ $ sudo du -h /mnt/disks/ssd0/cache
2.1G    /mnt/disks/ssd0/cache/kubernetes/test-infra,b68e017af9a3ae39e87829bd5fe5bfaa/cas
7.4M    /mnt/disks/ssd0/cache/kubernetes/test-infra,b68e017af9a3ae39e87829bd5fe5bfaa/ac
2.1G    /mnt/disks/ssd0/cache/kubernetes/test-infra,b68e017af9a3ae39e87829bd5fe5bfaa
2.1G    /mnt/disks/ssd0/cache/kubernetes
2.1G    /mnt/disks/ssd0/cache

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 9, 2018

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 9, 2018

k8s-ci-robot requested review from ixdy and krzyzacy February 9, 2018 21:47

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 9, 2018

BenTheElder force-pushed the cache-of-caches branch from dc61d9e to 126be60 Compare February 9, 2018 21:59

ixdy changed the title ~~[WIP] use per WORKSPACe / host tools keyed bazel cache(s)~~ [WIP] use per WORKSPACE / host tools keyed bazel cache(s) Feb 9, 2018

ixdy reviewed Feb 9, 2018

View reviewed changes

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

BenTheElder force-pushed the cache-of-caches branch 4 times, most recently from 6a2e1d9 to 159ab63 Compare February 10, 2018 01:29

BenTheElder force-pushed the cache-of-caches branch from 159ab63 to 8b52fdb Compare February 10, 2018 01:32

BenTheElder force-pushed the cache-of-caches branch 2 times, most recently from 5733ed5 to aa6d81c Compare February 10, 2018 02:10

BenTheElder force-pushed the cache-of-caches branch from a127ac6 to 2991eb1 Compare February 10, 2018 21:16

update bazelrc creation to use keyed cache

c6353bd

BenTheElder force-pushed the cache-of-caches branch from 2991eb1 to 62f2414 Compare February 10, 2018 21:38

k8s-ci-robot requested a review from Q-Lee February 12, 2018 21:10

add some tests

fc60337

BenTheElder force-pushed the cache-of-caches branch from f2e343a to fc60337 Compare February 12, 2018 21:15

k8s-ci-robot added the area/bazel label Feb 12, 2018

cjwagner reviewed Feb 12, 2018

View reviewed changes

address error code / logging nits

a9c164e

BenTheElder force-pushed the cache-of-caches branch from d447767 to a9c164e Compare February 12, 2018 23:48

cjwagner approved these changes Feb 13, 2018

View reviewed changes

k8s-ci-robot assigned cjwagner Feb 13, 2018

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

ixdy reviewed Feb 13, 2018

View reviewed changes

k8s-ci-robot reviewed Feb 13, 2018

View reviewed changes

fix more nits

20662c1

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 13, 2018

k8s-ci-robot merged commit 4d96d02 into kubernetes:master Feb 13, 2018

BenTheElder deleted the cache-of-caches branch February 13, 2018 02:21

BenTheElder mentioned this pull request Feb 13, 2018

Implement Bazel Remote Caching [Tracking Issue] #6808

Closed

12 tasks

BenTheElder added the area/greenhouse Issues or PRs related to code in /greenhouse (our remote bazel cache) label Sep 5, 2018

use per WORKSPACE / host tools keyed bazel cache(s) #6767

use per WORKSPACE / host tools keyed bazel cache(s) #6767

Conversation

BenTheElder commented Feb 9, 2018 • edited Loading

BenTheElder commented Feb 9, 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

BenTheElder Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixdy commented Feb 9, 2018

BenTheElder commented Feb 9, 2018

BenTheElder commented Feb 9, 2018

BenTheElder commented Feb 9, 2018

BenTheElder commented Feb 10, 2018

BenTheElder commented Feb 10, 2018 • edited Loading

BenTheElder commented Feb 10, 2018

BenTheElder commented Feb 12, 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

BenTheElder Feb 12, 2018 • edited Loading

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

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 Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

cjwagner commented Feb 12, 2018

BenTheElder commented Feb 12, 2018

BenTheElder commented Feb 13, 2018

krzyzacy commented Feb 13, 2018

cjwagner left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krzyzacy commented Feb 13, 2018

k8s-ci-robot commented Feb 13, 2018

k8s-ci-robot left a 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 13, 2018

BenTheElder commented Feb 13, 2018

BenTheElder commented Feb 13, 2018 • edited Loading

BenTheElder commented Feb 9, 2018 •

edited

Loading

BenTheElder Feb 9, 2018 •

edited

Loading

BenTheElder commented Feb 10, 2018 •

edited

Loading

BenTheElder Feb 12, 2018 •

edited

Loading

BenTheElder Feb 12, 2018 •

edited

Loading

BenTheElder Feb 12, 2018 •

edited

Loading

BenTheElder commented Feb 13, 2018 •

edited

Loading