From 0a0c96289068507a72cc6e315f27cf129aeef9b2 Mon Sep 17 00:00:00 2001 From: Jakob Buchgraber Date: Fri, 8 Mar 2019 03:40:09 -0800 Subject: [PATCH] Teach the FilesystemValueChecker about remotely stored outputs The FilesystemValueChecker is used by Bazel to detect modified outputs before running a command. This change teaches it about remote output files that don't exist in the output base, but are only stored physically on a remote storage system with SkyFrame only tracking file metadata. Note that this change only affects incremental builds, and there introduces the following new behavior for artifacts and tree artifacts: 1) If the previous build created a remote output and the output does not exist in the output base then no invalidation happens. 2) If the previous build created a remote output and the output does exist in the output base then the generating action is invalidated. A tree artifact may either be fully materialized in the output base or not materialized at all. Progress towards #6862 Closes #7269. PiperOrigin-RevId: 237422752 --- .../build/lib/actions/FileArtifactValue.java | 14 +- .../lib/skyframe/FilesystemValueChecker.java | 12 +- .../build/lib/skyframe/TreeArtifactValue.java | 45 +++++- .../skyframe/FilesystemValueCheckerTest.java | 149 ++++++++++++++++++ 4 files changed, 208 insertions(+), 12 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index 591227b8252c20..7beaae84c89398 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java @@ -51,6 +51,8 @@ *
  • a "middleman marker" object, which has a null digest, 0 size, and mtime of 0. *
  • The "self data" of a TreeArtifact, where we would expect to see a digest representing the * artifact's contents, and a size of 0. + *
  • a file that's only stored by a remote caching/execution system, in which case we would + * expect to see a digest and size. * */ @Immutable @@ -138,6 +140,11 @@ public final boolean isMarkerValue() { */ public abstract boolean wasModifiedSinceDigest(Path path) throws IOException; + /** Returns {@code true} if the file only exists remotely. */ + public boolean isRemote() { + return false; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -524,7 +531,12 @@ public int getLocationIndex() { @Override public boolean wasModifiedSinceDigest(Path path) { - throw new UnsupportedOperationException(); + return false; + } + + @Override + public boolean isRemote() { + return true; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java index 9efc2b67a78305..fad7df50aadda2 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java @@ -26,6 +26,7 @@ import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactFileMetadata; +import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.concurrent.ExecutorUtil; import com.google.devtools.build.lib.concurrent.Sharder; import com.google.devtools.build.lib.concurrent.ThrowableRecordingRunnableWrapper; @@ -80,7 +81,7 @@ public class FilesystemValueChecker { private static final Predicate ACTION_FILTER = SkyFunctionName.functionIs(SkyFunctions.ACTION_EXECUTION); - private final TimestampGranularityMonitor tsgm; + @Nullable private final TimestampGranularityMonitor tsgm; @Nullable private final Range lastExecutionTimeRange; private AtomicInteger modifiedOutputFilesCounter = new AtomicInteger(0); @@ -398,8 +399,8 @@ private boolean treeArtifactIsDirty(Artifact artifact, TreeArtifactValue value) try { Set currentDirectoryValue = TreeArtifactValue.explodeDirectory(artifact.getPath()); - Set valuePaths = value.getChildPaths(); - return !currentDirectoryValue.equals(valuePaths); + return !(currentDirectoryValue.isEmpty() && value.isRemote()) + && !currentDirectoryValue.equals(value.getChildPaths()); } catch (IOException e) { return true; } @@ -417,7 +418,10 @@ private boolean actionValueIsDirtyWithDirectSystemCalls(ActionExecutionValue act try { ArtifactFileMetadata fileMetadata = ActionMetadataHandler.fileMetadataFromArtifact(file, null, tsgm); - if (!fileMetadata.equals(lastKnownData)) { + FileArtifactValue fileValue = actionValue.getArtifactValue(file); + boolean lastSeenRemotely = (fileValue != null) && fileValue.isRemote(); + boolean trustRemoteValue = !fileMetadata.exists() && lastSeenRemotely; + if (!trustRemoteValue && !fileMetadata.equals(lastKnownData)) { updateIntraBuildModifiedCounter( fileMetadata.exists() ? file.getPath().getLastModifiedTime(Symlinks.FOLLOW) : -1, lastKnownData.isSymlink(), diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java index dd72ee2aabfb5a..b46ac77beea030 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java @@ -15,6 +15,7 @@ import com.google.common.base.Function; import com.google.common.base.MoreObjects; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -53,30 +54,55 @@ public PathFragment apply(Artifact artifact) { } }; + private static final TreeArtifactValue EMPTY = + new TreeArtifactValue( + DigestUtils.fromMetadata(ImmutableMap.of()).getDigestBytesUnsafe(), + ImmutableMap.of(), + /* remote= */ false); + private final byte[] digest; private final Map childData; private BigInteger valueFingerprint; + private final boolean remote; @AutoCodec.VisibleForSerialization - TreeArtifactValue(byte[] digest, Map childData) { + TreeArtifactValue( + byte[] digest, Map childData, boolean remote) { this.digest = digest; this.childData = ImmutableMap.copyOf(childData); + this.remote = remote; } /** - * Returns a TreeArtifactValue out of the given Artifact-relative path fragments - * and their corresponding FileArtifactValues. + * Returns a TreeArtifactValue out of the given Artifact-relative path fragments and their + * corresponding FileArtifactValues. + * + *

    All {@code childFileValues} must return the same value for {@link + * FileArtifactValue#isRemote()}. */ static TreeArtifactValue create(Map childFileValues) { + if (childFileValues.isEmpty()) { + return EMPTY; + } Map digestBuilder = Maps.newHashMapWithExpectedSize(childFileValues.size()); + Boolean remote = null; for (Map.Entry e : childFileValues.entrySet()) { - digestBuilder.put(e.getKey().getParentRelativePath().getPathString(), e.getValue()); + FileArtifactValue value = e.getValue(); + if (remote == null) { + remote = value.isRemote(); + } + Preconditions.checkArgument( + value.isRemote() == remote, + "files in a tree artifact must either be all remote or all local. '%v', remote=%b", + value, + value.isRemote()); + digestBuilder.put(e.getKey().getParentRelativePath().getPathString(), value); } - return new TreeArtifactValue( DigestUtils.fromMetadata(digestBuilder).getDigestBytesUnsafe(), - ImmutableMap.copyOf(childFileValues)); + ImmutableMap.copyOf(childFileValues), + Preconditions.checkNotNull(remote)); } FileArtifactValue getSelfData() { @@ -104,6 +130,11 @@ Map getChildValues() { return childData; } + /** Returns true if the {@link TreeFileArtifact}s are only stored remotely. */ + public boolean isRemote() { + return remote; + } + @Override public BigInteger getValueFingerprint() { if (valueFingerprint == null) { @@ -150,7 +181,7 @@ public String toString() { * Java's concurrent collections disallow null members. */ static final TreeArtifactValue MISSING_TREE_ARTIFACT = - new TreeArtifactValue(null, ImmutableMap.of()) { + new TreeArtifactValue(null, ImmutableMap.of(), false) { @Override FileArtifactValue getSelfData() { throw new UnsupportedOperationException(); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java index 62691af58e13ab..b80697b141b463 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java @@ -21,8 +21,10 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.hash.HashCode; import com.google.common.util.concurrent.Runnables; import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; @@ -32,6 +34,7 @@ import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.actions.util.TestAction; @@ -50,6 +53,7 @@ import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.BatchStat; +import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileStatusWithDigest; import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter; @@ -77,6 +81,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -787,6 +792,150 @@ private ActionExecutionValue actionValueWithTreeArtifacts(List /*actionDependsOnBuildId=*/ false); } + private ActionExecutionValue actionValueWithRemoteTreeArtifact( + SpecialArtifact output, Map children) { + ImmutableMap.Builder childFileValues = + ImmutableMap.builder(); + for (Map.Entry child : children.entrySet()) { + childFileValues.put( + ActionInputHelper.treeFileArtifact(output, child.getKey()), child.getValue()); + } + TreeArtifactValue treeArtifactValue = TreeArtifactValue.create(childFileValues.build()); + return ActionExecutionValue.create( + Collections.emptyMap(), + Collections.singletonMap(output, treeArtifactValue), + ImmutableMap.of(), + /* outputSymlinks= */ null, + /* discoveredModules= */ null, + /* actionDependsOnBuildId= */ false); + } + + private ActionExecutionValue actionValueWithRemoteArtifact( + Artifact output, RemoteFileArtifactValue value) { + return ActionExecutionValue.create( + Collections.singletonMap(output, ArtifactFileMetadata.PLACEHOLDER), + ImmutableMap.of(), + Collections.singletonMap(output, value), + /* outputSymlinks= */ null, + /* discoveredModules= */ null, + /* actionDependsOnBuildId= */ false); + } + + private RemoteFileArtifactValue createRemoteFileArtifactValue(String contents) { + byte[] data = contents.getBytes(); + DigestHashFunction hashFn = fs.getDigestFunction(); + HashCode hash = hashFn.getHashFunction().hashBytes(data); + return new RemoteFileArtifactValue(hash.asBytes(), data.length, -1); + } + + @Test + public void testRemoteAndLocalArtifacts() throws Exception { + // Test that injected remote artifacts are trusted by the FileSystemValueChecker + // and that local files always takes preference over remote files. + ActionLookupKey actionLookupKey = + new ActionLookupKey() { + @Override + public SkyFunctionName functionName() { + return SkyFunctionName.FOR_TESTING; + } + }; + SkyKey actionKey1 = ActionExecutionValue.key(actionLookupKey, 0); + SkyKey actionKey2 = ActionExecutionValue.key(actionLookupKey, 1); + + Artifact out1 = createDerivedArtifact("foo"); + Artifact out2 = createDerivedArtifact("bar"); + Map metadataToInject = new HashMap<>(); + metadataToInject.put( + actionKey1, + actionValueWithRemoteArtifact(out1, createRemoteFileArtifactValue("foo-content"))); + metadataToInject.put( + actionKey2, + actionValueWithRemoteArtifact(out2, createRemoteFileArtifactValue("bar-content"))); + differencer.inject(metadataToInject); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setNumThreads(1) + .setEventHander(NullEventHandler.INSTANCE) + .build(); + assertThat( + driver.evaluate(ImmutableList.of(actionKey1, actionKey2), evaluationContext).hasError()) + .isFalse(); + assertThat( + new FilesystemValueChecker(/* tsgm= */ null, /* lastExecutionTimeRange= */ null) + .getDirtyActionValues( + evaluator.getValues(), + /* batchStatter= */ null, + ModifiedFileSet.EVERYTHING_MODIFIED)) + .isEmpty(); + + // Create the "out1" artifact on the filesystem and test that it invalidates the generating + // action's SkyKey. + FileSystemUtils.writeContentAsLatin1(out1.getPath(), "new-foo-content"); + assertThat( + new FilesystemValueChecker(/* tsgm= */ null, /* lastExecutionTimeRange= */ null) + .getDirtyActionValues( + evaluator.getValues(), + /* batchStatter= */ null, + ModifiedFileSet.EVERYTHING_MODIFIED)) + .containsExactly(actionKey1); + } + + @Test + public void testRemoteAndLocalTreeArtifacts() throws Exception { + // Test that injected remote tree artifacts are trusted by the FileSystemValueChecker + // and that local files always takes preference over remote files. + ActionLookupKey actionLookupKey = + new ActionLookupKey() { + @Override + public SkyFunctionName functionName() { + return SkyFunctionName.FOR_TESTING; + } + }; + SkyKey actionKey = ActionExecutionValue.key(actionLookupKey, 0); + + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + treeArtifact.getPath().createDirectoryAndParents(); + Map treeArtifactMetadata = new HashMap<>(); + treeArtifactMetadata.put( + PathFragment.create("foo"), createRemoteFileArtifactValue("foo-content")); + treeArtifactMetadata.put( + PathFragment.create("bar"), createRemoteFileArtifactValue("bar-content")); + + Map metadataToInject = new HashMap<>(); + metadataToInject.put( + actionKey, actionValueWithRemoteTreeArtifact(treeArtifact, treeArtifactMetadata)); + differencer.inject(metadataToInject); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setNumThreads(1) + .setEventHander(NullEventHandler.INSTANCE) + .build(); + assertThat(driver.evaluate(ImmutableList.of(actionKey), evaluationContext).hasError()) + .isFalse(); + assertThat( + new FilesystemValueChecker(/* tsgm= */ null, /* lastExecutionTimeRange= */ null) + .getDirtyActionValues( + evaluator.getValues(), + /* batchStatter= */ null, + ModifiedFileSet.EVERYTHING_MODIFIED)) + .isEmpty(); + + // Create dir/foo on the local disk and test that it invalidates the associated sky key. + TreeFileArtifact fooArtifact = treeFileArtifact(treeArtifact, "foo"); + FileSystemUtils.writeContentAsLatin1(fooArtifact.getPath(), "new-foo-content"); + assertThat( + new FilesystemValueChecker(/* tsgm= */ null, /* lastExecutionTimeRange= */ null) + .getDirtyActionValues( + evaluator.getValues(), + /* batchStatter= */ null, + ModifiedFileSet.EVERYTHING_MODIFIED)) + .containsExactly(actionKey); + } + @Test public void testPropagatesRuntimeExceptions() throws Exception { Collection values =