bazel: use GCS remote cache #4050

lizan · 2018-08-03T22:27:47Z

Signed-off-by: Lizan Zhou zlizan@google.com

Partially addresses #3741

Description:
Use GCS remote cache for CI builds when key is provided.

Risk Level: Low
Testing: CI
Docs Changes:
Release Notes:

Signed-off-by: Lizan Zhou <zlizan@google.com>

mattklein123 · 2018-08-03T22:52:26Z

ci/setup_gcs_cache.sh

+  rm -rf /tmp/gcp_service_account.json
+}
+
+if [[ ! -z "${GCP_SERVICE_ACCOUNT_KEY}" && ! -z "${BAZEL_REMOTE_CACHE}" ]]; then


Do you want to grab me on slack and I can setup the env variables for you so we can test this?

I was able to add those env variables to CircleCI

Signed-off-by: Lizan Zhou <zlizan@google.com>

PiotrSikora · 2018-08-03T23:53:41Z

ci/do_circle_ci.sh

@@ -18,6 +18,8 @@ export NUM_CPUS=8
 # (see https://circleci.com/docs/2.0/executor-types/#using-machine)
 export BAZEL_EXTRA_TEST_OPTIONS="--test_env=ENVOY_IP_TEST_VERSIONS=v4only"

+export BAZEL_REMOTE_CACHE="${BAZEL_REMOTE_CACHE:-https://storage.googleapis.com/envoy-circleci-bazel-cache/}"


Wouldn't it be more appropriate to set this in .circleci/config.yml?

Yeah, can we make this end-user configurable and document how individual developers can also make use of it? Maybe in https://github.com/envoyproxy/envoy/blob/master/bazel/README.md.

(Specifically for when working outside a Docker image)

htuch

Awesome, excited to see this land; any idea of speedups observed so far?

htuch · 2018-08-05T01:46:56Z

ci/setup_gcs_cache.sh

+
+gcp_service_account_cleanup() {
+  echo "Deleting service account key file..."
+  rm -rf /tmp/gcp_service_account.json


Use TMPDIR or something, probably bad to assume the existent of /tmp.

htuch · 2018-08-05T01:47:18Z

ci/setup_gcs_cache.sh

+if [[ ! -z "${BAZEL_REMOTE_CACHE}" ]]; then
+
+  if [[ ! -z "${GCP_SERVICE_ACCOUNT_KEY}" ]]; then
+    echo "${GCP_SERVICE_ACCOUNT_KEY}" | base64 --decode > /tmp/gcp_service_account.json


htuch · 2018-08-05T01:48:29Z

ci/setup_gcs_cache.sh

+    trap gcp_service_account_cleanup EXIT
+
+    export BAZEL_BUILD_EXTRA_OPTIONS="${BAZEL_BUILD_EXTRA_OPTIONS} \
+      --remote_http_cache=${BAZEL_REMOTE_CACHE} --google_credentials=/tmp/gcp_service_account.json"


Do we have to use filesystem? If so, best to make sure that the file perms are appropriately restricted.

Yes, switched to using mktemp also addresses the permission.

htuch · 2018-08-05T01:49:10Z

ci/do_circle_ci.sh

@@ -18,6 +18,8 @@ export NUM_CPUS=8
 # (see https://circleci.com/docs/2.0/executor-types/#using-machine)
 export BAZEL_EXTRA_TEST_OPTIONS="--test_env=ENVOY_IP_TEST_VERSIONS=v4only"

+export BAZEL_REMOTE_CACHE="${BAZEL_REMOTE_CACHE:-https://storage.googleapis.com/envoy-circleci-bazel-cache/}"


(Specifically for when working outside a Docker image)

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan · 2018-08-06T22:43:20Z

Performance can be tracked at https://circleci.com/gh/envoyproxy/envoy/tree/ci_gcs_remote_cache , it doesn't looks like this helped a lot, perhaps the network is not fast enough compared to compilation? Needs more investigation.

PiotrSikora · 2018-08-06T23:44:51Z

The cache hit ratio on that branch is pretty terrible, e.g. for release builds:

From https://circleci.com/gh/envoyproxy/envoy/79682 (bazel: use GCS remote cache):

INFO: 4323 processes: 7 remote cache hit, 4316 local.
INFO: 582 processes: 21 remote cache hit, 561 local.

~0.5% cache hit ratio, total time: 1:16:05.

From https://circleci.com/gh/envoyproxy/envoy/79704 (setup read-only cache ...):

INFO: 1738 processes: 68 remote cache hit, 1670 local.
INFO: 4323 processes: 420 remote cache hit, 3903 local.
INFO: 582 processes: 67 remote cache hit, 515 local.

~8% cache hit ratio, total time: 1:10:28.

From https://circleci.com/gh/envoyproxy/envoy/79745 (merge with upstream/master):

INFO: 1741 processes: 91 remote cache hit, 1650 local.
INFO: 4329 processes: 245 remote cache hit, 4084 local.
INFO: 582 processes: 64 remote cache hit, 518 local.

~6% cache hit ratio, total time: 1:11:56.

From https://circleci.com/gh/envoyproxy/envoy/80260 (move env variables to circleci.yaml):

INFO: 1741 processes: 123 remote cache hit, 1618 local.
INFO: 4329 processes: 887 remote cache hit, 3442 local.
INFO: 582 processes: 156 remote cache hit, 426 local.

~17.5% cache hit ratio, total time: 55:33.

The last one is quite surprising, because it should be close to 100%, so something seems busted.

Anyway, the issue with caching is that any changes to the environment, api/ protos and/or include/ headers will effectively invalidate it, and while it's really amazing for local development and/or developing 3rd party filters (see: improvements in Istio Proxy with CircleCI's cache), I wouldn't have too much hopes for it for the new PRs... I suspect the cache hit ratio for the initial commits in the new PRs is going to be around 0-10%, but very high for subsequent commits addressing review comments (assuming no changes to api/ and include/). Ultimately, we'd need to deploy it to see how big of an improvement there is in practice.

lizan · 2018-08-07T00:59:05Z

The low cache hit rate is mostly due to

WARNING: Error writing to the remote cache:
429 Too Many Requests
<?xml version='1.0' encoding='UTF-8'?><Error><Code>SlowDown</Code><Message>Please reduce your request rate.</Message><Details>Changing object envoy-circleci-bazel-cache/cas/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 too quickly, please slow down</Details></Error>

I would expect after multiple runs the hit rate will be improved.

lizan · 2018-08-07T01:23:22Z

I wouldn't have too much hopes for it for the new PRs... I suspect the cache hit ratio for the initial commits in the new PRs is going to be around 0-10%, but very high for subsequent commits addressing review comments (assuming no changes to api/ and include/).

This won't be true unless we expose environment variables to PRs from forks (currently it doesn't and I don't feel it is safe), so the PRs from forks (like #4049 compared to this) will have read-only cache.

PiotrSikora · 2018-08-07T03:01:28Z

I would expect after multiple runs the hit rate will be improved.

From https://circleci.com/gh/envoyproxy/envoy/80286 (merge with upstream/master):

INFO: 1741 processes: 141 remote cache hit, 1600 local.
INFO: 4351 processes: 806 remote cache hit, 3545 local.
INFO: 586 processes: 223 remote cache hit, 363 local.

~17.5% cache hit ratio (same as before), total time: 1:20:06.

This won't be true unless we expose environment variables to PRs from forks (currently it doesn't and I don't feel it is safe), so the PRs from forks (like #4049 compared to this) will have read-only cache.

Good point, and agreed wrt security, but looking at the PRs history, it seems that only Lyft folks are using the main repo for PRs, and everybody else is using private forks, so this approach won't result in huge improvements.

lizan · 2018-08-07T04:20:54Z

Seems mac improves quite a bit during this PR, it might be because mac runs first (without downloading docker image) and fills GCS rate limit soon.

https://circleci.com/gh/envoyproxy/envoy/80290 (total time: 55:35)

INFO: 2290 processes: 2084 remote cache hit, 206 darwin-sandbox.
INFO: 1846 processes: 1214 remote cache hit, 632 darwin-sandbox.
INFO: 578 processes: 289 remote cache hit, 289 darwin-sandbox.

https://circleci.com/gh/envoyproxy/envoy/80368 (total time: 26:35)

INFO: 2290 processes: 2287 remote cache hit, 3 darwin-sandbox.
INFO: 1846 processes: 1554 remote cache hit, 292 darwin-sandbox.
INFO: 578 processes: 573 remote cache hit, 5 darwin-sandbox.

lizan · 2018-08-07T04:22:12Z

Good point, and agreed wrt security, but looking at the PRs history, it seems that only Lyft folks are using the main repo for PRs, and everybody else is using private forks, so this approach won't result in huge improvements.

The read-only build cache will be kept updated from master builds, so each PR (unless it changes include/) will use some of caches.

Signed-off-by: Lizan Zhou <zlizan@google.com>

htuch

Diff looks good, will continue discussion in PR on efficacy.

htuch · 2018-08-07T16:23:30Z

ci/setup_gcs_cache.sh

+set -e
+
+if [[ ! -z "${BAZEL_REMOTE_CACHE}" ]]; then
+


Nit: remove superfluous blank lines here and below.

htuch · 2018-08-07T16:32:00Z

I see evidence of rate limiting on the branch CI, e.g. https://circleci.com/gh/envoyproxy/envoy/80440. Is this killing our ability to effectively cache? Would we be better off with a dedicated cache server, e.g. Google Cloud Build? Are we should be hitting the 200 objects modifications per second at https://cloud.google.com/storage/quotas?

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan · 2018-08-07T17:54:00Z

@htuch the 200 per second is for object composition. Per https://cloud.google.com/storage/docs/request-rate GCS doesn't really have a hard limit but need warm up starting from 1000 req/s write and 5000 req/s read, I would expect after we merge this, CI will use cache for every commit: read/write from master builds, read from each PR builds, the storage will be warmer.

htuch · 2018-08-10T16:26:52Z

@lizan LGTM; is there anything holding us back on merging this?

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan · 2018-08-10T19:42:02Z

@htuch I was investigating the coverage drop for runs on CircleCI, turns out the remote cache doesn't cache gcno files unless you do bazel coverage or --collect_code_coverage, but that will break the coverage script.

lizan · 2018-08-10T19:43:49Z

Disabled the cache for coverage, I'll address it in a follow up. Let's merge this first and see how it goes on master.

htuch

Look, up in the sky! It's a bird! It's a plane! It's Bazel remote caching!

* origin/master: (38 commits) test: add tests for corner-cases around sending requests before run() starts or after run() ends. (envoyproxy#4114) perf: reduce the memory usage of LC Trie construction (envoyproxy#4117) test: moving redundant code in websocket_integration_test to utilities (envoyproxy#4127) test: make YamlLoadFromStringFail less picky about error msg. (envoyproxy#4141) rbac: add rbac network filter. (envoyproxy#4083) fuzz: route lookup and header finalization fuzzer. (envoyproxy#4116) Set content-type and content-length (envoyproxy#4113) fault: use FractionalPercent for percent (envoyproxy#3978) test: Fix inverted exact match logic in IntegrationTcpClient::waitForData() (envoyproxy#4134) Added cluster_name to load assignment config for static cluster (envoyproxy#4123) ssl: refactor ContextConfig to use TlsCertificateConfig (envoyproxy#4115) syscall: refactor OsSysCalls for deeper errno latching (envoyproxy#4111) thrift_proxy: fix oneway bugs (envoyproxy#4025) Do not crash when converting YAML to JSON fails (envoyproxy#4110) config: allow unknown fields flag (take 2) (envoyproxy#4096) Use a jittered backoff strategy for handling HdsDelegate stream/connection failures (envoyproxy#4108) bazel: use GCS remote cache (envoyproxy#4050) Add thread local cache of overload action states (envoyproxy#4090) Added TCP healthcheck capabilities to the HdsDelegate (envoyproxy#4079) secret: add secret provider interface and use it for TlsCertificates (envoyproxy#4086) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>

bazel: use GCS remote cache

72f153e

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan mentioned this pull request Aug 3, 2018

[WIP] bazel: use GCS remote cache #4049

Closed

mattklein123 reviewed Aug 3, 2018

View reviewed changes

setup read-only cache where key is not available

757581c

Signed-off-by: Lizan Zhou <zlizan@google.com>

PiotrSikora reviewed Aug 3, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into ci_gcs_remote_cache

c297830

htuch suggested changes Aug 5, 2018

View reviewed changes

htuch self-assigned this Aug 5, 2018

move env variables to circleci.yaml

ddcc25d

Signed-off-by: Lizan Zhou <zlizan@google.com>

PiotrSikora previously approved these changes Aug 6, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into ci_gcs_remote_cache

6ec7ebf

lizan changed the title ~~[WIP] bazel: use GCS remote cache~~ bazel: use GCS remote cache Aug 7, 2018

Empty commit to trigger CI again

e49e7db

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan dismissed PiotrSikora’s stale review via e49e7db August 7, 2018 04:23

lizan force-pushed the ci_gcs_remote_cache branch from 7feefdc to e49e7db Compare August 7, 2018 04:23

Merge remote-tracking branch 'upstream/master' into ci_gcs_remote_cache

58fcfdf

htuch reviewed Aug 7, 2018

View reviewed changes

ci/setup_gcs_cache.sh Outdated

set -e

if [[ ! -z "${BAZEL_REMOTE_CACHE}" ]]; then

Copy link

Member

htuch Aug 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove superfluous blank lines here and below.

remove blank lines

e4f2dd4

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan added 2 commits August 7, 2018 14:20

Merge remote-tracking branch 'upstream/master' into ci_gcs_remote_cache

9f415e8

Merge remote-tracking branch 'upstream/master' into ci_gcs_remote_cache

2c95de0

htuch previously approved these changes Aug 10, 2018

View reviewed changes

lizan added 2 commits August 10, 2018 12:39

disable cache for coverage

7a79526

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into ci_gcs_remote_cache

877957c

lizan dismissed htuch’s stale review via 877957c August 10, 2018 19:39

htuch approved these changes Aug 12, 2018

View reviewed changes

htuch merged commit 7309c14 into master Aug 12, 2018

htuch deleted the ci_gcs_remote_cache branch August 12, 2018 01:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bazel: use GCS remote cache #4050

bazel: use GCS remote cache #4050

lizan commented Aug 3, 2018

mattklein123 Aug 3, 2018

lizan Aug 3, 2018

PiotrSikora Aug 3, 2018

htuch Aug 5, 2018

htuch Aug 5, 2018

htuch left a comment

htuch Aug 5, 2018

htuch Aug 5, 2018

htuch Aug 5, 2018

lizan Aug 6, 2018

htuch Aug 5, 2018

lizan commented Aug 6, 2018

PiotrSikora commented Aug 6, 2018

lizan commented Aug 7, 2018

lizan commented Aug 7, 2018

PiotrSikora commented Aug 7, 2018

lizan commented Aug 7, 2018

lizan commented Aug 7, 2018

htuch left a comment

htuch Aug 7, 2018

htuch commented Aug 7, 2018

lizan commented Aug 7, 2018

htuch commented Aug 10, 2018

lizan commented Aug 10, 2018

lizan commented Aug 10, 2018

htuch left a comment

bazel: use GCS remote cache #4050

bazel: use GCS remote cache #4050

Conversation

lizan commented Aug 3, 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

htuch left a 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

lizan commented Aug 6, 2018

PiotrSikora commented Aug 6, 2018

lizan commented Aug 7, 2018

lizan commented Aug 7, 2018

PiotrSikora commented Aug 7, 2018

lizan commented Aug 7, 2018

lizan commented Aug 7, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Aug 7, 2018

lizan commented Aug 7, 2018

htuch commented Aug 10, 2018

lizan commented Aug 10, 2018

lizan commented Aug 10, 2018

htuch left a comment

Choose a reason for hiding this comment