From bd010944d69361d14181f2d5094776aae5589d20 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Fri, 3 Mar 2023 07:13:57 -0800 Subject: [PATCH] Store TTL in RemoteFileArtifactValue When building without the bytes, Bazel stores `RemoteFileArtifactValue` in skyframe (inmemory) and in local action cache which represents a file that is stored remotely. Bazel assumes that the remote file will never expire which is wrong. In practice, remote cache often evict files due to space constraint, and when it happens, the builds could fail. This PR introduces flag `--experimental_remote_cache_ttl` which tells Bazel at least how long the remote cache could store a file after returning a reference of it to Bazel. Bazel calculates the TTL of the file and store it in the `RemoteFileArtifactValue`. In an incremental build, Bazel will discard the `RemoteFileArtifactValue` and rerun the generating actions if it finds out that the `RemoteFileArtifactValue` is expired. The new field `expireAtEpochMilli` replaces `actionId` (deleted by f62a8b9f6a7e840c648f44f094390c3b85b236df), so there shouldn't be memory regression. There are two places Bazel checks the TTL: 1. If the skyframe has in-memory state about previous builds (e.g. incremental builds), the `SkyValue`s are marked as dirty if the `RemoteFileArtifactValue` is expired. 2. When checking local action cache, if the `RemoteFileArtifactValue` is expired, the cache entry is ignored. So that the generating actions can be re-executed. Part of #16660. Closes #17639. RELNOTES: Add flag `--experimental_remote_cache_ttl` and set the default value to 3 hours. PiperOrigin-RevId: 513819724 Change-Id: I9c9813621d04d5b1b94312be39384962feae2f7b --- .../build/lib/actions/ActionCacheChecker.java | 15 +- .../build/lib/actions/FileArtifactValue.java | 40 +++++- .../cache/CompactPersistentActionCache.java | 18 ++- .../lib/remote/RemoteActionFileSystem.java | 25 +++- .../lib/remote/RemoteExecutionService.java | 13 +- .../remote/options/CommonRemoteOptions.java | 39 +++++ .../lib/remote/options/RemoteOptions.java | 23 +-- .../lib/skyframe/FilesystemValueChecker.java | 83 ++++++----- .../lib/actions/ActionCacheCheckerTest.java | 124 +++++++++++++++- .../lib/actions/CompletionContextTest.java | 6 +- .../CompactPersistentActionCacheTest.java | 33 ++++- .../remote/ActionInputPrefetcherTestBase.java | 6 +- ...ildWithoutTheBytesIntegrationTestBase.java | 93 ++++++++++++ ...eStreamBuildEventArtifactUploaderTest.java | 3 +- .../remote/RemoteActionFileSystemTest.java | 9 +- .../remote/RemoteExecutionServiceTest.java | 25 ++-- .../lib/remote/options/RemoteOptionsTest.java | 5 +- .../skyframe/ActionExecutionValueTest.java | 10 +- .../skyframe/ActionMetadataHandlerTest.java | 23 ++- .../lib/skyframe/FileArtifactValueTest.java | 13 +- .../skyframe/FilesystemValueCheckerTest.java | 136 +++++++++++++++++- .../lib/skyframe/TreeArtifactBuildTest.java | 10 +- .../lib/skyframe/TreeArtifactValueTest.java | 3 +- 23 files changed, 635 insertions(+), 120 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java index a053d76695ff22..05d8bc8c5c0bd3 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java @@ -46,6 +46,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.io.FileNotFoundException; import java.io.IOException; +import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; @@ -317,6 +318,7 @@ private CachedOutputMetadata( private static CachedOutputMetadata loadCachedOutputMetadata( Action action, ActionCache.Entry entry, MetadataHandler metadataHandler) { + Instant now = Instant.now(); ImmutableMap.Builder remoteFileMetadata = ImmutableMap.builder(); ImmutableMap.Builder mergedTreeMetadata = @@ -330,6 +332,17 @@ private static CachedOutputMetadata loadCachedOutputMetadata( continue; } + // If any child is not alive, discard the entire tree + if (cachedTreeMetadata.childValues().values().stream() + .anyMatch(metadata -> !metadata.isAlive(now))) { + continue; + } + + if (cachedTreeMetadata.archivedFileValue().isPresent() + && !cachedTreeMetadata.archivedFileValue().get().isAlive(now)) { + continue; + } + TreeArtifactValue localTreeMetadata; try { localTreeMetadata = metadataHandler.getTreeArtifactValue(parent); @@ -377,7 +390,7 @@ private static CachedOutputMetadata loadCachedOutputMetadata( mergedTreeMetadata.put(parent, merged.build()); } else { RemoteFileArtifactValue cachedMetadata = entry.getOutputFile(artifact); - if (cachedMetadata == null) { + if (cachedMetadata == null || !cachedMetadata.isAlive(now)) { continue; } 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 b44f9e9a3b8045..b7ca668d84d401 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 @@ -34,6 +34,7 @@ import com.google.devtools.build.skyframe.SkyValue; import java.io.ByteArrayInputStream; import java.io.IOException; +import java.time.Instant; import java.util.Arrays; import java.util.Objects; import java.util.Optional; @@ -542,27 +543,34 @@ public static class RemoteFileArtifactValue extends FileArtifactValue { protected final byte[] digest; protected final long size; protected final int locationIndex; + // The time when the remote file expires in milliseconds since epoch. negative value means the + // remote file will never expire. This field doesn't contribute to the equality of the metadata. + protected final long expireAtEpochMilli; - private RemoteFileArtifactValue(byte[] digest, long size, int locationIndex) { + private RemoteFileArtifactValue( + byte[] digest, long size, int locationIndex, long expireAtEpochMilli) { this.digest = Preconditions.checkNotNull(digest); this.size = size; this.locationIndex = locationIndex; + this.expireAtEpochMilli = expireAtEpochMilli; } - public static RemoteFileArtifactValue create(byte[] digest, long size, int locationIndex) { - return new RemoteFileArtifactValue(digest, size, locationIndex); + public static RemoteFileArtifactValue create( + byte[] digest, long size, int locationIndex, long expireAtEpochMilli) { + return new RemoteFileArtifactValue(digest, size, locationIndex, expireAtEpochMilli); } public static RemoteFileArtifactValue create( byte[] digest, long size, int locationIndex, + long expireAtEpochMilli, @Nullable PathFragment materializationExecPath) { if (materializationExecPath != null) { return new RemoteFileArtifactValueWithMaterializationPath( - digest, size, locationIndex, materializationExecPath); + digest, size, locationIndex, expireAtEpochMilli, materializationExecPath); } - return new RemoteFileArtifactValue(digest, size, locationIndex); + return new RemoteFileArtifactValue(digest, size, locationIndex, expireAtEpochMilli); } @Override @@ -613,6 +621,18 @@ public int getLocationIndex() { return locationIndex; } + public long getExpireAtEpochMilli() { + return expireAtEpochMilli; + } + + public boolean isAlive(Instant now) { + if (expireAtEpochMilli < 0) { + return true; + } + + return now.toEpochMilli() < expireAtEpochMilli; + } + @Override public boolean wasModifiedSinceDigest(Path path) { return false; @@ -629,6 +649,7 @@ public String toString() { .add("digest", bytesToString(digest)) .add("size", size) .add("locationIndex", locationIndex) + .add("expireAtEpochMilli", expireAtEpochMilli) .toString(); } } @@ -644,8 +665,12 @@ public static final class RemoteFileArtifactValueWithMaterializationPath private final PathFragment materializationExecPath; private RemoteFileArtifactValueWithMaterializationPath( - byte[] digest, long size, int locationIndex, PathFragment materializationExecPath) { - super(digest, size, locationIndex); + byte[] digest, + long size, + int locationIndex, + long expireAtEpochMilli, + PathFragment materializationExecPath) { + super(digest, size, locationIndex, expireAtEpochMilli); this.materializationExecPath = Preconditions.checkNotNull(materializationExecPath); } @@ -679,6 +704,7 @@ public String toString() { .add("digest", bytesToString(digest)) .add("size", size) .add("locationIndex", locationIndex) + .add("expireAtEpochMilli", expireAtEpochMilli) .add("materializationExecPath", materializationExecPath) .toString(); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java index 3809a4f7db92ec..aac16acc51751e 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java @@ -76,7 +76,7 @@ public class CompactPersistentActionCache implements ActionCache { private static final int NO_INPUT_DISCOVERY_COUNT = -1; - private static final int VERSION = 15; + private static final int VERSION = 16; private static final class ActionMap extends PersistentMap { private final Clock clock; @@ -466,6 +466,8 @@ private static void encodeRemoteMetadata( VarInt.putVarInt(value.getLocationIndex(), sink); + VarInt.putVarLong(value.getExpireAtEpochMilli(), sink); + Optional materializationExecPath = value.getMaterializationExecPath(); if (materializationExecPath.isPresent()) { VarInt.putVarInt(1, sink); @@ -476,10 +478,11 @@ private static void encodeRemoteMetadata( } private static final int MAX_REMOTE_METADATA_SIZE = - DigestUtils.ESTIMATED_SIZE - + VarInt.MAX_VARLONG_SIZE - + VarInt.MAX_VARINT_SIZE - + VarInt.MAX_VARINT_SIZE; + DigestUtils.ESTIMATED_SIZE // digest + + VarInt.MAX_VARLONG_SIZE // size + + VarInt.MAX_VARINT_SIZE // locationIndex + + VarInt.MAX_VARINT_SIZE // expireAtEpochMilli + + VarInt.MAX_VARINT_SIZE; // materializationExecPath private static RemoteFileArtifactValue decodeRemoteMetadata( StringIndexer indexer, ByteBuffer source) throws IOException { @@ -489,6 +492,8 @@ private static RemoteFileArtifactValue decodeRemoteMetadata( int locationIndex = VarInt.getVarInt(source); + long expireAtEpochMilli = VarInt.getVarLong(source); + PathFragment materializationExecPath = null; int numMaterializationExecPath = VarInt.getVarInt(source); if (numMaterializationExecPath > 0) { @@ -499,7 +504,8 @@ private static RemoteFileArtifactValue decodeRemoteMetadata( PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source))); } - return RemoteFileArtifactValue.create(digest, size, locationIndex, materializationExecPath); + return RemoteFileArtifactValue.create( + digest, size, locationIndex, expireAtEpochMilli, materializationExecPath); } /** @return action data encoded as a byte[] array. */ diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java index dfd21d7d79e602..28ee9babf324f6 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java @@ -115,11 +115,12 @@ public void updateContext(MetadataInjector metadataInjector) { this.metadataInjector = metadataInjector; } - void injectRemoteFile(PathFragment path, byte[] digest, long size) throws IOException { + void injectRemoteFile(PathFragment path, byte[] digest, long size, long expireAtEpochMilli) + throws IOException { if (!isOutput(path)) { return; } - remoteOutputTree.injectRemoteFile(path, digest, size); + remoteOutputTree.injectRemoteFile(path, digest, size, expireAtEpochMilli); } void flush() throws IOException { @@ -206,6 +207,7 @@ private void maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact outpu metadata.getDigest(), metadata.getSize(), metadata.getLocationIndex(), + metadata.getExpireAtEpochMilli(), // Avoid a double indirection when the target is already materialized as a symlink. metadata.getMaterializationExecPath().orElse(targetPath.relativeTo(execRoot))); @@ -215,7 +217,10 @@ private void maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact outpu private RemoteFileArtifactValue createRemoteMetadata(RemoteFileInfo remoteFile) { return RemoteFileArtifactValue.create( - remoteFile.getFastDigest(), remoteFile.getSize(), /* locationIndex= */ 1); + remoteFile.getFastDigest(), + remoteFile.getSize(), + /* locationIndex= */ 1, + remoteFile.getExpireAtEpochMilli()); } @Override @@ -743,7 +748,8 @@ protected FileInfo newFile(Clock clock, PathFragment path) { return new RemoteFileInfo(clock); } - void injectRemoteFile(PathFragment path, byte[] digest, long size) throws IOException { + void injectRemoteFile(PathFragment path, byte[] digest, long size, long expireAtEpochMilli) + throws IOException { createDirectoryAndParents(path.getParentDirectory()); InMemoryContentInfo node = getOrCreateWritableInode(path); // If a node was already existed and is not a remote file node (i.e. directory or symlink node @@ -753,7 +759,7 @@ void injectRemoteFile(PathFragment path, byte[] digest, long size) throws IOExce } RemoteFileInfo remoteFileInfo = (RemoteFileInfo) node; - remoteFileInfo.set(digest, size); + remoteFileInfo.set(digest, size, expireAtEpochMilli); } @Nullable @@ -771,13 +777,16 @@ static class RemoteFileInfo extends FileInfo { private byte[] digest; private long size; + private long expireAtEpochMilli; + RemoteFileInfo(Clock clock) { super(clock); } - private void set(byte[] digest, long size) { + private void set(byte[] digest, long size, long expireAtEpochMilli) { this.digest = digest; this.size = size; + this.expireAtEpochMilli = expireAtEpochMilli; } @Override @@ -804,5 +813,9 @@ public byte[] getFastDigest() { public long getSize() { return size; } + + public long getExpireAtEpochMilli() { + return expireAtEpochMilli; + } } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index d97c62cd4f7ab1..b5f598a73d2bf0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -128,6 +128,7 @@ import io.reactivex.rxjava3.disposables.Disposable; import io.reactivex.rxjava3.schedulers.Schedulers; import java.io.IOException; +import java.time.Instant; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -806,7 +807,8 @@ private void createSymlinks(Iterable symlinks) throws IOExcepti } } - private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata metadata) + private void injectRemoteArtifacts( + RemoteAction action, ActionResultMetadata metadata, long expireAtEpochMilli) throws IOException { FileSystem actionFileSystem = action.getSpawnExecutionContext().getActionFileSystem(); checkState(actionFileSystem instanceof RemoteActionFileSystem); @@ -825,7 +827,8 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met remoteActionFileSystem.injectRemoteFile( file.path().asFragment(), DigestUtil.toBinaryDigest(file.digest()), - file.digest().getSizeBytes()); + file.digest().getSizeBytes(), + expireAtEpochMilli); } } @@ -833,7 +836,8 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met remoteActionFileSystem.injectRemoteFile( file.path().asFragment(), DigestUtil.toBinaryDigest(file.digest()), - file.digest().getSizeBytes()); + file.digest().getSizeBytes(), + expireAtEpochMilli); } } @@ -1162,7 +1166,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re } } - injectRemoteArtifacts(action, metadata); + var expireAtEpochMilli = Instant.now().plus(remoteOptions.remoteCacheTtl).toEpochMilli(); + injectRemoteArtifacts(action, metadata, expireAtEpochMilli); try (SilentCloseable c = Profiler.instance().profile("Remote.downloadInMemoryOutput")) { if (inMemoryOutput != null) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java index 27da47a8fb2da0..f016f07a24afc4 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java @@ -13,11 +13,16 @@ // limitations under the License. package com.google.devtools.build.lib.remote.options; +import com.google.devtools.common.options.Converter; +import com.google.devtools.common.options.Converters; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; import com.google.devtools.common.options.OptionsBase; +import com.google.devtools.common.options.OptionsParsingException; +import java.time.Duration; import java.util.List; +import java.util.regex.Pattern; /** Options for remote execution and distributed caching that shared between Bazel and Blaze. */ public class CommonRemoteOptions extends OptionsBase { @@ -33,4 +38,38 @@ public class CommonRemoteOptions extends OptionsBase { + " the client to request certain artifacts that might be needed locally (e.g. IDE" + " support)") public List remoteDownloadRegex; + + @Option( + name = "experimental_remote_cache_ttl", + defaultValue = "3h", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.EXECUTION}, + converter = RemoteDurationConverter.class, + help = + "The guaranteed minimal TTL of blobs in the remote cache after their digests are recently" + + " referenced e.g. by an ActionResult or FindMissingBlobs. Bazel does several" + + " optimizations based on the blobs' TTL e.g. doesn't repeatedly call" + + " GetActionResult in an incremental build. The value should be set slightly less" + + " than the real TTL since there is a gap between when the server returns the" + + " digests and when Bazel receives them.") + public Duration remoteCacheTtl; + + /** Returns the specified duration. Assumes seconds if unitless. */ + public static class RemoteDurationConverter extends Converter.Contextless { + + private static final Pattern UNITLESS_REGEX = Pattern.compile("^[0-9]+$"); + + @Override + public Duration convert(String input) throws OptionsParsingException { + if (UNITLESS_REGEX.matcher(input).matches()) { + input += "s"; + } + return new Converters.DurationConverter().convert(input, /* conversionContext= */ null); + } + + @Override + public String getTypeDescription() { + return "An immutable length of time."; + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 879ba1dfdcfce9..c7f2e7086041bb 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -24,7 +24,6 @@ import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution.Code; import com.google.devtools.build.lib.util.OptionsUtils; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.common.options.Converter; import com.google.devtools.common.options.Converters; import com.google.devtools.common.options.Converters.AssignmentConverter; import com.google.devtools.common.options.EnumConverter; @@ -32,7 +31,6 @@ import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; import com.google.devtools.common.options.OptionMetadataTag; -import com.google.devtools.common.options.OptionsParsingException; import com.google.protobuf.TextFormat; import com.google.protobuf.TextFormat.ParseException; import java.time.Duration; @@ -40,7 +38,6 @@ import java.util.Map; import java.util.Map.Entry; import java.util.SortedMap; -import java.util.regex.Pattern; /** Options for remote execution and distributed caching for Bazel only. */ public final class RemoteOptions extends CommonRemoteOptions { @@ -203,7 +200,7 @@ public final class RemoteOptions extends CommonRemoteOptions { defaultValue = "60s", documentationCategory = OptionDocumentationCategory.REMOTE, effectTags = {OptionEffectTag.UNKNOWN}, - converter = RemoteTimeoutConverter.class, + converter = RemoteDurationConverter.class, help = "The maximum amount of time to wait for remote execution and cache calls. For the REST" + " cache, this is both the connect and the read timeout. Following units can be" @@ -224,24 +221,6 @@ public final class RemoteOptions extends CommonRemoteOptions { + "When not set, it will default to \"${hostname}/${instance_name}\".") public String remoteBytestreamUriPrefix; - /** Returns the specified duration. Assumes seconds if unitless. */ - public static class RemoteTimeoutConverter extends Converter.Contextless { - private static final Pattern UNITLESS_REGEX = Pattern.compile("^[0-9]+$"); - - @Override - public Duration convert(String input) throws OptionsParsingException { - if (UNITLESS_REGEX.matcher(input).matches()) { - input += "s"; - } - return new Converters.DurationConverter().convert(input, /*conversionContext=*/ null); - } - - @Override - public String getTypeDescription() { - return "An immutable length of time."; - } - } - @Option( name = "remote_accept_cached", defaultValue = "true", 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 50ab2f99b171f8..47ce7d8610eb39 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 @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.FileStateType; import com.google.devtools.build.lib.concurrent.ExecutorUtil; import com.google.devtools.build.lib.concurrent.Sharder; @@ -58,6 +59,7 @@ import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.WalkableGraph; import java.io.IOException; +import java.time.Instant; import java.util.Collection; import java.util.Collections; import java.util.HashMap; @@ -228,6 +230,8 @@ public NavigableSet get() { } }); + var now = Instant.now(); + boolean interrupted; try (SilentCloseable c = Profiler.instance().profile("getDirtyActionValues.stat_files")) { for (List> shard : outputShards) { @@ -239,7 +243,8 @@ public NavigableSet get() { knownModifiedOutputFiles, sortedKnownModifiedOutputFiles, trustRemoteArtifacts, - modifiedOutputsReceiver) + modifiedOutputsReceiver, + now) : batchStatJob( dirtyKeys, shard, @@ -247,7 +252,8 @@ public NavigableSet get() { knownModifiedOutputFiles, sortedKnownModifiedOutputFiles, trustRemoteArtifacts, - modifiedOutputsReceiver); + modifiedOutputsReceiver, + now); executor.execute(job); } @@ -273,7 +279,8 @@ private Runnable batchStatJob( ImmutableSet knownModifiedOutputFiles, Supplier> sortedKnownModifiedOutputFiles, boolean trustRemoteArtifacts, - ModifiedOutputsReceiver modifiedOutputsReceiver) { + ModifiedOutputsReceiver modifiedOutputsReceiver, + Instant now) { return () -> { Map> fileToKeyAndValue = new HashMap<>(); Map> treeArtifactsToKeyAndValue = @@ -319,8 +326,8 @@ && shouldCheckFile(knownModifiedOutputFiles, artifact)) { try { stats = batchStatter.batchStat( - /*includeDigest=*/ true, - /*includeLinks=*/ true, + /* includeDigest= */ true, + /* includeLinks= */ true, Artifact.asPathFragments(artifacts)); } catch (IOException e) { logger.atWarning().withCause(e).log( @@ -331,7 +338,8 @@ && shouldCheckFile(knownModifiedOutputFiles, artifact)) { knownModifiedOutputFiles, sortedKnownModifiedOutputFiles, trustRemoteArtifacts, - modifiedOutputsReceiver) + modifiedOutputsReceiver, + now) .run(); return; } catch (InterruptedException e) { @@ -393,7 +401,8 @@ private Runnable outputStatJob( ImmutableSet knownModifiedOutputFiles, Supplier> sortedKnownModifiedOutputFiles, boolean trustRemoteArtifacts, - ModifiedOutputsReceiver modifiedOutputsReceiver) { + ModifiedOutputsReceiver modifiedOutputsReceiver, + Instant now) { return new Runnable() { @Override public void run() { @@ -405,7 +414,8 @@ public void run() { knownModifiedOutputFiles, sortedKnownModifiedOutputFiles, trustRemoteArtifacts, - modifiedOutputsReceiver)) { + modifiedOutputsReceiver, + now)) { dirtyKeys.add(keyAndValue.getFirst()); } } @@ -441,7 +451,8 @@ private boolean artifactIsDirtyWithDirectSystemCalls( ImmutableSet knownModifiedOutputFiles, boolean trustRemoteArtifacts, Map.Entry entry, - ModifiedOutputsReceiver modifiedOutputsReceiver) { + ModifiedOutputsReceiver modifiedOutputsReceiver, + Instant now) { Artifact file = entry.getKey(); FileArtifactValue lastKnownData = entry.getValue(); if (file.isMiddlemanArtifact() || !shouldCheckFile(knownModifiedOutputFiles, file)) { @@ -453,7 +464,8 @@ private boolean artifactIsDirtyWithDirectSystemCalls( boolean trustRemoteValue = fileMetadata.getType() == FileStateType.NONEXISTENT && lastKnownData.isRemote() - && trustRemoteArtifacts; + && trustRemoteArtifacts + && ((RemoteFileArtifactValue) lastKnownData).isAlive(now); if (!trustRemoteValue && fileMetadata.couldBeModifiedSince(lastKnownData)) { modifiedOutputsReceiver.reportModifiedOutputFile( fileMetadata.getType() != FileStateType.NONEXISTENT @@ -475,11 +487,12 @@ private boolean actionValueIsDirtyWithDirectSystemCalls( ImmutableSet knownModifiedOutputFiles, Supplier> sortedKnownModifiedOutputFiles, boolean trustRemoteArtifacts, - ModifiedOutputsReceiver modifiedOutputsReceiver) { + ModifiedOutputsReceiver modifiedOutputsReceiver, + Instant now) { boolean isDirty = false; for (Map.Entry entry : actionValue.getAllFileValues().entrySet()) { if (artifactIsDirtyWithDirectSystemCalls( - knownModifiedOutputFiles, trustRemoteArtifacts, entry, modifiedOutputsReceiver)) { + knownModifiedOutputFiles, trustRemoteArtifacts, entry, modifiedOutputsReceiver, now)) { isDirty = true; } } @@ -488,31 +501,31 @@ private boolean actionValueIsDirtyWithDirectSystemCalls( actionValue.getAllTreeArtifactValues().entrySet()) { TreeArtifactValue tree = entry.getValue(); - if (!tree.isEntirelyRemote()) { - for (Map.Entry childEntry : - tree.getChildValues().entrySet()) { - if (artifactIsDirtyWithDirectSystemCalls( - knownModifiedOutputFiles, - trustRemoteArtifacts, - childEntry, - modifiedOutputsReceiver)) { - isDirty = true; - } + for (Map.Entry childEntry : + tree.getChildValues().entrySet()) { + if (artifactIsDirtyWithDirectSystemCalls( + knownModifiedOutputFiles, + trustRemoteArtifacts, + childEntry, + modifiedOutputsReceiver, + now)) { + isDirty = true; } - isDirty = - isDirty - || tree.getArchivedRepresentation() - .map( - archivedRepresentation -> - artifactIsDirtyWithDirectSystemCalls( - knownModifiedOutputFiles, - trustRemoteArtifacts, - Maps.immutableEntry( - archivedRepresentation.archivedTreeFileArtifact(), - archivedRepresentation.archivedFileValue()), - modifiedOutputsReceiver)) - .orElse(false); } + isDirty = + isDirty + || tree.getArchivedRepresentation() + .map( + archivedRepresentation -> + artifactIsDirtyWithDirectSystemCalls( + knownModifiedOutputFiles, + trustRemoteArtifacts, + Maps.immutableEntry( + archivedRepresentation.archivedTreeFileArtifact(), + archivedRepresentation.archivedFileValue()), + modifiedOutputsReceiver, + now)) + .orElse(false); Artifact treeArtifact = entry.getKey(); if (shouldCheckTreeArtifact(sortedKnownModifiedOutputFiles.get(), treeArtifact) diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java index 7797cf67ccdd40..50f7df78317148 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java @@ -463,7 +463,15 @@ private RemoteFileArtifactValue createRemoteFileMetadata(String content) { private RemoteFileArtifactValue createRemoteFileMetadata( String content, @Nullable PathFragment materializationExecPath) { byte[] bytes = content.getBytes(UTF_8); - return RemoteFileArtifactValue.create(digest(bytes), bytes.length, 1, materializationExecPath); + return RemoteFileArtifactValue.create( + digest(bytes), bytes.length, 1, /* expireAtEpochMilli= */ -1, materializationExecPath); + } + + private RemoteFileArtifactValue createRemoteFileMetadata( + String content, long expireAtEpochMilli, @Nullable PathFragment materializationExecPath) { + byte[] bytes = content.getBytes(UTF_8); + return RemoteFileArtifactValue.create( + digest(bytes), bytes.length, 1, expireAtEpochMilli, materializationExecPath); } private static TreeArtifactValue createTreeMetadata( @@ -588,6 +596,37 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception { assertThat(metadataHandler.getMetadata(output)).isEqualTo(createRemoteFileMetadata(content)); } + @Test + public void saveOutputMetadata_remoteFileExpired_remoteFileMetadataNotLoaded() throws Exception { + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); + Artifact output = createArtifact(artifactRoot, "bin/dummy"); + String content = "content"; + Action action = + new InjectOutputFileMetadataAction( + output, + createRemoteFileMetadata( + content, /* expireAtEpochMilli= */ 0, /* materializationExecPath= */ null)); + MetadataHandler metadataHandler = new FakeMetadataHandler(); + + runAction(action); + Token token = + cacheChecker.getTokenIfNeedToExecute( + action, + /* resolvedCacheArtifacts= */ null, + /* clientEnv= */ ImmutableMap.of(), + OutputPermissions.READONLY, + /* handler= */ null, + metadataHandler, + /* artifactExpander= */ null, + /* remoteDefaultPlatformProperties= */ ImmutableMap.of(), + /* loadCachedOutputMetadata= */ true); + + assertThat(output.getPath().exists()).isFalse(); + assertThat(token).isNotNull(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNull(); + } + @Test public void saveOutputMetadata_remoteOutputUnavailable_remoteFileMetadataNotLoaded() throws Exception { @@ -999,6 +1038,89 @@ public void saveOutputMetadata_treeMetadata_localArchivedArtifactLoaded() throws assertThat(metadataHandler.getTreeArtifactValue(output)).isEqualTo(expectedMetadata); } + @Test + public void saveOutputMetadata_treeFileExpired_treeMetadataNotLoaded() throws Exception { + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + ImmutableMap children = + ImmutableMap.of( + "file1", createRemoteFileMetadata("content1"), + "file2", + createRemoteFileMetadata( + "content2", /* expireAtEpochMilli= */ 0, /* materializationExecPath= */ null)); + Action action = + new InjectOutputTreeMetadataAction( + output, + createTreeMetadata( + output, + children, + /* archivedArtifactValue= */ Optional.empty(), + /* materializationExecPath= */ Optional.empty())); + MetadataHandler metadataHandler = new FakeMetadataHandler(); + + runAction(action); + Token token = + cacheChecker.getTokenIfNeedToExecute( + action, + /* resolvedCacheArtifacts= */ null, + /* clientEnv= */ ImmutableMap.of(), + OutputPermissions.READONLY, + /* handler= */ null, + metadataHandler, + /* artifactExpander= */ null, + /* remoteDefaultPlatformProperties= */ ImmutableMap.of(), + /* loadCachedOutputMetadata= */ true); + + assertThat(output.getPath().exists()).isFalse(); + assertThat(token).isNotNull(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNull(); + } + + @Test + public void saveOutputMetadata_archivedRepresentationExpired_treeMetadataNotLoaded() + throws Exception { + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + ImmutableMap children = + ImmutableMap.of( + "file1", createRemoteFileMetadata("content1"), + "file2", createRemoteFileMetadata("content2")); + Action action = + new InjectOutputTreeMetadataAction( + output, + createTreeMetadata( + output, + children, + /* archivedArtifactValue= */ Optional.of( + createRemoteFileMetadata( + "archived", + /* expireAtEpochMilli= */ 0, + /* materializationExecPath= */ null)), + /* materializationExecPath= */ Optional.empty())); + MetadataHandler metadataHandler = new FakeMetadataHandler(); + + runAction(action); + Token token = + cacheChecker.getTokenIfNeedToExecute( + action, + /* resolvedCacheArtifacts= */ null, + /* clientEnv= */ ImmutableMap.of(), + OutputPermissions.READONLY, + /* handler= */ null, + metadataHandler, + /* artifactExpander= */ null, + /* remoteDefaultPlatformProperties= */ ImmutableMap.of(), + /* loadCachedOutputMetadata= */ true); + + assertThat(output.getPath().exists()).isFalse(); + assertThat(token).isNotNull(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNull(); + } + private static void writeContentAsLatin1(Path path, String content) throws IOException { Path parent = path.getParentDirectory(); if (parent != null) { diff --git a/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java b/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java index 764adbf976a4b2..e02257551e95c5 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java @@ -40,7 +40,11 @@ @RunWith(JUnit4.class) public class CompletionContextTest { private static final FileArtifactValue DUMMY_METADATA = - RemoteFileArtifactValue.create(/*digest=*/ new byte[0], /*size=*/ 0, /*locationIndex=*/ 0); + RemoteFileArtifactValue.create( + /* digest= */ new byte[0], + /* size= */ 0, + /* locationIndex= */ 0, + /* expireAtEpochMilli= */ -1); private Path execRoot; private ArtifactRoot outputRoot; diff --git a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java index c9f6d59a241452..905e25e2b74326 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java @@ -35,6 +35,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.time.Instant; import java.util.Map; import java.util.Optional; import javax.annotation.Nullable; @@ -198,7 +199,10 @@ private FileArtifactValue createLocalMetadata(Artifact artifact, String content) } private RemoteFileArtifactValue createRemoteMetadata( - Artifact artifact, String content, @Nullable PathFragment materializationExecPath) { + Artifact artifact, + String content, + long expireAtEpochMilli, + @Nullable PathFragment materializationExecPath) { byte[] bytes = content.getBytes(StandardCharsets.UTF_8); byte[] digest = artifact @@ -208,7 +212,14 @@ private RemoteFileArtifactValue createRemoteMetadata( .getHashFunction() .hashBytes(bytes) .asBytes(); - return RemoteFileArtifactValue.create(digest, bytes.length, 1, materializationExecPath); + return RemoteFileArtifactValue.create( + digest, bytes.length, 1, expireAtEpochMilli, materializationExecPath); + } + + private RemoteFileArtifactValue createRemoteMetadata( + Artifact artifact, String content, @Nullable PathFragment materializationExecPath) { + return createRemoteMetadata( + artifact, content, /* expireAtEpochMilli= */ -1, materializationExecPath); } private RemoteFileArtifactValue createRemoteMetadata(Artifact artifact, String content) { @@ -252,6 +263,24 @@ public void putAndGet_savesRemoteFileMetadata() { assertThat(entry.getOutputFile(artifact)).isEqualTo(metadata); } + @Test + public void putAndGet_savesRemoteFileMetadata_withExpireAtEpochMilli() { + String key = "key"; + ActionCache.Entry entry = + new ActionCache.Entry(key, ImmutableMap.of(), false, OutputPermissions.READONLY); + Artifact artifact = ActionsTestUtil.DUMMY_ARTIFACT; + long expireAtEpochMilli = Instant.now().toEpochMilli(); + RemoteFileArtifactValue metadata = + createRemoteMetadata( + artifact, "content", expireAtEpochMilli, /* materializationExecPath= */ null); + entry.addOutputFile(artifact, metadata, /* saveFileMetadata= */ true); + + cache.put(key, entry); + entry = cache.get(key); + + assertThat(entry.getOutputFile(artifact).getExpireAtEpochMilli()).isEqualTo(expireAtEpochMilli); + } + @Test public void putAndGet_savesRemoteFileMetadata_withmaterializationExecPath() { String key = "key"; diff --git a/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java index 8c097089a78200..c4c99508fb7b06 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java @@ -104,6 +104,7 @@ protected Artifact createRemoteArtifact( hashCode.asBytes(), contentsBytes.length, /* locationIndex= */ 1, + /* expireAtEpochMilli= */ -1, materializationExecPath); metadata.put(a, f); if (cas != null) { @@ -154,7 +155,10 @@ protected Pair> createRemoteTre HashCode hashCode = HASH_FUNCTION.getHashFunction().hashBytes(contents); RemoteFileArtifactValue childValue = RemoteFileArtifactValue.create( - hashCode.asBytes(), contents.length, /* locationIndex= */ 1); + hashCode.asBytes(), + contents.length, + /* locationIndex= */ 1, + /* expireAtEpochMilli= */ -1); treeBuilder.putChild(child, childValue); metadata.put(child, childValue); cas.put(hashCode, contents); diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java index a5e338bdc73903..fa089310cf91a4 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java @@ -1006,6 +1006,99 @@ public void remoteCacheEvictBlobs_whenPrefetchingInputTree_incrementalBuildCanCo "a/bar.out", "file-inside\nupdated bar" + lineSeparator(), /* isLocal= */ true); } + @Test + public void remoteFilesExpiredBetweenBuilds_rerunGeneratingActions() throws Exception { + // Arrange: Prepare workspace and populate remote cache + write( + "a/BUILD", + "genrule(", + " name = 'foo',", + " srcs = ['foo.in'],", + " outs = ['foo.out'],", + " cmd = 'cat $(SRCS) > $@',", + ")", + "genrule(", + " name = 'bar',", + " srcs = ['foo.out', 'bar.in'],", + " outs = ['bar.out'],", + " cmd = 'cat $(SRCS) > $@',", + ")"); + write("a/foo.in", "foo"); + write("a/bar.in", "bar"); + + // Populate remote cache + buildTarget("//a:bar"); + getOutputPath("a/foo.out").delete(); + getOutputPath("a/bar.out").delete(); + getOutputBase().getRelative("action_cache").deleteTreesBelow(); + restartServer(); + + // Clean build, foo.out isn't downloaded + setDownloadToplevel(); + addOptions("--experimental_remote_cache_ttl=0s"); + buildTarget("//a:bar"); + assertOutputDoesNotExist("a/foo.out"); + + // Evict blobs from remote cache + evictAllBlobs(); + + // Act: Do an incremental build + write("a/bar.in", "updated bar"); + addOptions("--strategy_regexp=.*bar=local"); + buildTarget("//a:bar"); + waitDownloads(); + + // Assert: target was successfully built + assertValidOutputFile("a/bar.out", "foo" + lineSeparator() + "updated bar" + lineSeparator()); + } + + @Test + public void remoteTreeFilesExpiredBetweenBuilds_rerunGeneratingActions() throws Exception { + // Arrange: Prepare workspace and populate remote cache + write("BUILD"); + writeOutputDirRule(); + write( + "a/BUILD", + "load('//:output_dir.bzl', 'output_dir')", + "output_dir(", + " name = 'foo.out',", + " content_map = {'file-inside': 'hello world'},", + ")", + "genrule(", + " name = 'bar',", + " srcs = ['foo.out', 'bar.in'],", + " outs = ['bar.out'],", + " cmd = '( ls $(location :foo.out); cat $(location :bar.in) ) > $@',", + ")"); + write("a/bar.in", "bar"); + + // Populate remote cache + buildTarget("//a:bar"); + getOutputPath("a/foo.out").deleteTreesBelow(); + getOutputPath("a/bar.out").delete(); + getOutputBase().getRelative("action_cache").deleteTreesBelow(); + restartServer(); + + // Clean build, foo.out isn't downloaded + setDownloadToplevel(); + addOptions("--experimental_remote_cache_ttl=0s"); + buildTarget("//a:bar"); + assertOutputDoesNotExist("a/foo.out/file-inside"); + + // Evict blobs from remote cache + evictAllBlobs(); + + // Act: Do an incremental build + write("a/bar.in", "updated bar"); + addOptions("--strategy_regexp=.*bar=local"); + buildTarget("//a:bar"); + waitDownloads(); + + // Assert: target was successfully built + assertValidOutputFile( + "a/bar.out", "file-inside\nupdated bar" + lineSeparator(), /* isLocal= */ true); + } + protected void assertOutputsDoNotExist(String target) throws Exception { for (Artifact output : getArtifacts(target)) { assertWithMessage( diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java index bd9c625110864a..be4d3487a7977b 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java @@ -442,7 +442,8 @@ private Artifact createRemoteArtifact( byte[] b = contents.getBytes(StandardCharsets.UTF_8); HashCode h = HashCode.fromString(DIGEST_UTIL.compute(b).getHash()); FileArtifactValue f = - RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 1); + RemoteFileArtifactValue.create( + h.asBytes(), b.length, /* locationIndex= */ 1, /* expireAtEpochMilli= */ -1); inputs.putWithNoDepOwner(a, f); return a; } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java index 9b33e0778a2a3d..9e1eb36126e601 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java @@ -326,7 +326,8 @@ protected void injectRemoteFile(FileSystem actionFs, PathFragment path, String c byte[] contentBytes = content.getBytes(StandardCharsets.UTF_8); HashCode hashCode = HASH_FUNCTION.getHashFunction().hashBytes(contentBytes); ((RemoteActionFileSystem) actionFs) - .injectRemoteFile(path, hashCode.asBytes(), contentBytes.length); + .injectRemoteFile( + path, hashCode.asBytes(), contentBytes.length, /* expireAtEpochMilli= */ -1); } @Override @@ -342,7 +343,8 @@ private Artifact createRemoteArtifact( byte[] b = contents.getBytes(StandardCharsets.UTF_8); HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b); RemoteFileArtifactValue f = - RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 1); + RemoteFileArtifactValue.create( + h.asBytes(), b.length, /* locationIndex= */ 1, /* expireAtEpochMilli= */ -1); inputs.putWithNoDepOwner(a, f); return a; } @@ -364,7 +366,8 @@ private TreeArtifactValue createRemoteTreeArtifactValue( byte[] b = entry.getValue().getBytes(StandardCharsets.UTF_8); HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b); RemoteFileArtifactValue childMeta = - RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 0); + RemoteFileArtifactValue.create( + h.asBytes(), b.length, /* locationIndex= */ 0, /* expireAtEpochMilli= */ -1); builder.putChild(child, childMeta); } return builder.build(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 72cffa1aa77d85..2eeee1a3dcc57d 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -23,6 +23,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertThrows; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; @@ -965,7 +966,8 @@ public void downloadOutputs_outputFilesWithoutTopLevel_inject() throws Exception .injectRemoteFile( eq(execRoot.asFragment().getRelative(a1.getExecPath())), eq(toBinaryDigest(d1)), - eq(d1.getSizeBytes())); + eq(d1.getSizeBytes()), + anyLong()); Path outputBase = checkNotNull(artifactRoot.getRoot().asPath()); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); assertThat(context.isLockOutputFilesCalled()).isTrue(); @@ -1005,12 +1007,14 @@ public void downloadOutputs_outputFilesWithMinimal_injectingMetadata() throws Ex .injectRemoteFile( eq(execRoot.asFragment().getRelative(a1.getExecPath())), eq(toBinaryDigest(d1)), - eq(d1.getSizeBytes())); + eq(d1.getSizeBytes()), + anyLong()); verify(actionFileSystem) .injectRemoteFile( eq(execRoot.asFragment().getRelative(a2.getExecPath())), eq(toBinaryDigest(d2)), - eq(d2.getSizeBytes())); + eq(d2.getSizeBytes()), + anyLong()); Path outputBase = checkNotNull(artifactRoot.getRoot().asPath()); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); assertThat(context.isLockOutputFilesCalled()).isTrue(); @@ -1063,12 +1067,14 @@ public void downloadOutputs_outputDirectoriesWithMinimal_injectingMetadata() thr .injectRemoteFile( eq(execRoot.asFragment().getRelative("outputs/dir/file1")), eq(toBinaryDigest(d1)), - eq(d1.getSizeBytes())); + eq(d1.getSizeBytes()), + anyLong()); verify(actionFileSystem) .injectRemoteFile( eq(execRoot.asFragment().getRelative("outputs/dir/a/file2")), eq(toBinaryDigest(d2)), - eq(d2.getSizeBytes())); + eq(d2.getSizeBytes()), + anyLong()); Path outputBase = checkNotNull(artifactRoot.getRoot().asPath()); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); assertThat(context.isLockOutputFilesCalled()).isTrue(); @@ -1201,12 +1207,14 @@ public void downloadOutputs_inMemoryOutputWithMinimal_downloadIt() throws Except .injectRemoteFile( eq(execRoot.asFragment().getRelative(a1.getExecPath())), eq(toBinaryDigest(d1)), - eq(d1.getSizeBytes())); + eq(d1.getSizeBytes()), + anyLong()); verify(actionFileSystem) .injectRemoteFile( eq(execRoot.asFragment().getRelative(a2.getExecPath())), eq(toBinaryDigest(d2)), - eq(d2.getSizeBytes())); + eq(d2.getSizeBytes()), + anyLong()); Path outputBase = checkNotNull(artifactRoot.getRoot().asPath()); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); assertThat(context.isLockOutputFilesCalled()).isTrue(); @@ -1254,7 +1262,8 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw .injectRemoteFile( eq(execRoot.asFragment().getRelative(a1.getExecPath())), eq(toBinaryDigest(d1)), - eq(d1.getSizeBytes())); + eq(d1.getSizeBytes()), + anyLong()); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/remote/options/RemoteOptionsTest.java b/src/test/java/com/google/devtools/build/lib/remote/options/RemoteOptionsTest.java index e6425a4889f650..90c09dfa6895a5 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/options/RemoteOptionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/options/RemoteOptionsTest.java @@ -79,7 +79,7 @@ public void testRemoteTimeoutOptionsConverterWithoutUnit() { try { int seconds = 60; Duration convert = - new RemoteOptions.RemoteTimeoutConverter().convert(String.valueOf(seconds)); + new CommonRemoteOptions.RemoteDurationConverter().convert(String.valueOf(seconds)); assertThat(Duration.ofSeconds(seconds)).isEqualTo(convert); } catch (OptionsParsingException e) { fail(e.getMessage()); @@ -90,7 +90,8 @@ public void testRemoteTimeoutOptionsConverterWithoutUnit() { public void testRemoteTimeoutOptionsConverterWithUnit() { try { int milliseconds = 60; - Duration convert = new RemoteOptions.RemoteTimeoutConverter().convert(milliseconds + "ms"); + Duration convert = + new CommonRemoteOptions.RemoteDurationConverter().convert(milliseconds + "ms"); assertThat(Duration.ofMillis(milliseconds)).isEqualTo(convert); } catch (OptionsParsingException e) { fail(e.getMessage()); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java index 07dfdbed185b93..f81bb23bc17ce6 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java @@ -43,10 +43,16 @@ public class ActionExecutionValueTest { private static final FileArtifactValue VALUE_1 = RemoteFileArtifactValue.create( - /* digest= */ new byte[0], /* size= */ 0, /* locationIndex= */ 1); + /* digest= */ new byte[0], + /* size= */ 0, + /* locationIndex= */ 1, + /* expireAtEpochMilli= */ -1); private static final FileArtifactValue VALUE_2 = RemoteFileArtifactValue.create( - /* digest= */ new byte[0], /* size= */ 0, /* locationIndex= */ 2); + /* digest= */ new byte[0], + /* size= */ 0, + /* locationIndex= */ 2, + /* expireAtEpochMilli= */ -1); private static final ActionLookupKey KEY = mock(ActionLookupKey.class); private static final ActionLookupData ACTION_LOOKUP_DATA_1 = ActionLookupData.create(KEY, 1); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java index 411c9dc44b780a..3ac63434af6047 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java @@ -307,7 +307,9 @@ public void resettingOutputs() throws Exception { assertThat(chmodCalls).containsExactly(outputPath, 0555); // Inject a remote file of size 42. - handler.injectFile(artifact, RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 42, 0)); + handler.injectFile( + artifact, + RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 42, 0, /* expireAtEpochMilli= */ -1)); assertThat(handler.getMetadata(artifact).getSize()).isEqualTo(42); // Reset this output, which will make the handler stat the file again. @@ -331,7 +333,9 @@ public void injectRemoteArtifactMetadata() throws Exception { byte[] digest = new byte[] {1, 2, 3}; int size = 10; handler.injectFile( - artifact, RemoteFileArtifactValue.create(digest, size, /* locationIndex= */ 1)); + artifact, + RemoteFileArtifactValue.create( + digest, size, /* locationIndex= */ 1, /* expireAtEpochMilli= */ -1)); FileArtifactValue v = handler.getMetadata(artifact); assertThat(v).isNotNull(); @@ -354,7 +358,8 @@ public void cannotInjectTreeArtifactChildIndividually() { /* outputs= */ ImmutableSet.of(treeArtifact)); handler.prepareForActionExecution(); - RemoteFileArtifactValue childValue = RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1); + RemoteFileArtifactValue childValue = + RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1, /* expireAtEpochMilli= */ -1); assertThrows(IllegalArgumentException.class, () -> handler.injectFile(child, childValue)); assertThat(handler.getOutputStore().getAllArtifactData()).isEmpty(); @@ -378,7 +383,8 @@ public void canInjectTemplateExpansionOutput() { /* outputs= */ ImmutableSet.of(treeArtifact)); handler.prepareForActionExecution(); - RemoteFileArtifactValue value = RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1); + RemoteFileArtifactValue value = + RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1, /* expireAtEpochMilli= */ -1); handler.injectFile(output, value); assertThat(handler.getOutputStore().getAllArtifactData()).containsExactly(output, value); @@ -402,10 +408,12 @@ public void injectRemoteTreeArtifactMetadata() throws Exception { TreeArtifactValue.newBuilder(treeArtifact) .putChild( TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), - RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1)) + RemoteFileArtifactValue.create( + new byte[] {1, 2, 3}, 5, 1, /* expireAtEpochMilli= */ -1)) .putChild( TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), - RemoteFileArtifactValue.create(new byte[] {4, 5, 6}, 10, 1)) + RemoteFileArtifactValue.create( + new byte[] {4, 5, 6}, 10, 1, /* expireAtEpochMilli= */ -1)) .build(); handler.injectTree(treeArtifact, tree); @@ -638,7 +646,8 @@ public void transformAfterInputDiscovery() throws Exception { assertThat(handler.getMetadata(unknown)).isNull(); OutputStore newStore = new OutputStore(); - FileArtifactValue knownMetadata = RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1); + FileArtifactValue knownMetadata = + RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1, /* expireAtEpochMilli= */ -1); newStore.putArtifactData(known, knownMetadata); ActionMetadataHandler newHandler = handler.transformAfterInputDiscovery(newStore); assertThat(newHandler.getOutputStore()).isNotEqualTo(handler.getOutputStore()); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java index 0435a19691606f..74fd5962c50196 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java @@ -20,6 +20,7 @@ import com.google.common.io.BaseEncoding; import com.google.common.testing.EqualsTester; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.DigestHashFunction; @@ -65,19 +66,23 @@ public void testEqualsAndHashCode() { new EqualsTester() .addEqualityGroup( FileArtifactValue.createForNormalFile( - toBytes("00112233445566778899AABBCCDDEEFF"), /*proxy=*/ null, 1L), + toBytes("00112233445566778899AABBCCDDEEFF"), /* proxy= */ null, 1L), FileArtifactValue.createForNormalFile( - toBytes("00112233445566778899AABBCCDDEEFF"), /*proxy=*/ null, 1L)) + toBytes("00112233445566778899AABBCCDDEEFF"), /* proxy= */ null, 1L)) .addEqualityGroup( FileArtifactValue.createForNormalFile( - toBytes("00112233445566778899AABBCCDDEEFF"), /*proxy=*/ null, 2L)) + toBytes("00112233445566778899AABBCCDDEEFF"), /* proxy= */ null, 2L)) .addEqualityGroup(FileArtifactValue.createForDirectoryWithMtime(1)) .addEqualityGroup( FileArtifactValue.createForNormalFile( - toBytes("FFFFFF00000000000000000000000000"), /*proxy=*/ null, 1L)) + toBytes("FFFFFF00000000000000000000000000"), /* proxy= */ null, 1L)) .addEqualityGroup( FileArtifactValue.createForDirectoryWithMtime(2), FileArtifactValue.createForDirectoryWithMtime(2)) + .addEqualityGroup( + // expireAtEpochMilli doesn't contribute to the equality + RemoteFileArtifactValue.create(toBytes("00112233445566778899AABBCCDDEEFF"), 1, 1, 1), + RemoteFileArtifactValue.create(toBytes("00112233445566778899AABBCCDDEEFF"), 1, 1, 2)) .addEqualityGroup(FileArtifactValue.OMITTED_FILE_MARKER) .addEqualityGroup(FileArtifactValue.MISSING_FILE_MARKER) .addEqualityGroup(FileArtifactValue.DEFAULT_MIDDLEMAN) 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 d838dd77db39c4..ecff843ceb530c 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 @@ -1311,10 +1311,15 @@ private static ActionExecutionValue actionValueWithRemoteArtifact( } private RemoteFileArtifactValue createRemoteFileArtifactValue(String contents) { + return createRemoteFileArtifactValue(contents, /* expireAtEpochMilli= */ -1); + } + + private RemoteFileArtifactValue createRemoteFileArtifactValue( + String contents, long expireAtEpochMilli) { byte[] data = contents.getBytes(); DigestHashFunction hashFn = fs.getDigestFunction(); HashCode hash = hashFn.getHashFunction().hashBytes(data); - return RemoteFileArtifactValue.create(hash.asBytes(), data.length, -1); + return RemoteFileArtifactValue.create(hash.asBytes(), data.length, -1, expireAtEpochMilli); } @Test @@ -1428,6 +1433,47 @@ public void testRemoteAndLocalArtifacts_identicalContent() throws Exception { .containsExactly(actionKey1); } + @Test + public void testRemoteArtifactsExpired() throws Exception { + // Test that if injected remote artifacts expired, they are considered as dirty. + SkyKey actionKey1 = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); + SkyKey actionKey2 = ActionLookupData.create(ACTION_LOOKUP_KEY, 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", /* expireAtEpochMilli= */ 0))); + differencer.inject(metadataToInject); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setParallelism(1) + .setEventHandler(NullEventHandler.INSTANCE) + .build(); + assertThat( + evaluator + .evaluate(ImmutableList.of(actionKey1, actionKey2), evaluationContext) + .hasError()) + .isFalse(); + assertThat( + new FilesystemValueChecker( + /* tsgm= */ null, SyscallCache.NO_CACHE, FSVC_THREADS_FOR_TEST) + .getDirtyActionValues( + evaluator.getValues(), + /* batchStatter= */ null, + ModifiedFileSet.EVERYTHING_MODIFIED, + /* trustRemoteArtifacts= */ true, + (ignored, ignored2) -> {})) + .containsExactly(actionKey2); + } + @Test public void testRemoteAndLocalTreeArtifacts() throws Exception { // Test that injected remote tree artifacts are trusted by the FileSystemValueChecker @@ -1463,7 +1509,7 @@ public void testRemoteAndLocalTreeArtifacts() throws Exception { evaluator.getValues(), /* batchStatter= */ null, ModifiedFileSet.EVERYTHING_MODIFIED, - /* trustRemoteArtifacts= */ false, + /* trustRemoteArtifacts= */ true, (ignored, ignored2) -> {})) .isEmpty(); @@ -1517,7 +1563,7 @@ public void testRemoteAndLocalTreeArtifacts_identicalContent() throws Exception evaluator.getValues(), /* batchStatter= */ null, ModifiedFileSet.EVERYTHING_MODIFIED, - /* trustRemoteArtifacts= */ false, + /* trustRemoteArtifacts= */ true, (ignored, ignored2) -> {})) .isEmpty(); @@ -1532,7 +1578,89 @@ public void testRemoteAndLocalTreeArtifacts_identicalContent() throws Exception evaluator.getValues(), /* batchStatter= */ null, ModifiedFileSet.EVERYTHING_MODIFIED, - /* trustRemoteArtifacts= */ false, + /* trustRemoteArtifacts= */ true, + (ignored, ignored2) -> {})) + .containsExactly(actionKey); + } + + @Test + public void testRemoteTreeArtifactsExpired() throws Exception { + // Test that if injected remote tree artifacts are expired, they are considered as dirty. + SkyKey actionKey = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); + + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + treeArtifact.getPath().createDirectoryAndParents(); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(treeArtifact) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), + createRemoteFileArtifactValue("foo-content")) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), + createRemoteFileArtifactValue("bar-content", /* expireAtEpochMilli= */ 0)) + .build(); + + differencer.inject(ImmutableMap.of(actionKey, actionValueWithTreeArtifact(treeArtifact, tree))); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setParallelism(1) + .setEventHandler(NullEventHandler.INSTANCE) + .build(); + assertThat(evaluator.evaluate(ImmutableList.of(actionKey), evaluationContext).hasError()) + .isFalse(); + assertThat( + new FilesystemValueChecker( + /* tsgm= */ null, SyscallCache.NO_CACHE, FSVC_THREADS_FOR_TEST) + .getDirtyActionValues( + evaluator.getValues(), + /* batchStatter= */ null, + ModifiedFileSet.EVERYTHING_MODIFIED, + /* trustRemoteArtifacts= */ true, + (ignored, ignored2) -> {})) + .containsExactly(actionKey); + } + + @Test + public void testRemoteTreeArtifacts_archivedRepresentationExpired() throws Exception { + // Test that if archived representation of injected remote tree artifacts are expired, they are + // considered as dirty. + SkyKey actionKey = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); + + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + treeArtifact.getPath().createDirectoryAndParents(); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(treeArtifact) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), + createRemoteFileArtifactValue("foo-content")) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), + createRemoteFileArtifactValue("bar-content")) + .setArchivedRepresentation( + createArchivedTreeArtifactWithContent(treeArtifact), + createRemoteFileArtifactValue("archived", /* expireAtEpochMilli= */ 0)) + .build(); + + differencer.inject(ImmutableMap.of(actionKey, actionValueWithTreeArtifact(treeArtifact, tree))); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setParallelism(1) + .setEventHandler(NullEventHandler.INSTANCE) + .build(); + assertThat(evaluator.evaluate(ImmutableList.of(actionKey), evaluationContext).hasError()) + .isFalse(); + assertThat( + new FilesystemValueChecker( + /* tsgm= */ null, SyscallCache.NO_CACHE, FSVC_THREADS_FOR_TEST) + .getDirtyActionValues( + evaluator.getValues(), + /* batchStatter= */ null, + ModifiedFileSet.EVERYTHING_MODIFIED, + /* trustRemoteArtifacts= */ true, (ignored, ignored2) -> {})) .containsExactly(actionKey); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java index 62a8c848aa7a6f..61603cf3a7665b 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java @@ -616,10 +616,16 @@ public void remoteDirectoryInjection() throws Exception { SpecialArtifact out = createTreeArtifact("output"); RemoteFileArtifactValue remoteFile1 = RemoteFileArtifactValue.create( - Hashing.sha256().hashString("one", UTF_8).asBytes(), /*size=*/ 3, /*locationIndex=*/ 1); + Hashing.sha256().hashString("one", UTF_8).asBytes(), + /* size= */ 3, + /* locationIndex= */ 1, + /* expireAtEpochMilli= */ -1); RemoteFileArtifactValue remoteFile2 = RemoteFileArtifactValue.create( - Hashing.sha256().hashString("two", UTF_8).asBytes(), /*size=*/ 3, /*locationIndex=*/ 2); + Hashing.sha256().hashString("two", UTF_8).asBytes(), + /* size= */ 3, + /* locationIndex= */ 2, + /* expireAtEpochMilli= */ -1); Action action = new SimpleTestAction(out) { diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java index 663f3784b81f52..9ef88e9394ae36 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java @@ -711,7 +711,8 @@ private static SpecialArtifact createTreeArtifact(String execPath, ArtifactRoot } private static FileArtifactValue metadataWithId(int id) { - return RemoteFileArtifactValue.create(new byte[] {(byte) id}, id, id); + return RemoteFileArtifactValue.create( + new byte[] {(byte) id}, id, id, /* expireAtEpochMilli= */ -1); } private static FileArtifactValue metadataWithIdNoDigest(int id) {