Skip to content

Commit

Permalink
5.x: Remote: Ignore blobs referenced in BEP if the generating action …
Browse files Browse the repository at this point in the history
…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 <chiwang@google.com>
  • Loading branch information
brentleyjones and coeuvre authored Dec 7, 2021
1 parent 24a340a commit bfc2413
Show file tree
Hide file tree
Showing 11 changed files with 361 additions and 50 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down
25 changes: 17 additions & 8 deletions src/main/java/com/google/devtools/build/lib/actions/Spawns.java
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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());
Expand All @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
Expand All @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -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 =
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Loading

0 comments on commit bfc2413

Please sign in to comment.