From dc59d9e8f7937f2e317c042e8da8f97ba6b1237e Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Thu, 23 Dec 2021 16:13:09 +0100 Subject: [PATCH] [5.x] Make remote BES uploader better (#14472) * Remote: Don't upload BES referenced blobs to disk cache. Fixes https://github.com/bazelbuild/bazel/issues/14435. Closes #14451. PiperOrigin-RevId: 417629899 * Remote: Make `--incompatible_remote_build_event_upload_respect_no_cache` working with alias. Fixes https://github.com/bazelbuild/bazel/issues/14456. Closes #14461. PiperOrigin-RevId: 417637635 * Remote: Make --incompatible_remote_build_event_upload_respect_no_cache working with --incompatible_remote_results_ignore_disk. Fixes https://github.com/bazelbuild/bazel/issues/14463. Closes #14468. PiperOrigin-RevId: 417984062 --- .../ByteStreamBuildEventArtifactUploader.java | 2 +- .../lib/remote/RemoteExecutionService.java | 23 ++--- .../build/lib/remote/RemoteModule.java | 6 +- .../common/RemoteActionExecutionContext.java | 19 +++- .../SimpleRemoteActionExecutionContext.java | 9 +- .../remote/disk/DiskAndRemoteCacheClient.java | 17 +++- .../bazel/remote/remote_execution_test.sh | 99 +++++++++++++++++-- src/test/shell/bazel/remote/remote_utils.sh | 10 ++ 8 files changed, 154 insertions(+), 31 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/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 9ea3ea212d80f9..8cb9a9e8762716 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -356,19 +356,6 @@ public boolean shouldAcceptCachedResult(Spawn spawn) { } } - public static boolean shouldUploadLocalResults( - RemoteOptions remoteOptions, @Nullable Map executionInfo) { - if (useRemoteCache(remoteOptions)) { - if (useDiskCache(remoteOptions)) { - return shouldUploadLocalResultsToCombinedDisk(remoteOptions, executionInfo); - } else { - return shouldUploadLocalResultsToRemoteCache(remoteOptions, executionInfo); - } - } else { - return shouldUploadLocalResultsToDiskCache(remoteOptions, executionInfo); - } - } - /** * Returns {@code true} if the local results of the {@code spawn} should be uploaded to remote * cache. @@ -378,7 +365,15 @@ public boolean shouldUploadLocalResults(Spawn spawn) { return false; } - return shouldUploadLocalResults(remoteOptions, spawn.getExecutionInfo()); + if (useRemoteCache(remoteOptions)) { + if (useDiskCache(remoteOptions)) { + return shouldUploadLocalResultsToCombinedDisk(remoteOptions, spawn); + } else { + return shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn); + } + } else { + return shouldUploadLocalResultsToDiskCache(remoteOptions, spawn); + } } /** Returns {@code true} if the spawn may be executed remotely. */ diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index df4e38cf5df2de..0e165d15217e87 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -754,12 +754,14 @@ private void parseNoCacheOutputs(AnalysisResult analysisResult) { } for (ConfiguredTarget configuredTarget : analysisResult.getTargetsToBuild()) { + // This will either dereference an alias chain, or return the final ConfiguredTarget. + configuredTarget = configuredTarget.getActual(); + if (configuredTarget instanceof RuleConfiguredTarget) { RuleConfiguredTarget ruleConfiguredTarget = (RuleConfiguredTarget) configuredTarget; for (ActionAnalysisMetadata action : ruleConfiguredTarget.getActions()) { boolean uploadLocalResults = - RemoteExecutionService.shouldUploadLocalResults( - remoteOptions, action.getExecutionInfo()); + Utils.shouldUploadLocalResultsToRemoteCache(remoteOptions, action.getExecutionInfo()); if (!uploadLocalResults) { for (Artifact output : action.getOutputs()) { if (output.isTreeArtifact()) { 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..344a85a047bca1 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/BUILD < \$@", + tags = ["no-cache"], +) + +alias( + name = 'foo-alias', + actual = '//a:foo', +) +EOF + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + --incompatible_remote_build_event_upload_respect_no_cache \ + --build_event_json_file=bep.json \ + //a:foo-alias >& $TEST_log || fail "Failed to build" + + cat bep.json > $TEST_log + expect_not_log "a:foo.*bytestream://" + expect_log "command.profile.gz.*bytestream://" +} + +function test_uploader_respect_no_cache_trees() { mkdir -p a cat > a/output_dir.bzl <<'EOF' def _gen_output_dir_impl(ctx): @@ -3365,7 +3392,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" +} + +function test_uploader_incompatible_remote_results_ignore_disk() { + mkdir -p a + cat > a/BUILD < \$@", + tags = ["no-remote"], +) +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 \ + --incompatible_remote_results_ignore_disk \ + --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)" + # foo.txt, stdout and stderr for action 'foo' + [[ "$disk_cas_files" == 3 ]] || fail "Expected 3 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)