Skip to content

Commit

Permalink
Handle remote cache eviction when uploading inputs for remote actions.
Browse files Browse the repository at this point in the history
Previously, we handle remote cache eviction when downloading inputs for local actions. However, it's possible that when Bazel need to re-execute an remote action, it detected that some inputs are missing from remote CAS. In this case, Bazel will try to upload inputs by reading from local filesystem. Since the inputs were generated remotely, not downloaded and evicted remotely, the upload will fail with FileNotFoundException.

This CL changes the code to correctly handles above case by reading through ActionFS when uploading inputs and propagate CacheNotFoundException.

Related to #16660.

PiperOrigin-RevId: 512568547
Change-Id: I3a28cadbb6285fa3727e1603f37abf8843c093c9
  • Loading branch information
coeuvre committed Feb 27, 2023
1 parent 2a8643f commit 2aa724e
Show file tree
Hide file tree
Showing 15 changed files with 289 additions and 65 deletions.
44 changes: 13 additions & 31 deletions src/main/java/com/google/devtools/build/lib/remote/Chunker.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import java.io.PushbackInputStream;
import java.util.NoSuchElementException;
import java.util.Objects;
import java.util.function.Supplier;

/**
* Splits a data source into one or more {@link Chunk}s of at most {@code chunkSize} bytes.
Expand Down Expand Up @@ -92,8 +91,7 @@ public boolean equals(Object o) {
return false;
}
Chunk other = (Chunk) o;
return other.offset == offset
&& other.data.equals(data);
return other.offset == offset && other.data.equals(data);
}

@Override
Expand All @@ -102,7 +100,12 @@ public int hashCode() {
}
}

private final Supplier<InputStream> dataSupplier;
/** A supplier that provide data as {@link InputStream}. */
public interface ChunkDataSupplier {
InputStream get() throws IOException;
}

private final ChunkDataSupplier dataSupplier;
private final long size;
private final int chunkSize;
private final Chunk emptyChunk;
Expand All @@ -117,7 +120,7 @@ public int hashCode() {
// lazily on the first call to next(), as opposed to opening it in the constructor or on reset().
private boolean initialized;

Chunker(Supplier<InputStream> dataSupplier, long size, int chunkSize, boolean compressed) {
Chunker(ChunkDataSupplier dataSupplier, long size, int chunkSize, boolean compressed) {
this.dataSupplier = checkNotNull(dataSupplier);
this.size = size;
this.chunkSize = chunkSize;
Expand Down Expand Up @@ -287,7 +290,7 @@ public static class Builder {
private int chunkSize = getDefaultChunkSize();
protected long size;
private boolean compressed;
protected Supplier<InputStream> inputStream;
protected ChunkDataSupplier inputStream;

@CanIgnoreReturnValue
public Builder setInput(byte[] data) {
Expand All @@ -310,14 +313,7 @@ public Builder setInput(long size, InputStream in) {
public Builder setInput(long size, Path file) {
checkState(inputStream == null);
this.size = size;
inputStream =
() -> {
try {
return file.getInputStream();
} catch (IOException e) {
throw new RuntimeException(e);
}
};
inputStream = file::getInputStream;
return this;
}

Expand All @@ -326,30 +322,16 @@ public Builder setInput(long size, ActionInput actionInput, Path execRoot) {
checkState(inputStream == null);
this.size = size;
if (actionInput instanceof VirtualActionInput) {
inputStream =
() -> {
try {
return ((VirtualActionInput) actionInput).getBytes().newInput();
} catch (IOException e) {
throw new RuntimeException(e);
}
};
inputStream = () -> ((VirtualActionInput) actionInput).getBytes().newInput();
} else {
inputStream =
() -> {
try {
return ActionInputHelper.toInputPath(actionInput, execRoot).getInputStream();
} catch (IOException e) {
throw new RuntimeException(e);
}
};
inputStream = () -> ActionInputHelper.toInputPath(actionInput, execRoot).getInputStream();
}
return this;
}

@CanIgnoreReturnValue
@VisibleForTesting
protected final Builder setInputSupplier(Supplier<InputStream> inputStream) {
protected final Builder setInputSupplier(ChunkDataSupplier inputStream) {
this.inputStream = inputStream;
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
import com.google.common.util.concurrent.ListenableFuture;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
Expand Down Expand Up @@ -371,10 +372,14 @@ private MerkleTree buildInputMerkleTree(
spawn,
context,
(Object nodeKey, InputWalker walker) -> {
subMerkleTrees.add(buildMerkleTreeVisitor(nodeKey, walker, metadataProvider));
subMerkleTrees.add(
buildMerkleTreeVisitor(
nodeKey, walker, metadataProvider, context.getPathResolver()));
});
if (!outputDirMap.isEmpty()) {
subMerkleTrees.add(MerkleTree.build(outputDirMap, metadataProvider, execRoot, digestUtil));
subMerkleTrees.add(
MerkleTree.build(
outputDirMap, metadataProvider, execRoot, context.getPathResolver(), digestUtil));
}
return MerkleTree.merge(subMerkleTrees, digestUtil);
} else {
Expand All @@ -394,31 +399,44 @@ private MerkleTree buildInputMerkleTree(
toolSignature == null ? ImmutableSet.of() : toolSignature.toolInputs,
context.getMetadataProvider(),
execRoot,
context.getPathResolver(),
digestUtil);
}
}

private MerkleTree buildMerkleTreeVisitor(
Object nodeKey, InputWalker walker, MetadataProvider metadataProvider)
Object nodeKey,
InputWalker walker,
MetadataProvider metadataProvider,
ArtifactPathResolver artifactPathResolver)
throws IOException, ForbiddenActionInputException {
MerkleTree result = merkleTreeCache.getIfPresent(nodeKey);
if (result == null) {
result = uncachedBuildMerkleTreeVisitor(walker, metadataProvider);
result = uncachedBuildMerkleTreeVisitor(walker, metadataProvider, artifactPathResolver);
merkleTreeCache.put(nodeKey, result);
}
return result;
}

@VisibleForTesting
public MerkleTree uncachedBuildMerkleTreeVisitor(
InputWalker walker, MetadataProvider metadataProvider)
InputWalker walker,
MetadataProvider metadataProvider,
ArtifactPathResolver artifactPathResolver)
throws IOException, ForbiddenActionInputException {
ConcurrentLinkedQueue<MerkleTree> subMerkleTrees = new ConcurrentLinkedQueue<>();
subMerkleTrees.add(
MerkleTree.build(walker.getLeavesInputMapping(), metadataProvider, execRoot, digestUtil));
MerkleTree.build(
walker.getLeavesInputMapping(),
metadataProvider,
execRoot,
artifactPathResolver,
digestUtil));
walker.visitNonLeaves(
(Object subNodeKey, InputWalker subWalker) -> {
subMerkleTrees.add(buildMerkleTreeVisitor(subNodeKey, subWalker, metadataProvider));
subMerkleTrees.add(
buildMerkleTreeVisitor(
subNodeKey, subWalker, metadataProvider, artifactPathResolver));
});
return MerkleTree.merge(subMerkleTrees, digestUtil);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,11 @@ private SpawnResult handleError(
catastrophe = true;
} else if (remoteCacheFailed) {
status = Status.REMOTE_CACHE_FAILED;
detailedCode = FailureDetails.Spawn.Code.REMOTE_CACHE_FAILED;
if (remoteOptions.useNewExitCodeForLostInputs) {
detailedCode = FailureDetails.Spawn.Code.REMOTE_CACHE_EVICTED;
} else {
detailedCode = FailureDetails.Spawn.Code.REMOTE_CACHE_FAILED;
}
catastrophe = false;
} else {
status = Status.EXECUTION_FAILED;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.MetadataProvider;
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
Expand Down Expand Up @@ -61,21 +62,25 @@ static DirectoryTree fromActionInputs(
SortedMap<PathFragment, ActionInput> inputs,
MetadataProvider metadataProvider,
Path execRoot,
ArtifactPathResolver artifactPathResolver,
DigestUtil digestUtil)
throws IOException {
return fromActionInputs(inputs, ImmutableSet.of(), metadataProvider, execRoot, digestUtil);
return fromActionInputs(
inputs, ImmutableSet.of(), metadataProvider, execRoot, artifactPathResolver, digestUtil);
}

static DirectoryTree fromActionInputs(
SortedMap<PathFragment, ActionInput> inputs,
Set<PathFragment> toolInputs,
MetadataProvider metadataProvider,
Path execRoot,
ArtifactPathResolver artifactPathResolver,
DigestUtil digestUtil)
throws IOException {
Map<PathFragment, DirectoryNode> tree = new HashMap<>();
int numFiles =
buildFromActionInputs(inputs, toolInputs, metadataProvider, execRoot, digestUtil, tree);
buildFromActionInputs(
inputs, toolInputs, metadataProvider, execRoot, artifactPathResolver, digestUtil, tree);
return new DirectoryTree(tree, numFiles);
}

Expand Down Expand Up @@ -135,6 +140,7 @@ private static int buildFromActionInputs(
Set<PathFragment> toolInputs,
MetadataProvider metadataProvider,
Path execRoot,
ArtifactPathResolver artifactPathResolver,
DigestUtil digestUtil,
Map<PathFragment, DirectoryNode> tree)
throws IOException {
Expand Down Expand Up @@ -164,7 +170,7 @@ private static int buildFromActionInputs(
case REGULAR_FILE:
{
Digest d = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize());
Path inputPath = ActionInputHelper.toInputPath(input, execRoot);
Path inputPath = artifactPathResolver.toPath(input);
boolean childAdded =
currDir.addChild(
FileNode.createExecutable(
Expand All @@ -176,7 +182,13 @@ private static int buildFromActionInputs(
SortedMap<PathFragment, ActionInput> directoryInputs =
explodeDirectory(input.getExecPath(), execRoot);
return buildFromActionInputs(
directoryInputs, toolInputs, metadataProvider, execRoot, digestUtil, tree);
directoryInputs,
toolInputs,
metadataProvider,
execRoot,
artifactPathResolver,
digestUtil,
tree);

case SYMLINK:
{
Expand All @@ -185,7 +197,7 @@ private static int buildFromActionInputs(
"Encountered symlink input '%s', but all source symlinks should have been"
+ " resolved by SkyFrame. This is a bug.",
path);
Path inputPath = ActionInputHelper.toInputPath(input, execRoot);
Path inputPath = artifactPathResolver.toPath(input);
boolean childAdded =
currDir.addChild(
new SymlinkNode(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.google.common.collect.Multimap;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.MetadataProvider;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.SilentCloseable;
Expand Down Expand Up @@ -222,11 +223,13 @@ public static MerkleTree build(
SortedMap<PathFragment, ActionInput> inputs,
MetadataProvider metadataProvider,
Path execRoot,
ArtifactPathResolver artifactPathResolver,
DigestUtil digestUtil)
throws IOException {
try (SilentCloseable c = Profiler.instance().profile("MerkleTree.build(ActionInput)")) {
DirectoryTree tree =
DirectoryTreeBuilder.fromActionInputs(inputs, metadataProvider, execRoot, digestUtil);
DirectoryTreeBuilder.fromActionInputs(
inputs, metadataProvider, execRoot, artifactPathResolver, digestUtil);
return build(tree, digestUtil);
}
}
Expand All @@ -247,12 +250,13 @@ public static MerkleTree build(
Set<PathFragment> toolInputs,
MetadataProvider metadataProvider,
Path execRoot,
ArtifactPathResolver artifactPathResolver,
DigestUtil digestUtil)
throws IOException {
try (SilentCloseable c = Profiler.instance().profile("MerkleTree.build(ActionInput)")) {
DirectoryTree tree =
DirectoryTreeBuilder.fromActionInputs(
inputs, toolInputs, metadataProvider, execRoot, digestUtil);
inputs, toolInputs, metadataProvider, execRoot, artifactPathResolver, digestUtil);
return build(tree, digestUtil);
}
}
Expand Down
Loading

0 comments on commit 2aa724e

Please sign in to comment.