Skip to content

Commit

Permalink
Remote: Ignore blobs referenced in BEP if the generating action canno…
Browse files Browse the repository at this point in the history
…t 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
  • Loading branch information
coeuvre authored and copybara-github committed Dec 7, 2021
1 parent 0d7d44d commit 2ec457d
Show file tree
Hide file tree
Showing 9 changed files with 289 additions and 37 deletions.
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
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Loading

0 comments on commit 2ec457d

Please sign in to comment.