Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote: Ignore blobs referenced in BEP if the generating action cannot be cached remotely. #14338

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
17 changes: 15 additions & 2 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,6 +19,7 @@
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 {
Expand All @@ -28,8 +29,13 @@ private Spawns() {}
* 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());
}

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. */
Expand All @@ -39,6 +45,13 @@ public static boolean mayBeCachedRemotely(Spawn spawn) {
&& !spawn.getExecutionInfo().containsKey(ExecutionRequirements.NO_REMOTE_CACHE);
}

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. */
public static boolean mayBeExecutedRemotely(Spawn spawn) {
return !spawn.getExecutionInfo().containsKey(ExecutionRequirements.LOCAL)
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,9 +303,11 @@ 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.localPaths = localPaths;
this.remoteServerInstanceName = remoteServerInstanceName;
pathToDigest = new HashMap<>(uploads.size());
ImmutableSet.Builder<Path> skippedPaths = ImmutableSet.builder();
Expand All @@ -301,6 +326,11 @@ private static class PathConverterImpl implements PathConverter {
@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,9 @@ 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 +55,20 @@ class ByteStreamBuildEventArtifactUploaderFactory implements BuildEventArtifactU

@Override
public BuildEventArtifactUploader create(CommandEnvironment env) {
return new ByteStreamBuildEventArtifactUploader(
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 Expand Up @@ -1140,6 +1145,7 @@ public void uploadOutputs(RemoteAction action, SpawnResult spawnResult)
throws InterruptedException, ExecException {
checkState(!shutdown.get(), "shutdown");
checkState(shouldUploadLocalResults(action.spawn), "spawn shouldn't upload local result");
checkNotNull(remoteCache, "remoteCache can't be null");
checkState(
SpawnResult.Status.SUCCESS.equals(spawnResult.status()) && spawnResult.exitCode() == 0,
"shouldn't upload outputs of failed local action");
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,7 @@ 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 +727,40 @@ 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 +864,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 +908,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, "remoteOutputsMode must not be null");

if (actionContextProvider == null) {
return;
Expand All @@ -897,7 +932,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 +948,22 @@ 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,11 @@ 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