Remote: Don't upload BES referenced blobs to disk cache.

coeuvre · coeuvre · commit 99d2fbe19508 · 2021-12-23T14:33:32.000Z
Fixes #14435. Closes #14451. PiperOrigin-RevId: 417629899
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/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/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
@@ -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,7 @@ EOF
   expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
 }
 
-function test_uploader_respsect_no_cache_trees() {
+function test_uploader_respect_no_cache_trees() {
   mkdir -p a
   cat > 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 <<EOF
 genrule(
@@ -3387,7 +3387,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 +3397,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 +3411,35 @@ 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"
 }
 
 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)
