From 99d2fbe19508e91c59391f68a77e166fa06e4050 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 21 Dec 2021 08:54:13 -0800 Subject: [PATCH] Remote: Don't upload BES referenced blobs to disk cache. Fixes https://github.com/bazelbuild/bazel/issues/14435. Closes #14451. PiperOrigin-RevId: 417629899 --- .../ByteStreamBuildEventArtifactUploader.java | 2 +- .../common/RemoteActionExecutionContext.java | 19 ++++++++- .../SimpleRemoteActionExecutionContext.java | 9 +++- .../remote/disk/DiskAndRemoteCacheClient.java | 17 +++++--- .../bazel/remote/remote_execution_test.sh | 42 ++++++++++++++++--- src/test/shell/bazel/remote/remote_utils.sh | 10 +++++ 6 files changed, 84 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java index 2f38d0193651fa..7c16c2973542f0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java @@ -252,7 +252,7 @@ private Single upload(Set files) { RequestMetadata metadata = TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "bes-upload", null); - RemoteActionExecutionContext context = RemoteActionExecutionContext.create(metadata); + RemoteActionExecutionContext context = RemoteActionExecutionContext.createForBES(metadata); return Single.using( remoteCache::retain, diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java index 52fafe3703b7e9..c27eeb90463fbe 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java @@ -20,6 +20,14 @@ /** A context that provide remote execution related information for executing an action remotely. */ public interface RemoteActionExecutionContext { + /** The type of the context. */ + enum Type { + REMOTE_EXECUTION, + BUILD_EVENT_SERVICE, + } + + /** Returns the {@link Type} of the context. */ + Type getType(); /** Returns the {@link Spawn} of the action being executed or {@code null}. */ @Nullable @@ -46,7 +54,8 @@ default ActionExecutionMetadata getSpawnOwner() { /** Creates a {@link SimpleRemoteActionExecutionContext} with given {@link RequestMetadata}. */ static RemoteActionExecutionContext create(RequestMetadata metadata) { - return new SimpleRemoteActionExecutionContext(/*spawn=*/ null, metadata, new NetworkTime()); + return new SimpleRemoteActionExecutionContext( + /*type=*/ Type.REMOTE_EXECUTION, /*spawn=*/ null, metadata, new NetworkTime()); } /** @@ -54,6 +63,12 @@ static RemoteActionExecutionContext create(RequestMetadata metadata) { * RequestMetadata}. */ static RemoteActionExecutionContext create(@Nullable Spawn spawn, RequestMetadata metadata) { - return new SimpleRemoteActionExecutionContext(spawn, metadata, new NetworkTime()); + return new SimpleRemoteActionExecutionContext( + /*type=*/ Type.REMOTE_EXECUTION, spawn, metadata, new NetworkTime()); + } + + static RemoteActionExecutionContext createForBES(RequestMetadata metadata) { + return new SimpleRemoteActionExecutionContext( + /*type=*/ Type.BUILD_EVENT_SERVICE, /*spawn=*/ null, metadata, new NetworkTime()); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/SimpleRemoteActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/remote/common/SimpleRemoteActionExecutionContext.java index 86d9501a7eb13c..1b099457962490 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/SimpleRemoteActionExecutionContext.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/SimpleRemoteActionExecutionContext.java @@ -20,17 +20,24 @@ /** A {@link RemoteActionExecutionContext} implementation */ public class SimpleRemoteActionExecutionContext implements RemoteActionExecutionContext { + private final Type type; private final Spawn spawn; private final RequestMetadata requestMetadata; private final NetworkTime networkTime; public SimpleRemoteActionExecutionContext( - Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) { + Type type, Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) { + this.type = type; this.spawn = spawn; this.requestMetadata = requestMetadata; this.networkTime = networkTime; } + @Override + public Type getType() { + return type; + } + @Nullable @Override public Spawn getSpawn() { diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java index 684a971f72a506..e5a936cf66d8bc 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java @@ -74,13 +74,14 @@ public void close() { @Override public ListenableFuture uploadFile( RemoteActionExecutionContext context, Digest digest, Path file) { + // For BES upload, we only upload to the remote cache. + if (context.getType() == RemoteActionExecutionContext.Type.BUILD_EVENT_SERVICE) { + return remoteCache.uploadFile(context, digest, file); + } + ListenableFuture future = diskCache.uploadFile(context, digest, file); - boolean uploadForSpawn = context.getSpawn() != null; - // If not upload for spawn e.g. for build event artifacts, we always upload files to remote - // cache. - if (!uploadForSpawn - || options.isRemoteExecutionEnabled() + if (options.isRemoteExecutionEnabled() || shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) { future = Futures.transformAsync( @@ -113,6 +114,12 @@ public ListenableFuture> findMissingDigests( if (options.isRemoteExecutionEnabled()) { return remoteCache.findMissingDigests(context, digests); } + + // For BES upload, we only check the remote cache. + if (context.getType() == RemoteActionExecutionContext.Type.BUILD_EVENT_SERVICE) { + return remoteCache.findMissingDigests(context, digests); + } + ListenableFuture> diskQuery = diskCache.findMissingDigests(context, digests); if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) { diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index ad9b686382632f..a963114f797102 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3296,7 +3296,7 @@ EOF //a:consumer >& $TEST_log || fail "Failed to build without remote cache" } -function test_uploader_respsect_no_cache() { +function test_uploader_respect_no_cache() { mkdir -p a cat > a/BUILD < a/output_dir.bzl <<'EOF' def _gen_output_dir_impl(ctx): @@ -3365,7 +3365,7 @@ EOF expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data" } -function test_uploader_respsect_no_upload_results() { +function test_uploader_respect_no_upload_results() { mkdir -p a cat > a/BUILD < a/BUILD < a/BUILD < \$@", + tags = ["no-cache"], +) +EOF + + cache_dir=$(mktemp -d) + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + --disk_cache=$cache_dir \ + --incompatible_remote_build_event_upload_respect_no_cache \ + --build_event_json_file=bep.json \ + //a:foo >& $TEST_log || fail "Failed to build" + + cat bep.json > $TEST_log + expect_not_log "a:foo.*bytestream://" || fail "local files are converted" + expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data" + + disk_cas_files="$(count_disk_cas_files $cache_dir)" + [[ "$disk_cas_files" == 0 ]] || fail "Expected 0 disk cas entries, not $disk_cas_files" } run_suite "Remote execution and remote cache tests" diff --git a/src/test/shell/bazel/remote/remote_utils.sh b/src/test/shell/bazel/remote/remote_utils.sh index 876376a1dd02df..6317c1ffd608fe 100644 --- a/src/test/shell/bazel/remote/remote_utils.sh +++ b/src/test/shell/bazel/remote/remote_utils.sh @@ -71,6 +71,16 @@ function count_disk_ac_files() { fi } +# Pass in the root of the disk cache and count number of files under /cas directory +# output int to stdout +function count_disk_cas_files() { + if [ -d "$1/cas" ]; then + expr $(find "$1/cas" -type f | wc -l) + else + echo 0 + fi +} + function count_remote_ac_files() { if [ -d "$cas_path/ac" ]; then expr $(find "$cas_path/ac" -type f | wc -l)