Add flag --experimental_remote_build_event_upload

Add flag `--experimental_remote_build_event_upload` which controls the way Bazel uploads files referenced in BEP to remote cache. It defaults to `all` which maintains current behaviour: Bazel uploads all local files referenced by BEP to remote cache and convert their paths to `bytestream://...`. Additionally, `--incompatible_remote_build_event_upload_respect_no_cache` can be set to avoid uploading outputs that are generated by "non-remote-cachable" spawns. If set to `minimal`, local outputs are not uploaded to the remote cache, except for files that are **important** to the consumers of BES (e.g. test logs and timing profile). Paths for files that are already uploaded to the remote cache are converted. `--incompatible_remote_build_event_upload_respect_no_cache` is deprecated in favour of this new flag. Fixes bazelbuild#16151. Closes bazelbuild#16299. PiperOrigin-RevId: 476865036 Change-Id: I4c506f7447a41e8e64a4ed0785e7f20a40ea3b84
aiuto · Oct 12, 2022 · 9e4eb4d · 9e4eb4d
1 parent 10d9615
commit 9e4eb4d
Show file tree

Hide file tree

Showing 8 changed files with 426 additions and 22 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
@@ -23,14 +23,17 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
+import com.google.common.eventbus.Subscribe;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile;
 import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader;
 import com.google.devtools.build.lib.buildeventstream.PathConverter;
+import com.google.devtools.build.lib.buildtool.buildevent.ProfilerStartedEvent;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
 import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.CachePolicy;
+import com.google.devtools.build.lib.remote.options.RemoteBuildEventUploadMode;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -52,12 +55,14 @@
 import java.util.concurrent.CancellationException;
 import java.util.concurrent.Executor;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 
 /** A {@link BuildEventArtifactUploader} backed by {@link RemoteCache}. */
 class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
     implements BuildEventArtifactUploader {
+  private static final Pattern TEST_LOG_PATTERN = Pattern.compile(".*/bazel-out/[^/]*/testlogs/.*");
 
   private final Executor executor;
   private final ExtendedEventHandler reporter;
@@ -73,6 +78,9 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
   private final Set<PathFragment> omittedFiles = Sets.newConcurrentHashSet();
   private final Set<PathFragment> omittedTreeRoots = Sets.newConcurrentHashSet();
   private final XattrProvider xattrProvider;
+  private final RemoteBuildEventUploadMode remoteBuildEventUploadMode;
+
+  @Nullable private Path profilePath;
 
   ByteStreamBuildEventArtifactUploader(
       Executor executor,
@@ -82,7 +90,8 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
       String remoteServerInstanceName,
       String buildRequestId,
       String commandId,
-      XattrProvider xattrProvider) {
+      XattrProvider xattrProvider,
+      RemoteBuildEventUploadMode remoteBuildEventUploadMode) {
     this.executor = executor;
     this.reporter = reporter;
     this.verboseFailures = verboseFailures;
@@ -92,6 +101,7 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
     this.remoteServerInstanceName = remoteServerInstanceName;
     this.scheduler = Schedulers.from(executor);
     this.xattrProvider = xattrProvider;
+    this.remoteBuildEventUploadMode = remoteBuildEventUploadMode;
   }
 
   public void omitFile(Path file) {
@@ -197,8 +207,27 @@ private static void processQueryResult(
     }
   }
 
-  private static boolean shouldUpload(PathMetadata path) {
-    return path.getDigest() != null && !path.isRemote() && !path.isDirectory() && !path.isOmitted();
+  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();
+
+    if (remoteBuildEventUploadMode == RemoteBuildEventUploadMode.MINIMAL) {
+      result = result && (isTestLog(path) || isProfile(path));
+    }
+
+    return result;
+  }
+
+  private boolean isTestLog(PathMetadata path) {
+    return TEST_LOG_PATTERN.matcher(path.getPath().getPathString()).matches();
+  }
+
+  private boolean isProfile(PathMetadata path) {
+    return path.getPath().equals(profilePath);
   }
 
   private Single<List<PathMetadata>> queryRemoteCache(
@@ -207,8 +236,7 @@ private Single<List<PathMetadata>> queryRemoteCache(
     List<PathMetadata> filesToQuery = new ArrayList<>();
     Set<Digest> digestsToQuery = new HashSet<>();
     for (PathMetadata path : paths) {
-      // Query remote cache for files even if omitted from uploading
-      if (shouldUpload(path) || path.isOmitted()) {
+      if (shouldQuery(path)) {
         filesToQuery.add(path);
         digestsToQuery.add(path.getDigest());
       } else {
@@ -256,17 +284,20 @@ private Single<List<PathMetadata>> uploadLocalFiles(
               return toCompletable(
                       () -> remoteCache.uploadFile(context, path.getDigest(), path.getPath()),
                       executor)
-                  .toSingleDefault(path)
+                  .toSingle(
+                      () ->
+                          new PathMetadata(
+                              path.getPath(),
+                              path.getDigest(),
+                              path.isDirectory(),
+                              // set remote to true so the PathConverter will use bytestream://
+                              // scheme to convert the URI for this file
+                              /*remote=*/ true,
+                              path.isOmitted()))
                   .onErrorResumeNext(
                       error -> {
                         reporterUploadError(error);
-                        return Single.just(
-                            new PathMetadata(
-                                path.getPath(),
-                                /*digest=*/ null,
-                                path.isDirectory(),
-                                path.isRemote(),
-                                path.isOmitted()));
+                        return Single.just(path);
                       });
             })
         .collect(Collectors.toList());
@@ -308,6 +339,11 @@ private Single<PathConverter> upload(Set<Path> files) {
         RemoteCache::release);
   }
 
+  @Subscribe
+  public void onProfilerStartedEvent(ProfilerStartedEvent event) {
+    profilePath = event.getProfilePath();
+  }
+
   @Override
   public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
     return toListenableFuture(upload(files.keySet()).subscribeOn(scheduler));
@@ -347,10 +383,12 @@ private static class PathConverterImpl implements PathConverter {
       for (PathMetadata pair : uploads) {
         Path path = pair.getPath();
         Digest digest = pair.getDigest();
-        if (!pair.isRemote() && pair.isOmitted()) {
-          localPaths.add(path);
-        } else if (digest != null) {
-          pathToDigest.put(path, digest);
+        if (digest != null) {
+          if (pair.isRemote()) {
+            pathToDigest.put(path, digest);
+          } else {
+            localPaths.add(path);
+          }
         } else {
           skippedPaths.add(path);
         }

diff --git a/...ava/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java b/...ava/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java
@@ -17,6 +17,7 @@
 
 import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.remote.options.RemoteBuildEventUploadMode;
 import com.google.devtools.build.lib.runtime.BuildEventArtifactUploaderFactory;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import java.util.concurrent.Executor;
@@ -32,6 +33,7 @@ class ByteStreamBuildEventArtifactUploaderFactory implements BuildEventArtifactU
   private final String remoteServerInstanceName;
   private final String buildRequestId;
   private final String commandId;
+  private final RemoteBuildEventUploadMode remoteBuildEventUploadMode;
 
   @Nullable private ByteStreamBuildEventArtifactUploader uploader;
 
@@ -42,14 +44,16 @@ class ByteStreamBuildEventArtifactUploaderFactory implements BuildEventArtifactU
       RemoteCache remoteCache,
       String remoteServerInstanceName,
       String buildRequestId,
-      String commandId) {
+      String commandId,
+      RemoteBuildEventUploadMode remoteBuildEventUploadMode) {
     this.executor = executor;
     this.reporter = reporter;
     this.verboseFailures = verboseFailures;
     this.remoteCache = remoteCache;
     this.remoteServerInstanceName = remoteServerInstanceName;
     this.buildRequestId = buildRequestId;
     this.commandId = commandId;
+    this.remoteBuildEventUploadMode = remoteBuildEventUploadMode;
   }
 
   @Override
@@ -64,7 +68,9 @@ public BuildEventArtifactUploader create(CommandEnvironment env) {
             remoteServerInstanceName,
             buildRequestId,
             commandId,
-            env.getXattrProvider());
+            env.getXattrProvider(),
+            remoteBuildEventUploadMode);
+    env.getEventBus().register(uploader);
     return uploader;
   }
 

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
@@ -68,6 +68,7 @@
 import com.google.devtools.build.lib.remote.common.RemoteExecutionClient;
 import com.google.devtools.build.lib.remote.downloader.GrpcRemoteDownloader;
 import com.google.devtools.build.lib.remote.logging.LoggingInterceptor;
+import com.google.devtools.build.lib.remote.options.RemoteBuildEventUploadMode;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
@@ -650,7 +651,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
             actionContextProvider.getRemoteCache(),
             remoteBytestreamUriPrefix,
             buildRequestId,
-            invocationId));
+            invocationId,
+            remoteOptions.remoteBuildEventUploadMode));
 
     if (enableRemoteDownloader) {
       Downloader fallbackDownloader = null;
@@ -754,7 +756,9 @@ public void afterAnalysis(
       actionContextProvider.setFilesToDownload(ImmutableSet.copyOf(filesToDownload));
     }
 
-    if (remoteOptions != null && remoteOptions.incompatibleRemoteBuildEventUploadRespectNoCache) {
+    if (remoteOptions != null
+        && remoteOptions.remoteBuildEventUploadMode == RemoteBuildEventUploadMode.ALL
+        && remoteOptions.incompatibleRemoteBuildEventUploadRespectNoCache) {
       parseNoCacheOutputs(analysisResult);
     }
   }

diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteBuildEventUploadMode.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteBuildEventUploadMode.java
@@ -0,0 +1,22 @@
+// Copyright 2022 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.remote.options;
+
+/** Describes how to upload local files referenced in BEP to remote cache. */
+public enum RemoteBuildEventUploadMode {
+  // Uploads all local files to remote cache.
+  ALL,
+  // Only upload important local files (e.g. test logs, timing profile) to remote cache.
+  MINIMAL,
+}
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
@@ -276,16 +276,44 @@ public String getTypeDescription() {
       help = "Whether to upload locally executed action results to the remote cache.")
   public boolean remoteUploadLocalResults;
 
+  @Deprecated
   @Option(
       name = "incompatible_remote_build_event_upload_respect_no_cache",
       defaultValue = "false",
       documentationCategory = OptionDocumentationCategory.REMOTE,
       effectTags = {OptionEffectTag.UNKNOWN},
+      deprecationWarning =
+          "--incompatible_remote_build_event_upload_respect_no_cache has been deprecated in favor"
+              + " of --experimental_remote_build_event_upload=minimal.",
       help =
           "If set to true, outputs referenced by BEP are not uploaded to remote cache if the"
               + " generating action cannot be cached remotely.")
   public boolean incompatibleRemoteBuildEventUploadRespectNoCache;
 
+  @Option(
+      name = "experimental_remote_build_event_upload",
+      defaultValue = "all",
+      documentationCategory = OptionDocumentationCategory.REMOTE,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      converter = RemoteBuildEventUploadModeConverter.class,
+      help =
+          "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"
+              + "Default to 'all'.")
+  public RemoteBuildEventUploadMode remoteBuildEventUploadMode;
+
+  /** Build event upload mode flag parser */
+  public static class RemoteBuildEventUploadModeConverter
+      extends EnumConverter<RemoteBuildEventUploadMode> {
+    public RemoteBuildEventUploadModeConverter() {
+      super(RemoteBuildEventUploadMode.class, "remote build event upload");
+    }
+  }
+
   @Option(
       name = "incompatible_remote_results_ignore_disk",
       defaultValue = "true",

diff --git a/...t/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/...t/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java
@@ -53,6 +53,7 @@
 import com.google.devtools.build.lib.remote.common.MissingDigestsFinder;
 import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
 import com.google.devtools.build.lib.remote.grpc.ChannelConnectionFactory;
+import com.google.devtools.build.lib.remote.options.RemoteBuildEventUploadMode;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.remote.util.RxNoGlobalErrorsRule;
@@ -481,7 +482,8 @@ private ByteStreamBuildEventArtifactUploader newArtifactUploader(RemoteCache rem
         /*remoteServerInstanceName=*/ "localhost/instance",
         /*buildRequestId=*/ "none",
         /*commandId=*/ "none",
-        SyscallCache.NO_CACHE);
+        SyscallCache.NO_CACHE,
+        RemoteBuildEventUploadMode.ALL);
   }
 
   private static class StaticMissingDigestsFinder implements MissingDigestsFinder {

diff --git a/src/test/shell/bazel/remote/BUILD b/src/test/shell/bazel/remote/BUILD
@@ -117,3 +117,14 @@ sh_test(
         "@bazel_tools//tools/bash/runfiles",
     ],
 )
+
+sh_test(
+    name = "remote_build_event_uploader_test",
+    srcs = ["remote_build_event_uploader_test.sh"],
+    data = [
+        ":remote_utils",
+        "//src/test/shell/bazel:test-deps",
+        "//src/tools/remote:worker",
+        "@bazel_tools//tools/bash/runfiles",
+    ],
+)