Skip to content

Commit

Permalink
[Cherrypick] Introduce options for remote cache key scrubbing.
Browse files Browse the repository at this point in the history
Introduce an experimental flag for remote cache key scrubbing.

This PR introduces the --experimental_remote_scrubbing_config flag, which can
be used to scrub platform-dependent data from the key used to retrieve and
store action results from a remote or disk cache This way, actions executing
on different platforms but targeting the same platform may be able to share
cache entries.

To avoid flag proliferation and make for a nicer API, the scrubbing parameters
are supplied via an external file in text proto format.

This is a simplified implementation of one of the ideas described in [1],
highly influenced by Olivier Notteghem's earlier proposal [2], intended to
provide a simple yet flexible API to enable further experimentation. It must
be used with care, as incorrect settings can compromise build correctness.

[1] https://docs.google.com/document/d/1uMPj2s0TlHSIKSngqOkWJoeqOtKzaxQLtBrRfYif3Lo/edit?usp=sharing
[2] bazelbuild#18669

Closes bazelbuild#19523.

PiperOrigin-RevId: 571947013
Change-Id: Ic8da59946ea6d6811b840aee01548746aab2eba0

Fix previous commit
  • Loading branch information
tjgq authored and oliviernotteghem committed Nov 29, 2023
1 parent cbe9ce5 commit fa43f9e
Show file tree
Hide file tree
Showing 21 changed files with 1,003 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public PersistentIndexMap(Path mapFile, Path journalFile, Clock clock) throws IO
@Override
protected boolean updateJournal() {
long time = clock.nanoTime();
if (SAVE_INTERVAL_NS == 0 || time > nextUpdate) {
if (time > nextUpdate) {
nextUpdate = time + SAVE_INTERVAL_NS;
return true;
}
Expand Down
15 changes: 15 additions & 0 deletions src/main/java/com/google/devtools/build/lib/remote/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ java_library(
"AbstractActionInputPrefetcher.java",
"ToplevelArtifactsDownloader.java",
"LeaseService.java",
"Scrubber.java",
],
),
exports = [
Expand Down Expand Up @@ -84,6 +85,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/exec/local",
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/remote:scrubber",
"//src/main/java/com/google/devtools/build/lib/remote/common",
"//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception",
"//src/main/java/com/google/devtools/build/lib/remote/disk",
Expand Down Expand Up @@ -239,3 +241,16 @@ java_library(
"//third_party:jsr305",
],
)

java_library(
name = "scrubber",
srcs = ["Scrubber.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/protobuf:remote_scrubbing_java_proto",
"//third_party:guava",
"//third_party:jsr305",
"//third_party/protobuf:protobuf_java",
],
)
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
import com.google.devtools.build.lib.remote.RemoteExecutionService.ActionResultMetadata.DirectoryMetadata;
import com.google.devtools.build.lib.remote.RemoteExecutionService.ActionResultMetadata.FileMetadata;
import com.google.devtools.build.lib.remote.RemoteExecutionService.ActionResultMetadata.SymlinkMetadata;
import com.google.devtools.build.lib.remote.Scrubber.SpawnScrubber;
import com.google.devtools.build.lib.remote.common.BulkTransferException;
import com.google.devtools.build.lib.remote.common.OperationObserver;
import com.google.devtools.build.lib.remote.common.OutputDigestMismatchException;
Expand Down Expand Up @@ -175,6 +176,8 @@ public class RemoteExecutionService {
private final AtomicBoolean shutdown = new AtomicBoolean(false);
private final AtomicBoolean buildInterrupted = new AtomicBoolean(false);

@Nullable private final Scrubber scrubber;

public RemoteExecutionService(
Executor executor,
Reporter reporter,
Expand Down Expand Up @@ -205,6 +208,7 @@ public RemoteExecutionService(
if (remoteOptions.remoteMerkleTreeCacheSize != 0) {
merkleTreeCacheBuilder.maximumSize(remoteOptions.remoteMerkleTreeCacheSize);
}
this.scrubber = remoteOptions.scrubber;
this.merkleTreeCache = merkleTreeCacheBuilder.build();

this.tempPathGenerator = tempPathGenerator;
Expand All @@ -213,12 +217,13 @@ public RemoteExecutionService(
this.scheduler = Schedulers.from(executor, /*interruptibleWorker=*/ true);
}

static Command buildCommand(
private Command buildCommand(
Collection<? extends ActionInput> outputs,
List<String> arguments,
ImmutableMap<String, String> env,
@Nullable Platform platform,
RemotePathResolver remotePathResolver) {
RemotePathResolver remotePathResolver,
@Nullable SpawnScrubber spawnScrubber) {
Command.Builder command = Command.newBuilder();
ArrayList<String> outputFiles = new ArrayList<>();
ArrayList<String> outputDirectories = new ArrayList<>();
Expand All @@ -239,6 +244,9 @@ static Command buildCommand(
command.setPlatform(platform);
}
for (String arg : arguments) {
if (spawnScrubber != null) {
arg = spawnScrubber.transformArgument(arg);
}
command.addArguments(decodeBytestringUtf8(arg));
}
// Sorting the environment pairs by variable name.
Expand Down Expand Up @@ -360,15 +368,18 @@ private SortedMap<PathFragment, ActionInput> buildOutputDirMap(Spawn spawn) {
}

private MerkleTree buildInputMerkleTree(
Spawn spawn, SpawnExecutionContext context, ToolSignature toolSignature)
Spawn spawn,
SpawnExecutionContext context,
ToolSignature toolSignature,
@Nullable SpawnScrubber spawnScrubber)
throws IOException, ForbiddenActionInputException {
// Add output directories to inputs so that they are created as empty directories by the
// executor. The spec only requires the executor to create the parent directory of an output
// directory, which differs from the behavior of both local and sandboxed execution.
SortedMap<PathFragment, ActionInput> outputDirMap = buildOutputDirMap(spawn);
boolean useMerkleTreeCache = remoteOptions.remoteMerkleTreeCache;
if (toolSignature != null) {
// Marking tool files is not yet supported in conjunction with the merkle tree cache.
if (toolSignature != null || spawnScrubber != null) {
// The Merkle tree cache is not yet compatible with scrubbing or marking tool files.
useMerkleTreeCache = false;
}
if (useMerkleTreeCache) {
Expand All @@ -377,15 +388,23 @@ private MerkleTree buildInputMerkleTree(
remotePathResolver.walkInputs(
spawn,
context,
(Object nodeKey, InputWalker walker) -> {
subMerkleTrees.add(
buildMerkleTreeVisitor(
nodeKey, walker, metadataProvider, context.getPathResolver()));
});
(Object nodeKey, InputWalker walker) ->
subMerkleTrees.add(
buildMerkleTreeVisitor(
nodeKey,
walker,
inputMetadataProvider,
context.getPathResolver(),
spawnScrubber)));
if (!outputDirMap.isEmpty()) {
subMerkleTrees.add(
MerkleTree.build(
outputDirMap, metadataProvider, execRoot, context.getPathResolver(), digestUtil));
outputDirMap,
inputMetadataProvider,
execRoot,
context.getPathResolver(),
/* spawnScrubber= */ null,
digestUtil));
}
return MerkleTree.merge(subMerkleTrees, digestUtil);
} else {
Expand All @@ -406,6 +425,7 @@ private MerkleTree buildInputMerkleTree(
context.getMetadataProvider(),
execRoot,
context.getPathResolver(),
spawnScrubber,
digestUtil);
}
}
Expand All @@ -414,7 +434,8 @@ private MerkleTree buildMerkleTreeVisitor(
Object nodeKey,
InputWalker walker,
MetadataProvider metadataProvider,
ArtifactPathResolver artifactPathResolver)
ArtifactPathResolver artifactPathResolver,
@Nullable SpawnScrubber spawnScrubber)
throws IOException, ForbiddenActionInputException {
// Deduplicate concurrent computations for the same node. It's not possible to use
// MerkleTreeCache#get(key, loader) because the loading computation may cause other nodes to be
Expand All @@ -426,7 +447,8 @@ private MerkleTree buildMerkleTreeVisitor(
// No preexisting cache entry, so we must do the computation ourselves.
try {
freshFuture.complete(
uncachedBuildMerkleTreeVisitor(walker, metadataProvider, artifactPathResolver));
uncachedBuildMerkleTreeVisitor(
walker, metadataProvider, artifactPathResolver, spawnScrubber));
} catch (Exception e) {
freshFuture.completeExceptionally(e);
}
Expand All @@ -451,6 +473,7 @@ public MerkleTree uncachedBuildMerkleTreeVisitor(
InputWalker walker,
MetadataProvider metadataProvider,
ArtifactPathResolver artifactPathResolver)
@Nullable SpawnScrubber scrubber)
throws IOException, ForbiddenActionInputException {
ConcurrentLinkedQueue<MerkleTree> subMerkleTrees = new ConcurrentLinkedQueue<>();
subMerkleTrees.add(
Expand All @@ -459,18 +482,19 @@ public MerkleTree uncachedBuildMerkleTreeVisitor(
metadataProvider,
execRoot,
artifactPathResolver,
scrubber,
digestUtil));
walker.visitNonLeaves(
(Object subNodeKey, InputWalker subWalker) -> {
subMerkleTrees.add(
buildMerkleTreeVisitor(
subNodeKey, subWalker, metadataProvider, artifactPathResolver));
subNodeKey, subWalker, metadataProvider, artifactPathResolver, scrubber));
});
return MerkleTree.merge(subMerkleTrees, digestUtil);
}

@Nullable
private static ByteString buildSalt(Spawn spawn) {
private static ByteString buildSalt(Spawn spawn, @Nullable SpawnScrubber spawnScrubber) {
String workspace =
spawn.getExecutionInfo().get(ExecutionRequirements.DIFFERENTIATE_WORKSPACE_CACHE);
if (workspace != null) {
Expand All @@ -482,7 +506,12 @@ private static ByteString buildSalt(Spawn spawn) {
return platform.toByteString();
}

return null;
if (spawnScrubber != null) {
saltBuilder.setScrubSalt(
CacheSalt.ScrubSalt.newBuilder().setSalt(spawnScrubber.getSalt()).build());
}

return saltBuilder.build().toByteString();
}

/**
Expand Down Expand Up @@ -517,7 +546,9 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
remoteActionBuildingSemaphore.acquire();
try {
ToolSignature toolSignature = getToolSignature(spawn, context);
final MerkleTree merkleTree = buildInputMerkleTree(spawn, context, toolSignature);
SpawnScrubber spawnScrubber = scrubber != null ? scrubber.forSpawn(spawn) : null;
final MerkleTree merkleTree =
buildInputMerkleTree(spawn, context, toolSignature, spawnScrubber);

// Get the remote platform properties.
Platform platform = PlatformUtils.getPlatformProto(spawn, remoteOptions);
Expand All @@ -535,7 +566,8 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
spawn.getArguments(),
spawn.getEnvironment(),
platform,
remotePathResolver);
remotePathResolver,
spawnScrubber);
Digest commandHash = digestUtil.compute(command);
Action action =
Utils.buildAction(
Expand All @@ -544,7 +576,7 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
platform,
context.getTimeout(),
Spawns.mayBeCachedRemotely(spawn),
buildSalt(spawn));
buildSalt(spawn, spawnScrubber));

ActionKey actionKey = digestUtil.computeActionKey(action);

Expand Down Expand Up @@ -1449,7 +1481,8 @@ public void uploadInputsIfNotPresent(RemoteAction action, boolean force)
Spawn spawn = action.getSpawn();
SpawnExecutionContext context = action.getSpawnExecutionContext();
ToolSignature toolSignature = getToolSignature(spawn, context);
merkleTree = buildInputMerkleTree(spawn, context, toolSignature);
SpawnScrubber spawnScrubber = scrubber != null ? scrubber.forSpawn(spawn) : null;
merkleTree = buildInputMerkleTree(spawn, context, toolSignature, spawnScrubber);
}

remoteExecutionCache.ensureInputsPresent(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,14 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
FailureDetails.RemoteOptions.Code.EXECUTION_WITH_INVALID_CACHE);
}

boolean enableScrubbing = remoteOptions.scrubber != null;
if (enableScrubbing && enableRemoteExecution) {

throw createOptionsExitException(
"Cannot combine remote cache key scrubbing with remote execution",
FailureDetails.RemoteOptions.Code.EXECUTION_WITH_SCRUBBING);
}

// TODO(bazel-team): Consider adding a warning or more validation if the remoteDownloadRegex is
// used without Build without the Bytes.
if (!remoteOptions.remoteOutputsMode.downloadAllOutputs()) {
Expand Down
Loading

0 comments on commit fa43f9e

Please sign in to comment.