Skip to content

Commit

Permalink
[5.x] Remote: Fix "file not found" error when remote cache is changed…
Browse files Browse the repository at this point in the history 
… from enabled to disabled. (bazelbuild#14321)

* Remote: Invalidate actions if previous build used BwoB and remote cache is changed from enabled to disabled.

Part of bazelbuild#14252.

PiperOrigin-RevId: 410243297

* In `ArchivedTreeArtifact`, make the assumption that the relative output path is always a single segment, and use this to serialize less data.

This assumption holds because the origin of the relative output path (i.e. `bazel-out`) is `BlazeDirectories#getRelativeOutputPath`, which always returns a single-segment string. Instead of passing that around, just extract it from the tree artifact's exec path.

Additionally, the public `ArchivedTreeArtifact#create` method is removed in order to enforce a consistent naming pattern for all instances.

The codec supports custom derived tree roots even though there is currently no such serialization in skyframe (all serialized instances have the default `:archived_tree_artifacts`, but it was easy enough to be flexible).

PiperOrigin-RevId: 406382340

* Remote: Don't load remote metadata from action cache if remote cache is disabled.

Part of bazelbuild#14252.

Closes bazelbuild#14252.

PiperOrigin-RevId: 410448656

Co-authored-by: jhorvitz <jhorvitz@google.com>
  • Loading branch information
@coeuvre @justinhorvitz
coeuvre and justinhorvitz committed Nov 24, 2021
1 parent b587be3 commit f948989
Show file tree
Hide file tree
Showing 24 changed files with 294 additions and 268 deletions.
18 changes: 8 additions & 10 deletions src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
View file
Expand Up @@ -401,19 +401,17 @@ public void repr(Printer printer) {
*
* @param execRoot the exec root in which this action is executed
* @param bulkDeleter a helper to bulk delete outputs to avoid delegating to the filesystem
* @param outputPrefixForArchivedArtifactsCleanup derived output prefix to construct archived tree
* artifacts to be cleaned up. If null, no cleanup is needed.
* @param cleanupArchivedArtifacts whether to clean up archived tree artifacts
*/
protected final void deleteOutputs(
Path execRoot,
ArtifactPathResolver pathResolver,
@Nullable BulkDeleter bulkDeleter,
@Nullable PathFragment outputPrefixForArchivedArtifactsCleanup)
boolean cleanupArchivedArtifacts)
throws IOException, InterruptedException {
Iterable<Artifact> artifactsToDelete =
outputPrefixForArchivedArtifactsCleanup != null
? Iterables.concat(
outputs, archivedTreeArtifactOutputs(outputPrefixForArchivedArtifactsCleanup))
cleanupArchivedArtifacts
? Iterables.concat(outputs, archivedTreeArtifactOutputs())
: outputs;
Iterable<PathFragment> additionalPathOutputsToDelete = getAdditionalPathOutputsToDelete();
Iterable<PathFragment> directoryOutputsToDelete = getDirectoryOutputsToDelete();
Expand Down Expand Up @@ -452,10 +450,10 @@ protected Iterable<PathFragment> getDirectoryOutputsToDelete() {
return ImmutableList.of();
}

private Iterable<Artifact> archivedTreeArtifactOutputs(PathFragment derivedPathPrefix) {
private Iterable<Artifact> archivedTreeArtifactOutputs() {
return Iterables.transform(
Iterables.filter(outputs, Artifact::isTreeArtifact),
tree -> ArchivedTreeArtifact.createForTree((SpecialArtifact) tree, derivedPathPrefix));
tree -> ArchivedTreeArtifact.createForTree((SpecialArtifact) tree));
}

/**
Expand Down Expand Up @@ -581,9 +579,9 @@ public void prepare(
Path execRoot,
ArtifactPathResolver pathResolver,
@Nullable BulkDeleter bulkDeleter,
@Nullable PathFragment outputPrefixForArchivedArtifactsCleanup)
boolean cleanupArchivedArtifacts)
throws IOException, InterruptedException {
deleteOutputs(execRoot, pathResolver, bulkDeleter, outputPrefixForArchivedArtifactsCleanup);
deleteOutputs(execRoot, pathResolver, bulkDeleter, cleanupArchivedArtifacts);
}

@Override
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/com/google/devtools/build/lib/actions/Action.java
View file
Expand Up @@ -20,7 +20,6 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.ConditionallyThreadCompatible;
import com.google.devtools.build.lib.vfs.BulkDeleter;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
import java.util.Map;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -92,7 +91,7 @@ void prepare(
Path execRoot,
ArtifactPathResolver pathResolver,
@Nullable BulkDeleter bulkDeleter,
@Nullable PathFragment outputPrefixForArchivedArtifactsCleanup)
boolean cleanupArchivedArtifacts)
throws IOException, InterruptedException;

/**
Expand Down
33 changes: 14 additions & 19 deletions src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
View file
Expand Up @@ -22,6 +22,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.GoogleLogger;
import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact;
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
Expand Down Expand Up @@ -72,7 +73,6 @@ public class ActionCacheChecker {
private final Predicate<? super Action> executionFilter;
private final ArtifactResolver artifactResolver;
private final CacheConfig cacheConfig;
private final PathFragment derivedPathPrefix;

@Nullable private final ActionCache actionCache; // Null when not enabled.

Expand Down Expand Up @@ -107,8 +107,7 @@ public ActionCacheChecker(
ArtifactResolver artifactResolver,
ActionKeyContext actionKeyContext,
Predicate<? super Action> executionFilter,
@Nullable CacheConfig cacheConfig,
PathFragment derivedPathPrefix) {
@Nullable CacheConfig cacheConfig) {
this.executionFilter = executionFilter;
this.actionKeyContext = actionKeyContext;
this.artifactResolver = artifactResolver;
Expand All @@ -125,7 +124,6 @@ public ActionCacheChecker(
} else {
this.actionCache = null;
}
this.derivedPathPrefix = derivedPathPrefix;
}

public boolean isActionExecutionProhibited(Action action) {
Expand Down Expand Up @@ -310,10 +308,7 @@ private CachedOutputMetadata(
}

private static CachedOutputMetadata loadCachedOutputMetadata(
Action action,
ActionCache.Entry entry,
MetadataHandler metadataHandler,
PathFragment derivedPathPrefix) {
Action action, ActionCache.Entry entry, MetadataHandler metadataHandler) {
ImmutableMap.Builder<Artifact, RemoteFileArtifactValue> remoteFileMetadata =
ImmutableMap.builder();
ImmutableMap.Builder<SpecialArtifact, TreeArtifactValue> mergedTreeMetadata =
Expand Down Expand Up @@ -361,12 +356,9 @@ private static CachedOutputMetadata loadCachedOutputMetadata(
cachedTreeMetadata
.archivedFileValue()
.map(
fileArtifactValue -> {
Artifact.ArchivedTreeArtifact archivedTreeArtifact =
Artifact.ArchivedTreeArtifact.createForTree(parent, derivedPathPrefix);
return ArchivedRepresentation.create(
archivedTreeArtifact, fileArtifactValue);
});
fileArtifactValue ->
ArchivedRepresentation.create(
ArchivedTreeArtifact.createForTree(parent), fileArtifactValue));
}

TreeArtifactValue.Builder merged = TreeArtifactValue.newBuilder(parent);
Expand Down Expand Up @@ -422,7 +414,8 @@ public Token getTokenIfNeedToExecute(
EventHandler handler,
MetadataHandler metadataHandler,
ArtifactExpander artifactExpander,
Map<String, String> remoteDefaultPlatformProperties)
Map<String, String> remoteDefaultPlatformProperties,
boolean isRemoteCacheEnabled)
throws InterruptedException {
// TODO(bazel-team): (2010) For RunfilesAction/SymlinkAction and similar actions that
// produce only symlinks we should not check whether inputs are valid at all - all that matters
Expand Down Expand Up @@ -464,10 +457,12 @@ public Token getTokenIfNeedToExecute(
}
ActionCache.Entry entry = getCacheEntry(action);
CachedOutputMetadata cachedOutputMetadata = null;
// load remote metadata from action cache
if (entry != null && !entry.isCorrupted() && cacheConfig.storeOutputMetadata()) {
cachedOutputMetadata =
loadCachedOutputMetadata(action, entry, metadataHandler, derivedPathPrefix);
if (entry != null
&& !entry.isCorrupted()
&& cacheConfig.storeOutputMetadata()
&& isRemoteCacheEnabled) {
// load remote metadata from action cache
cachedOutputMetadata = loadCachedOutputMetadata(action, entry, metadataHandler);
}

if (mustExecute(
Expand Down
127 changes: 72 additions & 55 deletions src/main/java/com/google/devtools/build/lib/actions/Artifact.java
View file
Expand Up @@ -41,7 +41,6 @@
import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
import com.google.devtools.build.lib.skyframe.serialization.SerializationContext;
import com.google.devtools.build.lib.skyframe.serialization.SerializationException;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
import com.google.devtools.build.lib.starlarkbuildapi.FileApi;
Expand Down Expand Up @@ -309,7 +308,7 @@ public ArchivedTreeArtifact getArchivedTreeArtifact(SpecialArtifact treeArtifact
/** A Predicate that evaluates to true if the Artifact is not a middleman artifact. */
public static final Predicate<Artifact> MIDDLEMAN_FILTER = input -> !input.isMiddlemanArtifact();

protected final ArtifactRoot root;
private final ArtifactRoot root;

private final int hashCode;
private final PathFragment execPath;
Expand Down Expand Up @@ -978,7 +977,7 @@ boolean ownersEqual(Artifact other) {

@Override
public PathFragment getRootRelativePath() {
return root.isExternal() ? getExecPath().subFragment(2) : getExecPath();
return getRoot().isExternal() ? getExecPath().subFragment(2) : getExecPath();
}

@Override
Expand Down Expand Up @@ -1168,10 +1167,10 @@ public SpecialArtifact deserialize(DeserializationContext context, CodedInputStr
* TreeFileArtifact children} (and nothing else) of the tree artifact with their filesystem
* structure, relative to the {@linkplain SpecialArtifact#getExecPath() tree artifact directory}.
*/
@AutoCodec
public static final class ArchivedTreeArtifact extends DerivedArtifact {
private static final PathFragment ARCHIVED_ARTIFACTS_DERIVED_TREE_ROOT =
private static final PathFragment DEFAULT_DERIVED_TREE_ROOT =
PathFragment.create(":archived_tree_artifacts");

private final SpecialArtifact treeArtifact;

private ArchivedTreeArtifact(
Expand All @@ -1180,6 +1179,8 @@ private ArchivedTreeArtifact(
PathFragment execPath,
Object generatingActionKey) {
super(root, execPath, generatingActionKey);
Preconditions.checkArgument(
treeArtifact.isTreeArtifact(), "Not a tree artifact: %s", treeArtifact);
this.treeArtifact = treeArtifact;
}

Expand All @@ -1188,13 +1189,6 @@ public SpecialArtifact getParent() {
return treeArtifact;
}

/** Creates an archived tree artifact with a given {@code root} and {@code execPath}. */
public static ArchivedTreeArtifact create(
SpecialArtifact treeArtifact, ArtifactRoot root, PathFragment execPath) {
return new ArchivedTreeArtifact(
treeArtifact, root, execPath, treeArtifact.getGeneratingActionKey());
}

/**
* Creates an {@link ArchivedTreeArtifact} for a given tree artifact at the path inferred from
* the provided tree.
Expand All @@ -1206,13 +1200,12 @@ public static ArchivedTreeArtifact create(
* {@linkplain ArchivedTreeArtifact artifact} of: {@code
* bazel-out/:archived_tree_artifacts/k8-fastbuild/bin/directory.zip}.
*/
public static ArchivedTreeArtifact createForTree(
SpecialArtifact treeArtifact, PathFragment derivedPathPrefix) {
return createWithCustomDerivedTreeRoot(
public static ArchivedTreeArtifact createForTree(SpecialArtifact treeArtifact) {
return createInternal(
treeArtifact,
derivedPathPrefix,
ARCHIVED_ARTIFACTS_DERIVED_TREE_ROOT,
treeArtifact.getRootRelativePath().replaceName(treeArtifact.getFilename() + ".zip"));
DEFAULT_DERIVED_TREE_ROOT,
treeArtifact.getRootRelativePath().replaceName(treeArtifact.getFilename() + ".zip"),
treeArtifact.getGeneratingActionKey());
}

/**
Expand All @@ -1221,31 +1214,30 @@ public static ArchivedTreeArtifact createForTree(
*
* <p>Example: for a tree artifact with root of {@code bazel-out/k8-fastbuild/bin} returns an
* {@linkplain ArchivedTreeArtifact artifact} of: {@code
* bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin/{rootRelativePath}} with root of: {@code
* bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin}.
* bazel-out/{derivedTreeRoot}/k8-fastbuild/bin/{rootRelativePath}} with root of: {@code
* bazel-out/{derivedTreeRoot}/k8-fastbuild/bin}.
*
* <p>Such artifacts should only be used as outputs of intermediate spawns. Action execution
* results must come from {@link #createForTree}.
*/
public static ArchivedTreeArtifact createWithCustomDerivedTreeRoot(
SpecialArtifact treeArtifact, PathFragment derivedTreeRoot, PathFragment rootRelativePath) {
return createInternal(
treeArtifact, derivedTreeRoot, rootRelativePath, treeArtifact.getGeneratingActionKey());
}

private static ArchivedTreeArtifact createInternal(
SpecialArtifact treeArtifact,
PathFragment derivedPathPrefix,
PathFragment customDerivedTreeRoot,
PathFragment rootRelativePath) {
ArtifactRoot artifactRoot =
createRootForArchivedArtifact(
treeArtifact.getRoot(), derivedPathPrefix, customDerivedTreeRoot);
return create(
treeArtifact, artifactRoot, artifactRoot.getExecPath().getRelative(rootRelativePath));
}

private static ArtifactRoot createRootForArchivedArtifact(
ArtifactRoot treeArtifactRoot,
PathFragment derivedPathPrefix,
PathFragment customDerivedTreeRoot) {
return ArtifactRoot.asDerivedRoot(
getExecRoot(treeArtifactRoot),
// e.g. bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin
RootType.Output,
getExecPathWithinCustomDerivedRoot(
derivedPathPrefix, customDerivedTreeRoot, treeArtifactRoot.getExecPath()));
PathFragment derivedTreeRoot,
PathFragment rootRelativePath,
Object generatingActionKey) {
ArtifactRoot treeRoot = treeArtifact.getRoot();
PathFragment archiveRoot = embedDerivedTreeRoot(treeRoot.getExecPath(), derivedTreeRoot);
return new ArchivedTreeArtifact(
treeArtifact,
ArtifactRoot.asDerivedRoot(getExecRoot(treeRoot), RootType.Output, archiveRoot),
archiveRoot.getRelative(rootRelativePath),
generatingActionKey);
}

/**
Expand All @@ -1255,23 +1247,22 @@ private static ArtifactRoot createRootForArchivedArtifact(
* <p>Example: {@code bazel-out/k8-fastbuild/bin ->
* bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin}.
*/
public static PathFragment getExecPathWithinArchivedArtifactsTree(
PathFragment derivedPathPrefix, PathFragment execPath) {
return getExecPathWithinCustomDerivedRoot(
derivedPathPrefix, ARCHIVED_ARTIFACTS_DERIVED_TREE_ROOT, execPath);
public static PathFragment getExecPathWithinArchivedArtifactsTree(PathFragment execPath) {
return embedDerivedTreeRoot(execPath, DEFAULT_DERIVED_TREE_ROOT);
}

/**
* Translates provided output {@code execPath} to one under provided derived tree root.
*
* <p>Example: {@code bazel-out/k8-fastbuild/bin ->
* bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin}.
* bazel-out/{derivedTreeRoot}/k8-fastbuild/bin}.
*/
private static PathFragment getExecPathWithinCustomDerivedRoot(
PathFragment derivedPathPrefix, PathFragment customDerivedTreeRoot, PathFragment execPath) {
return derivedPathPrefix
.getRelative(customDerivedTreeRoot)
.getRelative(execPath.relativeTo(derivedPathPrefix));
private static PathFragment embedDerivedTreeRoot(
PathFragment execPath, PathFragment derivedTreeRoot) {
return execPath
.subFragment(0, 1)
.getRelative(derivedTreeRoot)
.getRelative(execPath.subFragment(1));
}

private static Path getExecRoot(ArtifactRoot artifactRoot) {
Expand All @@ -1284,16 +1275,42 @@ private static Path getExecRoot(ArtifactRoot artifactRoot) {
0, rootPathFragment.segmentCount() - artifactRoot.getExecPath().segmentCount());
return rootPath.getFileSystem().getPath(execRootPath);
}
}

@VisibleForSerialization
@AutoCodec.Instantiator
static ArchivedTreeArtifact createForDeserialization(
SpecialArtifact treeArtifact, ArtifactRoot root, PathFragment execPath) {
@SuppressWarnings("unused") // Codec used by reflection.
private static final class ArchivedTreeArtifactCodec
implements ObjectCodec<ArchivedTreeArtifact> {

@Override
public Class<ArchivedTreeArtifact> getEncodedClass() {
return ArchivedTreeArtifact.class;
}

@Override
public void serialize(
SerializationContext context, ArchivedTreeArtifact obj, CodedOutputStream codedOut)
throws SerializationException, IOException {
PathFragment derivedTreeRoot = obj.getRoot().getExecPath().subFragment(1, 2);

context.serialize(obj.getParent(), codedOut);
context.serialize(derivedTreeRoot, codedOut);
context.serialize(obj.getRootRelativePath(), codedOut);
}

@Override
public ArchivedTreeArtifact deserialize(
DeserializationContext context, CodedInputStream codedIn)
throws SerializationException, IOException {
SpecialArtifact treeArtifact = context.deserialize(codedIn);
PathFragment derivedTreeRoot = context.deserialize(codedIn);
PathFragment rootRelativePath = context.deserialize(codedIn);
Object generatingActionKey =
treeArtifact.hasGeneratingActionKey()
? treeArtifact.getGeneratingActionKey()
: OMITTED_FOR_SERIALIZATION;
return new ArchivedTreeArtifact(treeArtifact, root, execPath, generatingActionKey);

return ArchivedTreeArtifact.createInternal(
treeArtifact, derivedTreeRoot, rootRelativePath, generatingActionKey);
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
View file
Expand Up @@ -113,7 +113,7 @@ private String getAdditionalWorkspaceStatus(
} catch (IOException e) {
throw createExecutionException(e, Code.STDERR_IO_EXCEPTION);
}
return new String(stdoutStream.toByteArray(), UTF_8);
return stdoutStream.toString(UTF_8);
}
} catch (BadExitStatusException e) {
throw createExecutionException(e, Code.NON_ZERO_EXIT);
Expand Down Expand Up @@ -159,7 +159,7 @@ public void prepare(
Path execRoot,
ArtifactPathResolver pathResolver,
@Nullable BulkDeleter bulkDeleter,
@Nullable PathFragment outputPrefixForArchivedArtifactsCleanup)
boolean cleanupArchivedArtifacts)
throws IOException {
// The default implementation of this method deletes all output files; override it to keep
// the old stableStatus around. This way we can reuse the existing file (preserving its mtime)
Expand Down Expand Up @@ -356,8 +356,8 @@ public com.google.devtools.build.lib.shell.Command getCommand() {
@Override
public Iterable<Class<? extends OptionsBase>> getCommandOptions(Command command) {
return "build".equals(command.name())
? ImmutableList.<Class<? extends OptionsBase>>of(WorkspaceStatusAction.Options.class)
: ImmutableList.<Class<? extends OptionsBase>>of();
? ImmutableList.of(WorkspaceStatusAction.Options.class)
: ImmutableList.of();
}

@Override
Expand Down

0 comments on commit f948989

Please sign in to comment.