From bfc24139d93f8643686d91596ba347df2e01966a Mon Sep 17 00:00:00 2001 From: Brentley Jones Date: Tue, 7 Dec 2021 10:06:23 -0600 Subject: [PATCH] 5.x: Remote: Ignore blobs referenced in BEP if the generating action cannot be cached remotely. (#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 #14338. PiperOrigin-RevId: 414708472 (cherry picked from commit 0d7d44d5b2e65741812ca644542a24c7618e193f) * 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 #14338. Closes #14338. PiperOrigin-RevId: 414721139 (cherry picked from commit 2ec457df0f0ea1b7314c0906ae5c611bf488d868) Co-authored-by: chiwang --- .../lib/actions/ExecutionRequirements.java | 3 + .../devtools/build/lib/actions/Spawns.java | 25 ++- .../ByteStreamBuildEventArtifactUploader.java | 37 ++++- ...reamBuildEventArtifactUploaderFactory.java | 29 +++- .../lib/remote/RemoteExecutionService.java | 23 +-- .../build/lib/remote/RemoteModule.java | 65 ++++++-- .../remote/disk/DiskAndRemoteCacheClient.java | 7 +- .../lib/remote/options/RemoteOptions.java | 10 ++ .../devtools/build/lib/remote/util/Utils.java | 52 ++++-- .../bazel/remote/remote_execution_test.sh | 152 ++++++++++++++++++ src/test/shell/bazel/remote/remote_utils.sh | 8 + 11 files changed, 361 insertions(+), 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 index dcdd4358b497c4..a21f1417860655 100644 --- 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 index 182fc0c6e6d448..82db8bd1a7c9e6 100644 --- 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 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 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 index ffb8ee87ca71ff..2f38d0193651fa 100644 --- 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 omittedFiles = Sets.newConcurrentHashSet(); + private final Set 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 upload(Set 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 pathToDigest; private final Set skippedPaths; + private final Set localPaths; - PathConverterImpl(String remoteServerInstanceName, List uploads) { + PathConverterImpl( + String remoteServerInstanceName, List uploads, Set 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/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java index 31edf398e75653..23fa5d513d1137 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java +++ b/src/main/java/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 index 8cb9a9e8762716..9ea3ea212d80f9 100644 --- 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 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 index 5b857ca0eaced4..252abc94622141 100644 --- 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,7 +246,7 @@ 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) { @@ -252,7 +254,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { 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> 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 index df2974c1e919ad..684a971f72a506 100644 --- 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 uploadFile( RemoteActionExecutionContext context, Digest digest, Path file) { ListenableFuture 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( 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 index 5518672924d369..d7dc2ec61bdcc2 100644 --- 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 @@ -269,6 +269,16 @@ public String getTypeDescription() { help = "Whether to upload locally executed action results to the remote cache.") public boolean remoteUploadLocalResults; + @Option( + name = "incompatible_remote_build_event_upload_respect_no_cache", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.UNKNOWN}, + 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 = "incompatible_remote_results_ignore_disk", defaultValue = "false", diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java index a13faab04af198..8c4099a5931bd6 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java @@ -25,6 +25,7 @@ import com.google.common.base.Preconditions; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.AsyncCallable; import com.google.common.util.concurrent.FluentFuture; @@ -71,6 +72,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.Locale; +import java.util.Map; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.function.BiFunction; @@ -596,31 +598,59 @@ public static boolean shouldAcceptCachedResultFromCombinedCache( } public static boolean shouldUploadLocalResultsToRemoteCache( - RemoteOptions remoteOptions, @Nullable Spawn spawn) { + RemoteOptions remoteOptions, @Nullable Map executionInfo) { return remoteOptions.remoteUploadLocalResults - && (spawn == null || Spawns.mayBeCachedRemotely(spawn)); + && (executionInfo == null + || (Spawns.mayBeCachedRemotely(executionInfo) + && !executionInfo.containsKey(ExecutionRequirements.NO_REMOTE_CACHE_UPLOAD))); } - public static boolean shouldUploadLocalResultsToDiskCache( + public static boolean shouldUploadLocalResultsToRemoteCache( RemoteOptions remoteOptions, @Nullable Spawn spawn) { + ImmutableMap executionInfo = null; + if (spawn != null) { + executionInfo = spawn.getExecutionInfo(); + } + return shouldUploadLocalResultsToRemoteCache(remoteOptions, executionInfo); + } + + public static boolean shouldUploadLocalResultsToDiskCache( + RemoteOptions remoteOptions, @Nullable Map executionInfo) { if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) { - return spawn == null || Spawns.mayBeCached(spawn); + return executionInfo == null || Spawns.mayBeCached(executionInfo); } else { - return remoteOptions.remoteUploadLocalResults && (spawn == null || Spawns.mayBeCached(spawn)); + return remoteOptions.remoteUploadLocalResults + && (executionInfo == null || Spawns.mayBeCached(executionInfo)); } } - public static boolean shouldUploadLocalResultsToCombinedDisk( + public static boolean shouldUploadLocalResultsToDiskCache( RemoteOptions remoteOptions, @Nullable Spawn spawn) { + ImmutableMap executionInfo = null; + if (spawn != null) { + executionInfo = spawn.getExecutionInfo(); + } + return shouldUploadLocalResultsToDiskCache(remoteOptions, executionInfo); + } + + public static boolean shouldUploadLocalResultsToCombinedDisk( + RemoteOptions remoteOptions, @Nullable Map executionInfo) { if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) { // If --incompatible_remote_results_ignore_disk is set, we treat the disk cache part as local - // cache. Actions - // which are tagged with `no-remote-cache` can still hit the disk cache. - return spawn == null || Spawns.mayBeCached(spawn); + // cache. Actions which are tagged with `no-remote-cache` can still hit the disk cache. + return shouldUploadLocalResultsToDiskCache(remoteOptions, executionInfo); } else { // Otherwise, it's treated as a remote cache and disabled for `no-remote-cache`. - return remoteOptions.remoteUploadLocalResults - && (spawn == null || Spawns.mayBeCachedRemotely(spawn)); + return shouldUploadLocalResultsToRemoteCache(remoteOptions, executionInfo); + } + } + + public static boolean shouldUploadLocalResultsToCombinedDisk( + RemoteOptions remoteOptions, @Nullable Spawn spawn) { + ImmutableMap executionInfo = null; + if (spawn != null) { + executionInfo = spawn.getExecutionInfo(); } + return shouldUploadLocalResultsToCombinedDisk(remoteOptions, executionInfo); } } diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 286e508c07c3f0..ad9b686382632f 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -1900,6 +1900,42 @@ EOF expect_not_log "remote cache hit" } +function test_tag_no_remote_cache_upload() { + mkdir -p a + cat > a/BUILD <<'EOF' +genrule( + name = "foo", + srcs = [], + outs = ["foo.txt"], + cmd = "echo \"foo\" > \"$@\"", +) +EOF + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + --modify_execution_info=.*=+no-remote-cache-upload \ + //a:foo >& $TEST_log || "Failed to build //a:foo" + + remote_ac_files="$(count_remote_ac_files)" + [[ "$remote_ac_files" == 0 ]] || fail "Expected 0 remote action cache entries, not $remote_ac_files" + + # populate the cache + bazel clean + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + //a:foo >& $TEST_log || "Failed to build //a:foo" + + bazel clean + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + --modify_execution_info=.*=+no-remote-cache-upload \ + //a:foo >& $TEST_log || "Failed to build //a:foo" + + expect_log "remote cache hit" +} + function test_tag_no_remote_cache_for_disk_cache() { mkdir -p a cat > a/BUILD <<'EOF' @@ -3260,4 +3296,120 @@ EOF //a:consumer >& $TEST_log || fail "Failed to build without remote cache" } +function test_uploader_respsect_no_cache() { + mkdir -p a + cat > a/BUILD < \$@", + tags = ["no-cache"], +) +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 >& $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" +} + +function test_uploader_respsect_no_cache_trees() { + mkdir -p a + cat > a/output_dir.bzl <<'EOF' +def _gen_output_dir_impl(ctx): + output_dir = ctx.actions.declare_directory(ctx.attr.outdir) + ctx.actions.run_shell( + 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 "Shuffle, duffle, muzzle, muff" > $1/sub/bar + """, + arguments = [output_dir.path], + execution_requirements = {"no-cache": ""}, + ) + return [ + DefaultInfo(files = depset(direct = [output_dir])), + ] +gen_output_dir = rule( + implementation = _gen_output_dir_impl, + attrs = { + "outdir": attr.string(mandatory = True), + }, +) +EOF + + cat > a/BUILD <& $TEST_log || fail "Failed to build" + + cat bep.json > $TEST_log + expect_not_log "a:foo.*bytestream://" || fail "local tree files are converted" + expect_not_log "a/dir/.*bytestream://" || fail "local tree files are converted" + expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data" +} + +function test_uploader_respsect_no_upload_results() { + mkdir -p a + cat > a/BUILD < \$@", +) +EOF + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + --remote_upload_local_results=false \ + --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" +} + +function test_uploader_respsect_no_upload_results_combined_cache() { + mkdir -p a + cat > a/BUILD < \$@", +) +EOF + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + --disk_cache="${TEST_TMPDIR}/disk_cache" \ + --remote_upload_local_results=false \ + --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" + remote_cas_files="$(count_remote_cas_files)" + [[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote action cache entries, not $remote_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 index 9a0191a9ced131..876376a1dd02df 100644 --- a/src/test/shell/bazel/remote/remote_utils.sh +++ b/src/test/shell/bazel/remote/remote_utils.sh @@ -78,3 +78,11 @@ function count_remote_ac_files() { echo 0 fi } + +function count_remote_cas_files() { + if [ -d "$cas_path/cas" ]; then + expr $(find "$cas_path/cas" -type f | wc -l) + else + echo 0 + fi +}