Dont query remote cache but always use bytestream protocol

When using `--experimental_remote_build_event_upload=minimal`, instead of querying remote cache to decide whether convert local path of a file to `bytestream://`, it now always use `bytestream://` for BEP referenced files. Closes bazelbuild#16999. Closes bazelbuild#17025. PiperOrigin-RevId: 502351401 Change-Id: Ic858367ffaf3c2a2c20db88ada85fbbe1d93aae8
joomcode · Jan 16, 2023 · b0c5eb3 · b0c5eb3
1 parent 2b22bf0
commit b0c5eb3
Show file tree

Hide file tree

Showing 4 changed files with 97 additions and 58 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
@@ -22,6 +22,7 @@
 import build.bazel.remote.execution.v2.RequestMetadata;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import com.google.common.eventbus.Subscribe;
 import com.google.common.util.concurrent.ListenableFuture;
@@ -47,7 +48,6 @@
 import io.reactivex.rxjava3.schedulers.Schedulers;
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -107,10 +107,16 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
   }
 
   public void omitFile(Path file) {
+    Preconditions.checkState(
+        remoteBuildEventUploadMode != RemoteBuildEventUploadMode.MINIMAL,
+        "Cannot omit file in MINIMAL mode");
     omittedFiles.add(file.asFragment());
   }
 
   public void omitTree(Path treeRoot) {
+    Preconditions.checkState(
+        remoteBuildEventUploadMode != RemoteBuildEventUploadMode.MINIMAL,
+        "Cannot omit tree in MINIMAL mode");
     omittedTreeRoots.add(treeRoot.asFragment());
   }
 
@@ -209,10 +215,6 @@ private static void processQueryResult(
     }
   }
 
-  private boolean shouldQuery(PathMetadata path) {
-    return path.getDigest() != null && !path.isRemote() && !path.isDirectory();
-  }
-
   private boolean shouldUpload(PathMetadata path) {
     boolean result =
         path.getDigest() != null && !path.isRemote() && !path.isDirectory() && !path.isOmitted();
@@ -239,7 +241,8 @@ private Single<List<PathMetadata>> queryRemoteCache(
     List<PathMetadata> filesToQuery = new ArrayList<>();
     Set<Digest> digestsToQuery = new HashSet<>();
     for (PathMetadata path : paths) {
-      if (shouldQuery(path)) {
+      // Query remote cache for files even if omitted from uploading
+      if (shouldUpload(path) || path.isOmitted()) {
         filesToQuery.add(path);
         digestsToQuery.add(path.getDigest());
       } else {
@@ -329,16 +332,19 @@ private Single<PathConverter> upload(Set<Path> files) {
                         reporterUploadError(e);
                         return new PathMetadata(
                             file,
-                            /*digest=*/ null,
-                            /*directory=*/ false,
-                            /*remote=*/ false,
-                            /*omitted=*/ false);
+                            /* digest= */ null,
+                            /* directory= */ false,
+                            /* remote= */ false,
+                            /* omitted= */ false);
                       }
                     })
                 .collect(Collectors.toList())
                 .flatMap(paths -> queryRemoteCache(remoteCache, context, paths))
                 .flatMap(paths -> uploadLocalFiles(remoteCache, context, paths))
-                .map(paths -> new PathConverterImpl(remoteServerInstanceName, paths)),
+                .map(
+                    paths ->
+                        new PathConverterImpl(
+                            remoteServerInstanceName, paths, remoteBuildEventUploadMode)),
         RemoteCache::release);
   }
 
@@ -377,17 +383,23 @@ private static class PathConverterImpl implements PathConverter {
     private final Set<Path> skippedPaths;
     private final Set<Path> localPaths;
 
-    PathConverterImpl(String remoteServerInstanceName, List<PathMetadata> uploads) {
+    PathConverterImpl(
+        String remoteServerInstanceName,
+        List<PathMetadata> uploads,
+        RemoteBuildEventUploadMode remoteBuildEventUploadMode) {
       Preconditions.checkNotNull(uploads);
       this.remoteServerInstanceName = remoteServerInstanceName;
-      pathToDigest = new HashMap<>(uploads.size());
+      pathToDigest = Maps.newHashMapWithExpectedSize(uploads.size());
       ImmutableSet.Builder<Path> skippedPaths = ImmutableSet.builder();
       ImmutableSet.Builder<Path> localPaths = ImmutableSet.builder();
       for (PathMetadata pair : uploads) {
         Path path = pair.getPath();
         Digest digest = pair.getDigest();
         if (digest != null) {
-          if (pair.isRemote()) {
+          // Always use bytestream:// in MINIMAL mode
+          if (remoteBuildEventUploadMode == RemoteBuildEventUploadMode.MINIMAL) {
+            pathToDigest.put(path, digest);
+          } else if (pair.isRemote()) {
             pathToDigest.put(path, digest);
           } else {
             localPaths.add(path);

diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java
@@ -302,9 +302,8 @@ public String getTypeDescription() {
           "If set to 'all', all local outputs referenced by BEP are uploaded to remote cache.\n"
               + "If set to 'minimal', local outputs referenced by BEP are not uploaded to the"
               + " remote cache, except for files that are important to the consumers of BEP (e.g."
-              + " test logs and timing profile).\n"
-              + "file:// scheme is used for the paths of local files and bytestream:// scheme is"
-              + " used for the paths of (already) uploaded files.\n"
+              + " test logs and timing profile). bytestream:// scheme is always used for the uri of"
+              + " files even if they are missing from remote cache.\n"
               + "Default to 'all'.")
   public RemoteBuildEventUploadMode remoteBuildEventUploadMode;
 

diff --git a/src/test/shell/bazel/remote/remote_build_event_uploader_test.sh b/src/test/shell/bazel/remote/remote_build_event_uploader_test.sh
@@ -71,6 +71,32 @@ else
   declare -r EXE_EXT=""
 fi
 
+BEP_JSON=bep.json
+
+function expect_bes_file_uploaded() {
+  local file=$1
+  if [[ $(cat $BEP_JSON) =~ ${file}\",\"uri\":\"bytestream://localhost:${worker_port}/blobs/([^/]*) ]]; then
+    if ! remote_cas_file_exist ${BASH_REMATCH[1]}; then
+      cat $BEP_JSON >> $TEST_log && append_remote_cas_files $TEST_log && fail "$file is not uploaded"
+    fi
+  else
+    cat $BEP_JSON > $TEST_log
+    fail "$file is not converted to bytestream://"
+  fi
+}
+
+function expect_bes_file_not_uploaded() {
+  local file=$1
+  if [[ $(cat $BEP_JSON) =~ ${file}\",\"uri\":\"bytestream://localhost:${worker_port}/blobs/([^/]*) ]]; then
+    if remote_cas_file_exist ${BASH_REMATCH[1]}; then
+     cat $BEP_JSON >> $TEST_log && append_remote_cas_files $TEST_log && fail "$file is uploaded"
+    fi
+  else
+    cat $BEP_JSON > $TEST_log
+    fail "$file is not converted to bytestream://"
+  fi
+}
+
 function test_upload_minimal_convert_paths_for_existed_blobs() {
   mkdir -p a
   cat > a/BUILD <<EOF
@@ -84,12 +110,11 @@ EOF
   bazel build \
       --remote_executor=grpc://localhost:${worker_port} \
       --experimental_remote_build_event_upload=minimal \
-      --build_event_json_file=bep.json \
+      --build_event_json_file=$BEP_JSON \
       //a:foo >& $TEST_log || fail "Failed to build"
 
-  cat bep.json > $TEST_log
-  expect_log "a:foo.*bytestream://" || fail "paths for existed blobs should be converted"
-  expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
+  expect_bes_file_uploaded foo.txt
+  expect_bes_file_uploaded command.profile.gz
 }
 
 function test_upload_minimal_doesnt_upload_missing_blobs() {
@@ -106,12 +131,11 @@ EOF
   bazel build \
       --remote_executor=grpc://localhost:${worker_port} \
       --experimental_remote_build_event_upload=minimal \
-      --build_event_json_file=bep.json \
+      --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 uploaded"
-  expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
+  expect_bes_file_not_uploaded foo.txt
+  expect_bes_file_uploaded command.profile.gz
 }
 
 function test_upload_minimal_respect_no_upload_results() {
@@ -128,12 +152,11 @@ EOF
       --remote_cache=grpc://localhost:${worker_port} \
       --remote_upload_local_results=false \
       --experimental_remote_build_event_upload=minimal \
-      --build_event_json_file=bep.json \
+      --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 uploaded"
-  expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
+  expect_bes_file_not_uploaded foo.txt
+  expect_bes_file_uploaded command.profile.gz
 }
 
 function test_upload_minimal_respect_no_upload_results_combined_cache() {
@@ -154,12 +177,11 @@ EOF
       --incompatible_remote_results_ignore_disk \
       --remote_upload_local_results=false \
       --experimental_remote_build_event_upload=minimal \
-      --build_event_json_file=bep.json \
+      --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 uploaded"
-  expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
+  expect_bes_file_not_uploaded foo.txt
+  expect_bes_file_uploaded command.profile.gz
   remote_cas_files="$(count_remote_cas_files)"
   [[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote cas entries, not $remote_cas_files"
   disk_cas_files="$(count_disk_cas_files $cache_dir)"
@@ -187,12 +209,11 @@ EOF
   bazel build \
       --remote_executor=grpc://localhost:${worker_port} \
       --experimental_remote_build_event_upload=minimal \
-      --build_event_json_file=bep.json \
+      --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://"
+  expect_bes_file_not_uploaded foo.txt
+  expect_bes_file_uploaded command.profile.gz
 }
 
 function test_upload_minimal_trees_doesnt_upload_missing_blobs() {
@@ -204,8 +225,9 @@ def _gen_output_dir_impl(ctx):
         outputs = [output_dir],
         inputs = [],
         command = """
-          mkdir -p $1/sub; \
-          index=0; while ((index<10)); do echo $index >$1/$index.txt; index=$(($index+1)); done
+          echo 0 > $1/0.txt
+          echo 1 > $1/1.txt
+          mkdir -p $1/sub
           echo "Shuffle, duffle, muzzle, muff" > $1/sub/bar
         """,
         arguments = [output_dir.path],
@@ -233,13 +255,13 @@ EOF
   bazel build \
       --remote_executor=grpc://localhost:${worker_port} \
       --experimental_remote_build_event_upload=minimal \
-      --build_event_json_file=bep.json \
+      --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 tree files are uploaded"
-  expect_not_log "a/dir/.*bytestream://" || fail "local tree files are uploaded"
-  expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
+  expect_bes_file_not_uploaded dir/0.txt
+  expect_bes_file_not_uploaded dir/1.txt
+  expect_bes_file_not_uploaded dir/sub/bar
+  expect_bes_file_uploaded command.profile.gz
 }
 
 function test_upload_minimal_upload_testlogs() {
@@ -259,14 +281,13 @@ EOF
   bazel test \
       --remote_executor=grpc://localhost:${worker_port} \
       --experimental_remote_build_event_upload=minimal \
-      --build_event_json_file=bep.json \
+      --build_event_json_file=$BEP_JSON \
       //a:test >& $TEST_log || fail "Failed to build"
 
-  cat bep.json > $TEST_log
-  expect_not_log "test.sh.*bytestream://" || fail "test script is uploaded"
-  expect_log "test.log.*bytestream://" || fail "should upload test.log"
-  expect_log "test.xml.*bytestream://" || fail "should upload test.xml"
-  expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
+  expect_bes_file_not_uploaded test.sh
+  expect_bes_file_uploaded test.log
+  expect_bes_file_uploaded test.xml
+  expect_bes_file_uploaded command.profile.gz
 }
 
 function test_upload_minimal_upload_buildlogs() {
@@ -283,13 +304,12 @@ EOF
   bazel build \
       --remote_executor=grpc://localhost:${worker_port} \
       --experimental_remote_build_event_upload=minimal \
-      --build_event_json_file=bep.json \
+      --build_event_json_file=$BEP_JSON \
       //a:foo >& $TEST_log || true
 
-  cat bep.json > $TEST_log
-  expect_log "stdout.*bytestream://" || fail "should upload stdout"
-  expect_log "stderr.*bytestream://" || fail "should upload stderr"
-  expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
+  expect_bes_file_uploaded stdout
+  expect_bes_file_uploaded stderr
+  expect_bes_file_uploaded command.profile.gz
 }
 
 function test_upload_minimal_upload_profile() {
@@ -306,11 +326,10 @@ EOF
       --remote_executor=grpc://localhost:${worker_port} \
       --experimental_remote_build_event_upload=minimal \
       --profile=mycommand.profile.gz \
-      --build_event_json_file=bep.json \
+      --build_event_json_file=$BEP_JSON \
       //a:foo >& $TEST_log || fail "Failed to build"
 
-  cat bep.json > $TEST_log
-  expect_log "mycommand.profile.gz.*bytestream://" || fail "should upload profile data"
+  expect_bes_file_uploaded "mycommand.profile.gz"
 }
 
 run_suite "Remote build event uploader tests"
diff --git a/src/test/shell/bazel/remote/remote_utils.sh b/src/test/shell/bazel/remote/remote_utils.sh
@@ -96,3 +96,12 @@ function count_remote_cas_files() {
     echo 0
   fi
 }
+
+function remote_cas_file_exist() {
+  local file=$1
+  [ -f "$cas_path/cas/${file:0:2}/$file" ]
+}
+
+function append_remote_cas_files() {
+  find "$cas_path/cas" -type f >> $1
+}