5.x: Remote: Ignore blobs referenced in BEP if the generating action …

…cannot be cached remotely. (bazelbuild#14389) * Remote: Add support for tag no-remote-cache-upload. When executing a spawn that is tagged with no-remote-cache-upload, Bazel still looks up the remote cache. However, its local outputs are not uploaded after local execution. Part of bazelbuild#14338. PiperOrigin-RevId: 414708472 (cherry picked from commit 0d7d44d) * Remote: Ignore blobs referenced in BEP if the generating action cannot be cached remotely. Introduces new flag --incompatible_remote_build_event_upload_respect_no_cache which when set to true, remote uploader won't upload blobs referenced in BEP if the generating action cannot be cached remotely. Part of bazelbuild#14338. Closes bazelbuild#14338. PiperOrigin-RevId: 414721139 (cherry picked from commit 2ec457d) Co-authored-by: chiwang <[email protected]>
EngFlow · Dec 7, 2021 · bfc2413 · bfc2413
1 parent 24a340a
commit bfc2413
Show file tree

Hide file tree

Showing 11 changed files with 361 additions and 50 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java
@@ -194,6 +194,9 @@ public enum WorkerProtocolFormat {
   /** Disables remote caching of a spawn. Note: does not disable remote execution */
   public static final String NO_REMOTE_CACHE = "no-remote-cache";
 
+  /** Disables upload part of remote caching of a spawn. Note: does not disable remote execution */
+  public static final String NO_REMOTE_CACHE_UPLOAD = "no-remote-cache-upload";
+
   /** Disables remote execution of a spawn. Note: does not disable remote caching */
   public static final String NO_REMOTE_EXEC = "no-remote-exec";
 

diff --git a/src/main/java/com/google/devtools/build/lib/actions/Spawns.java b/src/main/java/com/google/devtools/build/lib/actions/Spawns.java
@@ -19,24 +19,33 @@
 import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code;
 import java.io.IOException;
 import java.time.Duration;
+import java.util.Map;
 
 /** Helper methods relating to implementations of {@link Spawn}. */
 public final class Spawns {
   private Spawns() {}
 
-  /**
-   * Returns {@code true} if the result of {@code spawn} may be cached.
-   */
+  /** Returns {@code true} if the result of {@code spawn} may be cached. */
   public static boolean mayBeCached(Spawn spawn) {
-    return !spawn.getExecutionInfo().containsKey(ExecutionRequirements.NO_CACHE)
-        && !spawn.getExecutionInfo().containsKey(ExecutionRequirements.LOCAL);
+    return mayBeCached(spawn.getExecutionInfo());
+  }
+
+  /** Returns {@code true} if the result of {@code spawn} may be cached. */
+  public static boolean mayBeCached(Map<String, String> executionInfo) {
+    return !executionInfo.containsKey(ExecutionRequirements.NO_CACHE)
+        && !executionInfo.containsKey(ExecutionRequirements.LOCAL);
   }
 
   /** Returns {@code true} if the result of {@code spawn} may be cached remotely. */
   public static boolean mayBeCachedRemotely(Spawn spawn) {
-    return mayBeCached(spawn)
-        && !spawn.getExecutionInfo().containsKey(ExecutionRequirements.NO_REMOTE)
-        && !spawn.getExecutionInfo().containsKey(ExecutionRequirements.NO_REMOTE_CACHE);
+    return mayBeCachedRemotely(spawn.getExecutionInfo());
+  }
+
+  /** Returns {@code true} if the result of {@code spawn} may be cached remotely. */
+  public static boolean mayBeCachedRemotely(Map<String, String> executionInfo) {
+    return mayBeCached(executionInfo)
+        && !executionInfo.containsKey(ExecutionRequirements.NO_REMOTE)
+        && !executionInfo.containsKey(ExecutionRequirements.NO_REMOTE_CACHE);
   }
 
   /** Returns {@code true} if {@code spawn} may be executed remotely. */

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.Sets;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile;
 import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader;
@@ -65,6 +66,9 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
   private final AtomicBoolean shutdown = new AtomicBoolean();
   private final Scheduler scheduler;
 
+  private final Set<Path> omittedFiles = Sets.newConcurrentHashSet();
+  private final Set<Path> omittedTreeRoots = Sets.newConcurrentHashSet();
+
   ByteStreamBuildEventArtifactUploader(
       Executor executor,
       ExtendedEventHandler reporter,
@@ -83,6 +87,14 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
     this.scheduler = Schedulers.from(executor);
   }
 
+  public void omitFile(Path file) {
+    omittedFiles.add(file);
+  }
+
+  public void omitTree(Path treeRoot) {
+    omittedTreeRoots.add(treeRoot);
+  }
+
   /** Returns {@code true} if Bazel knows that the file is stored on a remote system. */
   private static boolean isRemoteFile(Path file) {
     return file.getFileSystem() instanceof RemoteActionFileSystem
@@ -124,10 +136,21 @@ public boolean isRemote() {
    * Collects metadata for {@code file}. Depending on the underlying filesystem used this method
    * might do I/O.
    */
-  private static PathMetadata readPathMetadata(Path file) throws IOException {
+  private PathMetadata readPathMetadata(Path file) throws IOException {
     if (file.isDirectory()) {
       return new PathMetadata(file, /* digest= */ null, /* directory= */ true, /* remote= */ false);
     }
+    if (omittedFiles.contains(file)) {
+      return new PathMetadata(file, /*digest=*/ null, /*directory=*/ false, /*remote=*/ false);
+    }
+
+    for (Path treeRoot : omittedTreeRoots) {
+      if (file.startsWith(treeRoot)) {
+        omittedFiles.add(file);
+        return new PathMetadata(file, /*digest=*/ null, /*directory=*/ false, /*remote=*/ false);
+      }
+    }
+
     DigestUtil digestUtil = new DigestUtil(file.getFileSystem().getDigestFunction());
     Digest digest = digestUtil.compute(file);
     return new PathMetadata(file, digest, /* directory= */ false, isRemoteFile(file));
@@ -248,7 +271,7 @@ private Single<PathConverter> upload(Set<Path> files) {
                 .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, omittedFiles)),
         RemoteCache::release);
   }
 
@@ -280,8 +303,10 @@ private static class PathConverterImpl implements PathConverter {
     private final String remoteServerInstanceName;
     private final Map<Path, Digest> pathToDigest;
     private final Set<Path> skippedPaths;
+    private final Set<Path> localPaths;
 
-    PathConverterImpl(String remoteServerInstanceName, List<PathMetadata> uploads) {
+    PathConverterImpl(
+        String remoteServerInstanceName, List<PathMetadata> uploads, Set<Path> localPaths) {
       Preconditions.checkNotNull(uploads);
       this.remoteServerInstanceName = remoteServerInstanceName;
       pathToDigest = new HashMap<>(uploads.size());
@@ -296,11 +321,17 @@ private static class PathConverterImpl implements PathConverter {
         }
       }
       this.skippedPaths = skippedPaths.build();
+      this.localPaths = localPaths;
     }
 
     @Override
     public String apply(Path path) {
       Preconditions.checkNotNull(path);
+
+      if (localPaths.contains(path)) {
+        return String.format("file://%s", path.getPathString());
+      }
+
       Digest digest = pathToDigest.get(path);
       if (digest == null) {
         if (skippedPaths.contains(path)) {

diff --git a/...ava/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java b/...ava/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java
@@ -13,11 +13,14 @@
 // limitations under the License.
 package com.google.devtools.build.lib.remote;
 
+import static com.google.common.base.Preconditions.checkState;
+
 import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.runtime.BuildEventArtifactUploaderFactory;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import java.util.concurrent.Executor;
+import javax.annotation.Nullable;
 
 /** A factory for {@link ByteStreamBuildEventArtifactUploader}. */
 class ByteStreamBuildEventArtifactUploaderFactory implements BuildEventArtifactUploaderFactory {
@@ -30,6 +33,8 @@ class ByteStreamBuildEventArtifactUploaderFactory implements BuildEventArtifactU
   private final String buildRequestId;
   private final String commandId;
 
+  @Nullable private ByteStreamBuildEventArtifactUploader uploader;
+
   ByteStreamBuildEventArtifactUploaderFactory(
       Executor executor,
       ExtendedEventHandler reporter,
@@ -49,13 +54,21 @@ class ByteStreamBuildEventArtifactUploaderFactory implements BuildEventArtifactU
 
   @Override
   public BuildEventArtifactUploader create(CommandEnvironment env) {
-    return new ByteStreamBuildEventArtifactUploader(
-        executor,
-        reporter,
-        verboseFailures,
-        remoteCache.retain(),
-        remoteServerInstanceName,
-        buildRequestId,
-        commandId);
+    checkState(uploader == null, "Already created");
+    uploader =
+        new ByteStreamBuildEventArtifactUploader(
+            executor,
+            reporter,
+            verboseFailures,
+            remoteCache.retain(),
+            remoteServerInstanceName,
+            buildRequestId,
+            commandId);
+    return uploader;
+  }
+
+  @Nullable
+  public ByteStreamBuildEventArtifactUploader get() {
+    return uploader;
   }
 }
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,6 +356,19 @@ 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.
@@ -365,15 +378,7 @@ public boolean shouldUploadLocalResults(Spawn spawn) {
       return false;
     }
 
-    if (useRemoteCache(remoteOptions)) {
-      if (useDiskCache(remoteOptions)) {
-        return shouldUploadLocalResultsToCombinedDisk(remoteOptions, spawn);
-      } else {
-        return shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn);
-      }
-    } else {
-      return shouldUploadLocalResultsToDiskCache(remoteOptions, spawn);
-    }
+    return shouldUploadLocalResults(remoteOptions, spawn.getExecutionInfo());
   }
 
   /** 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
@@ -30,6 +30,7 @@
 import com.google.common.util.concurrent.ListeningScheduledExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
 import com.google.devtools.build.lib.actions.ActionGraph;
 import com.google.devtools.build.lib.actions.ActionInput;
@@ -111,6 +112,7 @@
 import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.ThreadPoolExecutor;
+import javax.annotation.Nullable;
 
 /** RemoteModule provides distributed cache and remote execution for Bazel. */
 public final class RemoteModule extends BlazeModule {
@@ -126,7 +128,7 @@ public final class RemoteModule extends BlazeModule {
 
   private RemoteActionContextProvider actionContextProvider;
   private RemoteActionInputFetcher actionInputFetcher;
-  private RemoteOutputsMode remoteOutputsMode;
+  private RemoteOptions remoteOptions;
   private RemoteOutputService remoteOutputService;
 
   private ChannelFactory channelFactory =
@@ -244,15 +246,15 @@ private void initHttpAndDiskCache(
   public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
     Preconditions.checkState(actionContextProvider == null, "actionContextProvider must be null");
     Preconditions.checkState(actionInputFetcher == null, "actionInputFetcher must be null");
-    Preconditions.checkState(remoteOutputsMode == null, "remoteOutputsMode must be null");
+    Preconditions.checkState(remoteOptions == null, "remoteOptions must be null");
 
     RemoteOptions remoteOptions = env.getOptions().getOptions(RemoteOptions.class);
     if (remoteOptions == null) {
       // Quit if no supported command is being used. See getCommandOptions for details.
       return;
     }
 
-    remoteOutputsMode = remoteOptions.remoteOutputsMode;
+    this.remoteOptions = remoteOptions;
 
     AuthAndTLSOptions authAndTlsOptions = env.getOptions().getOptions(AuthAndTLSOptions.class);
     DigestHashFunction hashFn = env.getRuntime().getFileSystem().getDigestFunction();
@@ -705,8 +707,8 @@ public void afterAnalysis(
       AnalysisResult analysisResult) {
     // The actionContextProvider may be null if remote execution is disabled or if there was an
     // error during initialization.
-    if (remoteOutputsMode != null
-        && remoteOutputsMode.downloadToplevelOutputsOnly()
+    if (remoteOptions != null
+        && remoteOptions.remoteOutputsMode.downloadToplevelOutputsOnly()
         && actionContextProvider != null) {
       boolean isTestCommand = env.getCommandName().equals("test");
       TopLevelArtifactContext artifactContext = request.getTopLevelArtifactContext();
@@ -726,6 +728,41 @@ public void afterAnalysis(
       }
       actionContextProvider.setFilesToDownload(ImmutableSet.copyOf(filesToDownload));
     }
+
+    if (remoteOptions != null && remoteOptions.incompatibleRemoteBuildEventUploadRespectNoCache) {
+      parseNoCacheOutputs(analysisResult);
+    }
+  }
+
+  private void parseNoCacheOutputs(AnalysisResult analysisResult) {
+    if (actionContextProvider == null || actionContextProvider.getRemoteCache() == null) {
+      return;
+    }
+
+    ByteStreamBuildEventArtifactUploader uploader = buildEventArtifactUploaderFactoryDelegate.get();
+    if (uploader == null) {
+      return;
+    }
+
+    for (ConfiguredTarget configuredTarget : analysisResult.getTargetsToBuild()) {
+      if (configuredTarget instanceof RuleConfiguredTarget) {
+        RuleConfiguredTarget ruleConfiguredTarget = (RuleConfiguredTarget) configuredTarget;
+        for (ActionAnalysisMetadata action : ruleConfiguredTarget.getActions()) {
+          boolean uploadLocalResults =
+              RemoteExecutionService.shouldUploadLocalResults(
+                  remoteOptions, action.getExecutionInfo());
+          if (!uploadLocalResults) {
+            for (Artifact output : action.getOutputs()) {
+              if (output.isTreeArtifact()) {
+                uploader.omitTree(output.getPath());
+              } else {
+                uploader.omitFile(output.getPath());
+              }
+            }
+          }
+        }
+      }
+    }
   }
 
   // This is a short-term fix for top-level outputs that are symlinks. Unfortunately, we cannot
@@ -829,7 +866,7 @@ public void afterCommand() throws AbruptExitException {
     remoteDownloaderSupplier.set(null);
     actionContextProvider = null;
     actionInputFetcher = null;
-    remoteOutputsMode = null;
+    remoteOptions = null;
     remoteOutputService = null;
 
     if (failure != null) {
@@ -873,7 +910,7 @@ public void registerActionContexts(
   @Override
   public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) {
     Preconditions.checkState(actionInputFetcher == null, "actionInputFetcher must be null");
-    Preconditions.checkNotNull(remoteOutputsMode, "remoteOutputsMode must not be null");
+    Preconditions.checkNotNull(remoteOptions, "remoteOptions must not be null");
 
     if (actionContextProvider == null) {
       return;
@@ -897,7 +934,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
   @Override
   public OutputService getOutputService() {
     Preconditions.checkState(remoteOutputService == null, "remoteOutputService must be null");
-    if (remoteOutputsMode != null && !remoteOutputsMode.downloadAllOutputs()) {
+    if (remoteOptions != null && !remoteOptions.remoteOutputsMode.downloadAllOutputs()) {
       remoteOutputService = new RemoteOutputService();
     }
     return remoteOutputService;
@@ -913,13 +950,21 @@ public Iterable<Class<? extends OptionsBase>> getCommandOptions(Command command)
   private static class BuildEventArtifactUploaderFactoryDelegate
       implements BuildEventArtifactUploaderFactory {
 
-    private volatile BuildEventArtifactUploaderFactory uploaderFactory;
+    @Nullable private ByteStreamBuildEventArtifactUploaderFactory uploaderFactory;
 
-    public void init(BuildEventArtifactUploaderFactory uploaderFactory) {
+    public void init(ByteStreamBuildEventArtifactUploaderFactory uploaderFactory) {
       Preconditions.checkState(this.uploaderFactory == null);
       this.uploaderFactory = uploaderFactory;
     }
 
+    @Nullable
+    public ByteStreamBuildEventArtifactUploader get() {
+      if (uploaderFactory == null) {
+        return null;
+      }
+      return uploaderFactory.get();
+    }
+
     public void reset() {
       this.uploaderFactory = null;
     }

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
@@ -75,7 +75,12 @@ public void close() {
   public ListenableFuture<Void> uploadFile(
       RemoteActionExecutionContext context, Digest digest, Path file) {
     ListenableFuture<Void> future = diskCache.uploadFile(context, digest, file);
-    if (options.isRemoteExecutionEnabled()
+
+    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()
         || shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {
       future =
           Futures.transformAsync(