[5.x] Make remote BES uploader better (bazelbuild#14472)

* Remote: Don't upload BES referenced blobs to disk cache. Fixes bazelbuild#14435. Closes bazelbuild#14451. PiperOrigin-RevId: 417629899 * Remote: Make `--incompatible_remote_build_event_upload_respect_no_cache` working with alias. Fixes bazelbuild#14456. Closes bazelbuild#14461. PiperOrigin-RevId: 417637635 * Remote: Make --incompatible_remote_build_event_upload_respect_no_cache working with --incompatible_remote_results_ignore_disk. Fixes bazelbuild#14463. Closes bazelbuild#14468. PiperOrigin-RevId: 417984062
databricks · Dec 23, 2021 · dc59d9e · dc59d9e
1 parent 90965b0
commit dc59d9e
Show file tree

Hide file tree

Showing 8 changed files with 154 additions and 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
@@ -252,7 +252,7 @@ private Single<PathConverter> upload(Set<Path> 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
@@ -356,19 +356,6 @@ public boolean shouldAcceptCachedResult(Spawn spawn) {
     }
   }
 
-  public static boolean shouldUploadLocalResults(
-      RemoteOptions remoteOptions, @Nullable Map<String, String> 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
@@ -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
@@ -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,14 +54,21 @@ 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());
   }
 
   /**
    * Creates a {@link SimpleRemoteActionExecutionContext} with given {@link Spawn} and {@link
    * 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/.../java/com/google/devtools/build/lib/remote/common/SimpleRemoteActionExecutionContext.java b/.../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
@@ -74,13 +74,14 @@ public void close() {
   @Override
   public ListenableFuture<Void> 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<Void> 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<ImmutableSet<Digest>> 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<ImmutableSet<Digest>> 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
@@ -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 <<EOF
 genrule(
@@ -3318,7 +3318,34 @@ EOF
   expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
 }
 
-function test_uploader_respsect_no_cache_trees() {
+function test_uploader_alias_action_respect_no_cache() {
+  mkdir -p a
+  cat > a/BUILD <<EOF
+genrule(
+  name = 'foo',
+  outs = ["foo.txt"],
+  cmd = "echo \"foo bar\" > \$@",
+  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 <<EOF
 genrule(
@@ -3387,7 +3414,7 @@ EOF
   expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
 }
 
-function test_uploader_respsect_no_upload_results_combined_cache() {
+function test_uploader_respect_no_upload_results_combined_cache() {
   mkdir -p a
   cat > a/BUILD <<EOF
 genrule(
@@ -3397,9 +3424,11 @@ genrule(
 )
 EOF
 
+  cache_dir=$(mktemp -d)
+
   bazel build \
       --remote_cache=grpc://localhost:${worker_port} \
-      --disk_cache="${TEST_TMPDIR}/disk_cache" \
+      --disk_cache=$cache_dir \
       --remote_upload_local_results=false \
       --incompatible_remote_build_event_upload_respect_no_cache \
       --build_event_json_file=bep.json \
@@ -3409,7 +3438,65 @@ EOF
   expect_not_log "a:foo.*bytestream://" || fail "local files are converted"
   expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
   remote_cas_files="$(count_remote_cas_files)"
-  [[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote action cache entries, not $remote_cas_files"
+  [[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote cas entries, not $remote_cas_files"
+}
+
+function test_uploader_ignore_disk_cache_of_combined_cache() {
+  mkdir -p a
+  cat > a/BUILD <<EOF
+genrule(
+  name = 'foo',
+  outs = ["foo.txt"],
+  cmd = "echo \"foo bar\" > \$@",
+  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 <<EOF
+genrule(
+  name = 'foo',
+  outs = ["foo.txt"],
+  cmd = "echo \"foo bar\" > \$@",
+  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
@@ -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)