From 8bccaeab0151327590d5b125bc96cfc8447cf580 Mon Sep 17 00:00:00 2001 From: George Gensure Date: Fri, 3 Apr 2020 22:49:47 -0400 Subject: [PATCH 1/6] Anchor input fetches to source action id Provide the specific action id via RequestMetadata which provided an action input artifact when using remote_download_minimal. This replaces the unattributable "fetch-remote-inputs" identifier populated for each input via a nested context. --- .../build/lib/actions/ActionCacheChecker.java | 5 ++ .../build/lib/actions/FileArtifactValue.java | 51 ++++++++++++++++++- .../lib/actions/cache/MetadataInjector.java | 3 +- .../lib/remote/RemoteActionInputFetcher.java | 15 ++++-- .../build/lib/remote/RemoteCache.java | 12 +++-- .../build/lib/remote/RemoteModule.java | 11 ++-- .../build/lib/remote/RemoteSpawnCache.java | 1 + .../build/lib/remote/RemoteSpawnRunner.java | 4 ++ .../lib/remote/util/TracingMetadataUtils.java | 23 +++++---- .../lib/skyframe/ActionMetadataHandler.java | 4 +- .../build/lib/skyframe/OutputStore.java | 4 +- .../lib/actions/util/ActionsTestUtil.java | 2 +- ...eStreamBuildEventArtifactUploaderTest.java | 2 +- .../remote/RemoteActionFileSystemTest.java | 2 +- .../remote/RemoteActionInputFetcherTest.java | 13 ++--- .../build/lib/remote/RemoteCacheTests.java | 18 ++++--- .../lib/remote/RemoteSpawnCacheTest.java | 8 +-- .../lib/remote/RemoteSpawnRunnerTest.java | 10 ++-- .../util/FakeSpawnExecutionContext.java | 2 +- 19 files changed, 137 insertions(+), 53 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 d8d32f858748e0..0dcc5cd49c0457 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 @@ -602,6 +602,11 @@ public long getSize() { return 0; } + @Override + public String getActionId() { + return ""; + } + @Override public long getModifiedTime() { return -1; 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 8900a7a7861ff7..f0e9b2d6dfc6e5 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 @@ -82,6 +82,8 @@ public abstract class FileArtifactValue implements SkyValue, HasDigest { // TODO(ulfjack): Throw an exception if it's not a file. public abstract long getSize(); + public abstract String getActionId(); + /** * Returns the last modified time; see the documentation of {@link #getDigest} for when this can * and should be called. @@ -365,6 +367,11 @@ public long getSize() { return 0; } + @Override + public String getActionId() { + return ""; + } + @Override public boolean wasModifiedSinceDigest(Path path) throws IOException { return false; @@ -425,6 +432,11 @@ public long getSize() { return 0; } + @Override + public String getActionId() { + return ""; + } + @Override public boolean wasModifiedSinceDigest(Path path) { // TODO(ulfjack): Ideally, we'd attempt to detect intra-build modifications here. I'm @@ -490,6 +502,11 @@ public long getSize() { return size; } + @Override + public String getActionId() { + return ""; + } + @Override public boolean wasModifiedSinceDigest(Path path) throws IOException { if (proxy == null) { @@ -544,11 +561,13 @@ public static final class RemoteFileArtifactValue extends FileArtifactValue { private final byte[] digest; private final long size; private final int locationIndex; + private final String actionId; - public RemoteFileArtifactValue(byte[] digest, long size, int locationIndex) { + public RemoteFileArtifactValue(byte[] digest, long size, int locationIndex, String actionId) { this.digest = digest; this.size = size; this.locationIndex = locationIndex; + this.actionId = actionId; } @Override @@ -588,6 +607,11 @@ public long getSize() { return size; } + @Override + public String getActionId() { + return actionId; + } + @Override public long getModifiedTime() { throw new UnsupportedOperationException( @@ -642,6 +666,11 @@ public long getSize() { return 0; } + @Override + public String getActionId() { + return ""; + } + @Override public long getModifiedTime() { throw new IllegalStateException(); @@ -720,6 +749,11 @@ public long getSize() { return data.length; } + @Override + public String getActionId() { + return ""; + } + @Override public long getModifiedTime() { throw new UnsupportedOperationException(); @@ -802,6 +836,11 @@ public long getSize() { return size; } + @Override + public String getActionId() { + return ""; + } + @Override public long getModifiedTime() { throw new UnsupportedOperationException(); @@ -825,6 +864,11 @@ public byte[] getDigest() { return null; } + @Override + public String getActionId() { + return ""; + } + @Override public FileContentsProxy getContentsProxy() { throw new UnsupportedOperationException(); @@ -868,6 +912,11 @@ public byte[] getDigest() { throw new UnsupportedOperationException(); } + @Override + public String getActionId() { + throw new UnsupportedOperationException(); + } + @Override public FileContentsProxy getContentsProxy() { throw new UnsupportedOperationException(); diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java index 78ed3899c1ea84..ced63bd28a3220 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java @@ -31,8 +31,9 @@ public interface MetadataInjector { * @param digest the digest of the file. * @param size the size of the file in bytes. * @param locationIndex is only used in Blaze. + * @param actionId the id of the action that produced this file. */ - void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex); + void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex, String actionId); /** * Inject the metadata of a tree artifact whose contents are stored remotely. diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java index b47b7283a272c2..f1bc0af2dc742b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.remote; import build.bazel.remote.execution.v2.Digest; +import build.bazel.remote.execution.v2.RequestMetadata; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; @@ -32,6 +33,7 @@ import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.vfs.Path; import io.grpc.Context; @@ -66,12 +68,12 @@ class RemoteActionInputFetcher implements ActionInputPrefetcher { private final RemoteCache remoteCache; private final Path execRoot; - private final Context ctx; + private final RequestMetadata requestMetadata; - RemoteActionInputFetcher(RemoteCache remoteCache, Path execRoot, Context ctx) { + RemoteActionInputFetcher(RemoteCache remoteCache, Path execRoot, RequestMetadata requestMetadata) { this.remoteCache = Preconditions.checkNotNull(remoteCache); this.execRoot = Preconditions.checkNotNull(execRoot); - this.ctx = Preconditions.checkNotNull(ctx); + this.requestMetadata = Preconditions.checkNotNull(requestMetadata); } /** @@ -125,9 +127,10 @@ public void prefetchFiles( e = new IOException( String.format( - "Failed to fetch file with hash '%s' because it does not exist remotely." + "Failed to fetch file with hash '%s' into '%s' because it does not exist remotely." + " --experimental_remote_outputs=minimal does not work if" + " your remote cache evicts files during builds.", + entry.getKey(), ((CacheNotFoundException) e).getMissingDigest().getHash())); } ioException = ioException == null ? e : ioException; @@ -172,6 +175,10 @@ private ListenableFuture downloadFileAsync(Path path, FileArtifactValue me ListenableFuture download = downloadsInProgress.get(path); if (download == null) { + Context ctx = TracingMetadataUtils.contextWithMetadata( + requestMetadata.toBuilder() + .setActionId(metadata.getActionId()) + .build()); Context prevCtx = ctx.attach(); try { Digest digest = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize()); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index e104671786de78..ccbcf7f14f432d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -528,6 +528,7 @@ private List> downloadOutErr(ActionResult result, */ @Nullable public InMemoryOutput downloadMinimal( + String actionId, ActionResult result, Collection outputs, @Nullable PathFragment inMemoryOutputPath, @@ -565,7 +566,7 @@ public InMemoryOutput downloadMinimal( inMemoryOutput = output; } if (output instanceof Artifact) { - injectRemoteArtifact((Artifact) output, metadata, execRoot, metadataInjector); + injectRemoteArtifact((Artifact) output, metadata, execRoot, metadataInjector, actionId); } } @@ -589,7 +590,8 @@ private void injectRemoteArtifact( Artifact output, ActionResultMetadata metadata, Path execRoot, - MetadataInjector metadataInjector) + MetadataInjector metadataInjector, + String actionId) throws IOException { if (output.isTreeArtifact()) { DirectoryMetadata directory = @@ -612,7 +614,8 @@ private void injectRemoteArtifact( new RemoteFileArtifactValue( DigestUtil.toBinaryDigest(file.digest()), file.digest().getSizeBytes(), - /* locationIndex= */ 1); + /* locationIndex= */ 1, + actionId); childMetadata.put(p, r); } metadataInjector.injectRemoteDirectory( @@ -628,7 +631,8 @@ private void injectRemoteArtifact( output, DigestUtil.toBinaryDigest(outputMetadata.digest()), outputMetadata.digest().getSizeBytes(), - /* locationIndex= */ 1); + /* locationIndex= */ 1, + actionId); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index ee2107e722e811..e2478fb104575b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.remote; import build.bazel.remote.execution.v2.DigestFunction; +import build.bazel.remote.execution.v2.RequestMetadata; import build.bazel.remote.execution.v2.ServerCapabilities; import com.google.auth.Credentials; import com.google.common.base.Preconditions; @@ -662,12 +663,14 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions"); RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode; if (!remoteOutputsMode.downloadAllOutputs()) { - Context ctx = - TracingMetadataUtils.contextWithMetadata( - env.getBuildRequestId(), env.getCommandId().toString(), "fetch-remote-inputs"); + RequestMetadata requestMetadata = + RequestMetadata.newBuilder() + .setCorrelatedInvocationsId(env.getBuildRequestId()) + .setToolInvocationId(env.getCommandId().toString()) + .build(); actionInputFetcher = new RemoteActionInputFetcher( - actionContextProvider.getRemoteCache(), env.getExecRoot(), ctx); + actionContextProvider.getRemoteCache(), env.getExecRoot(), requestMetadata); builder.setActionInputPrefetcher(actionInputFetcher); remoteOutputService.setActionInputFetcher(actionInputFetcher); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index 88023c1c0aa08d..7b0a6742158fc6 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java @@ -186,6 +186,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) prof.profile(ProfilerTask.REMOTE_DOWNLOAD, "download outputs minimal")) { inMemoryOutput = remoteCache.downloadMinimal( + actionKey.getDigest().getHash(), result, spawn.getOutputFiles(), inMemoryOutputPath, diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index a2e00a30058ab4..ebfb0e276143b3 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -262,6 +262,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) } else { try { return downloadAndFinalizeSpawnResult( + actionKey.getDigest().getHash(), cachedResult, /* cacheHit= */ true, spawn, @@ -341,6 +342,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) try { return downloadAndFinalizeSpawnResult( + actionKey.getDigest().getHash(), actionResult, reply.getCachedResult(), spawn, @@ -413,6 +415,7 @@ static void spawnMetricsAccounting( } private SpawnResult downloadAndFinalizeSpawnResult( + String actionId, ActionResult actionResult, boolean cacheHit, Spawn spawn, @@ -441,6 +444,7 @@ private SpawnResult downloadAndFinalizeSpawnResult( Profiler.instance().profile(REMOTE_DOWNLOAD, "download outputs minimal")) { inMemoryOutput = remoteCache.downloadMinimal( + actionId, actionResult, spawn.getOutputFiles(), inMemoryOutputPath, diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/TracingMetadataUtils.java b/src/main/java/com/google/devtools/build/lib/remote/util/TracingMetadataUtils.java index a4b5511688cf19..79a0cbb8afa3ee 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/TracingMetadataUtils.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/TracingMetadataUtils.java @@ -70,18 +70,21 @@ public static Context contextWithMetadata( Preconditions.checkNotNull(buildRequestId); Preconditions.checkNotNull(commandId); Preconditions.checkNotNull(actionId); - RequestMetadata.Builder metadata = + RequestMetadata metadata = RequestMetadata.newBuilder() .setCorrelatedInvocationsId(buildRequestId) - .setToolInvocationId(commandId); - metadata.setActionId(actionId); - metadata - .setToolDetails( - ToolDetails.newBuilder() - .setToolName("bazel") - .setToolVersion(BlazeVersionInfo.instance().getVersion())) - .build(); - return Context.current().withValue(CONTEXT_KEY, metadata.build()); + .setToolInvocationId(commandId) + .setActionId(actionId) + .setToolDetails( + ToolDetails.newBuilder() + .setToolName("bazel") + .setToolVersion(BlazeVersionInfo.instance().getVersion())) + .build(); + return contextWithMetadata(metadata); + } + + public static Context contextWithMetadata(RequestMetadata metadata) { + return Context.current().withValue(CONTEXT_KEY, metadata); } /** diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index 347c5f5cec17fd..f6adb0a89d6f90 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -542,7 +542,7 @@ public void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] dig } @Override - public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) { + public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex, String actionId) { Preconditions.checkArgument( isKnownOutput(output), output + " is not a declared output of this action"); Preconditions.checkArgument( @@ -551,7 +551,7 @@ public void injectRemoteFile(Artifact output, byte[] digest, long size, int loca output); Preconditions.checkState( executionMode.get(), "Tried to inject %s outside of execution", output); - store.injectRemoteFile(output, digest, size, locationIndex); + store.injectRemoteFile(output, digest, size, locationIndex, actionId); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java index bc7525845e60a4..f7fe7e2332ca00 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java @@ -95,9 +95,9 @@ void addTreeArtifactContents(Artifact artifact, TreeFileArtifact contents) { treeArtifactContents.computeIfAbsent(artifact, a -> Sets.newConcurrentHashSet()).add(contents); } - void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) { + void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex, String actionId) { injectOutputData( - output, new FileArtifactValue.RemoteFileArtifactValue(digest, size, locationIndex)); + output, new FileArtifactValue.RemoteFileArtifactValue(digest, size, locationIndex, actionId)); } final void injectOutputData(Artifact output, FileArtifactValue artifactValue) { diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java index 792fe3affc88c4..767983d914de35 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java @@ -967,7 +967,7 @@ public void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] dig } @Override - public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) { + public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex, String actionId) { throw new UnsupportedOperationException(); } 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 7fc54691a61fc4..51881924f74597 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 @@ -360,7 +360,7 @@ private Artifact createRemoteArtifact( byte[] b = contents.getBytes(StandardCharsets.UTF_8); HashCode h = HashCode.fromString(DIGEST_UTIL.compute(b).getHash()); FileArtifactValue f = - new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1); + new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1, "action-id"); 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 cdc5946f379ce5..0262d442c030b4 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 @@ -172,7 +172,7 @@ private Artifact createRemoteArtifact( byte[] b = contents.getBytes(StandardCharsets.UTF_8); HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b); FileArtifactValue f = - new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1); + new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1, "action-id"); inputs.putWithNoDepOwner(a, f); return a; } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java index 2453cfed5e0a5c..bf274f6ab4ce40 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java @@ -17,6 +17,7 @@ import static org.junit.Assert.assertThrows; import build.bazel.remote.execution.v2.Digest; +import build.bazel.remote.execution.v2.RequestMetadata; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; @@ -85,7 +86,7 @@ public void testFetching() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); RemoteCache remoteCache = newCache(options, digestUtil, cacheEntries); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher(remoteCache, execRoot, Context.current()); + new RemoteActionInputFetcher(remoteCache, execRoot, RequestMetadata.getDefaultInstance()); // act actionInputFetcher.prefetchFiles(metadata.keySet(), metadataProvider); @@ -108,7 +109,7 @@ public void testStagingVirtualActionInput() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(new HashMap<>()); RemoteCache remoteCache = newCache(options, digestUtil, new HashMap<>()); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher(remoteCache, execRoot, Context.current()); + new RemoteActionInputFetcher(remoteCache, execRoot, RequestMetadata.getDefaultInstance()); VirtualActionInput a = new StringActionInput("hello world", PathFragment.create("file1")); // act @@ -133,7 +134,7 @@ public void testFileNotFound() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); RemoteCache remoteCache = newCache(options, digestUtil, new HashMap<>()); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher(remoteCache, execRoot, Context.current()); + new RemoteActionInputFetcher(remoteCache, execRoot, RequestMetadata.getDefaultInstance()); // act assertThrows( @@ -157,7 +158,7 @@ public void testIgnoreNoneRemoteFiles() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(ImmutableMap.of(a, f)); RemoteCache remoteCache = newCache(options, digestUtil, new HashMap<>()); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher(remoteCache, execRoot, Context.current()); + new RemoteActionInputFetcher(remoteCache, execRoot, RequestMetadata.getDefaultInstance()); // act actionInputFetcher.prefetchFiles(ImmutableList.of(a), metadataProvider); @@ -175,7 +176,7 @@ public void testDownloadFile() throws Exception { Artifact a1 = createRemoteArtifact("file1", "hello world", metadata, cacheEntries); RemoteCache remoteCache = newCache(options, digestUtil, cacheEntries); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher(remoteCache, execRoot, Context.current()); + new RemoteActionInputFetcher(remoteCache, execRoot, RequestMetadata.getDefaultInstance()); // act actionInputFetcher.downloadFile(a1.getPath(), metadata.get(a1)); @@ -198,7 +199,7 @@ private Artifact createRemoteArtifact( byte[] b = contents.getBytes(StandardCharsets.UTF_8); HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b); FileArtifactValue f = - new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1); + new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1, "action-id"); metadata.put(a, f); cacheEntries.put(DigestUtil.buildDigest(h.asBytes(), b.length), ByteString.copyFrom(b)); return a; diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java index af617f49a95cbc..a61fbe6e793d27 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java @@ -17,6 +17,7 @@ import static com.google.devtools.build.lib.remote.util.DigestUtil.toBinaryDigest; 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.anyInt; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; @@ -915,6 +916,7 @@ public void testDownloadMinimalFiles() throws Exception { // act InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( + "action-id", r, ImmutableList.of(a1, a2), /* inMemoryOutputPath= */ null, @@ -926,9 +928,9 @@ public void testDownloadMinimalFiles() throws Exception { // assert assertThat(inMemoryOutput).isNull(); verify(injector) - .injectRemoteFile(eq(a1), eq(toBinaryDigest(d1)), eq(d1.getSizeBytes()), anyInt()); + .injectRemoteFile(eq(a1), eq(toBinaryDigest(d1)), eq(d1.getSizeBytes()), anyInt(), any()); verify(injector) - .injectRemoteFile(eq(a2), eq(toBinaryDigest(d2)), eq(d2.getSizeBytes()), anyInt()); + .injectRemoteFile(eq(a2), eq(toBinaryDigest(d2)), eq(d2.getSizeBytes()), anyInt(), any()); Path outputBase = artifactRoot.getRoot().asPath(); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); @@ -977,6 +979,7 @@ public void testDownloadMinimalDirectory() throws Exception { // act InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( + "action-id", r, ImmutableList.of(dir), /* inMemoryOutputPath= */ null, @@ -992,10 +995,10 @@ public void testDownloadMinimalDirectory() throws Exception { ImmutableMap.builder() .put( PathFragment.create("file1"), - new RemoteFileArtifactValue(toBinaryDigest(d1), d1.getSizeBytes(), 1)) + new RemoteFileArtifactValue(toBinaryDigest(d1), d1.getSizeBytes(), 1, "action-id")) .put( PathFragment.create("a/file2"), - new RemoteFileArtifactValue(toBinaryDigest(d2), d2.getSizeBytes(), 1)) + new RemoteFileArtifactValue(toBinaryDigest(d2), d2.getSizeBytes(), 1, "action-id")) .build(); verify(injector).injectRemoteDirectory(eq(dir), eq(m)); @@ -1050,6 +1053,7 @@ public void testDownloadMinimalDirectoryFails() throws Exception { BulkTransferException.class, () -> remoteCache.downloadMinimal( + "action-id", r, ImmutableList.of(dir), /* inMemoryOutputPath= */ null, @@ -1083,6 +1087,7 @@ public void testDownloadMinimalWithStdoutStderr() throws Exception { // act InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( + "action-id", r, ImmutableList.of(), /* inMemoryOutputPath= */ null, @@ -1125,6 +1130,7 @@ public void testDownloadMinimalWithInMemoryOutput() throws Exception { // act InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( + "action-id", r, ImmutableList.of(a1, a2), inMemoryOutputPathFragment, @@ -1140,9 +1146,9 @@ public void testDownloadMinimalWithInMemoryOutput() throws Exception { assertThat(inMemoryOutput.getOutput()).isEqualTo(a1); // The in memory file also needs to be injected as an output verify(injector) - .injectRemoteFile(eq(a1), eq(toBinaryDigest(d1)), eq(d1.getSizeBytes()), anyInt()); + .injectRemoteFile(eq(a1), eq(toBinaryDigest(d1)), eq(d1.getSizeBytes()), anyInt(), any()); verify(injector) - .injectRemoteFile(eq(a2), eq(toBinaryDigest(d2)), eq(d2.getSizeBytes()), anyInt()); + .injectRemoteFile(eq(a2), eq(toBinaryDigest(d2)), eq(d2.getSizeBytes()), anyInt(), any()); Path outputBase = artifactRoot.getRoot().asPath(); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index c33b9377c43e6f..fa44aa4ec84ed1 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -180,7 +180,7 @@ public MetadataInjector getMetadataInjector() { return new MetadataInjector() { @Override public void injectRemoteFile( - Artifact output, byte[] digest, long size, int locationIndex) { + Artifact output, byte[] digest, long size, int locationIndex, String actionId) { throw new UnsupportedOperationException(); } @@ -690,7 +690,7 @@ public void testDownloadMinimal() throws Exception { // assert assertThat(cacheHandle.hasResult()).isTrue(); assertThat(cacheHandle.getResult().exitCode()).isEqualTo(0); - verify(remoteCache).downloadMinimal(any(), anyCollection(), any(), any(), any(), any(), any()); + verify(remoteCache).downloadMinimal(any(), any(), anyCollection(), any(), any(), any(), any(), any()); } @Test @@ -705,7 +705,7 @@ public void testDownloadMinimalIoError() throws Exception { ActionResult success = ActionResult.newBuilder().setExitCode(0).build(); when(remoteCache.downloadActionResult(any(), /* inlineOutErr= */ eq(false))) .thenReturn(success); - when(remoteCache.downloadMinimal(any(), anyCollection(), any(), any(), any(), any(), any())) + when(remoteCache.downloadMinimal(any(), any(), anyCollection(), any(), any(), any(), any(), any())) .thenThrow(downloadFailure); // act @@ -713,7 +713,7 @@ public void testDownloadMinimalIoError() throws Exception { // assert assertThat(cacheHandle.hasResult()).isFalse(); - verify(remoteCache).downloadMinimal(any(), anyCollection(), any(), any(), any(), any(), any()); + verify(remoteCache).downloadMinimal(any(), any(), anyCollection(), any(), any(), any(), any(), any()); assertThat(eventHandler.getEvents().size()).isEqualTo(1); Event evt = eventHandler.getEvents().get(0); assertThat(evt.getKind()).isEqualTo(EventKind.WARNING); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index 00ecddf28f19cb..4f30f8785e99a2 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -885,7 +885,7 @@ public void testDownloadMinimalOnCacheHit() throws Exception { // assert verify(cache) - .downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any(), any()); + .downloadMinimal(any(), eq(succeededAction), anyCollection(), any(), any(), any(), any(), any()); verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr), any()); } @@ -911,7 +911,7 @@ public void testDownloadMinimalOnCacheMiss() throws Exception { // assert verify(executor).executeRemotely(any()); verify(cache) - .downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any(), any()); + .downloadMinimal(any(), eq(succeededAction), anyCollection(), any(), any(), any(), any(), any()); verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr), any()); } @@ -924,7 +924,7 @@ public void testDownloadMinimalIoError() throws Exception { when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) .thenReturn(succeededAction); IOException downloadFailure = new IOException("downloadMinimal failed"); - when(cache.downloadMinimal(any(), anyCollection(), any(), any(), any(), any(), any())) + when(cache.downloadMinimal(any(), any(), anyCollection(), any(), any(), any(), any(), any())) .thenThrow(downloadFailure); RemoteSpawnRunner runner = newSpawnRunner(); @@ -938,7 +938,7 @@ public void testDownloadMinimalIoError() throws Exception { // assert verify(cache) - .downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any(), any()); + .downloadMinimal(any(), eq(succeededAction), anyCollection(), any(), any(), any(), any(), any()); verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr), any()); } @@ -969,7 +969,7 @@ public void testDownloadTopLevel() throws Exception { // assert verify(cache).download(eq(succeededAction), any(Path.class), eq(outErr), any()); verify(cache, never()) - .downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any(), any()); + .downloadMinimal(any(), eq(succeededAction), anyCollection(), any(), any(), any(), any(), any()); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java b/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java index 67f7d302ac4be1..b6ab146539357d 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java +++ b/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java @@ -132,7 +132,7 @@ public void report(ProgressStatus state, String name) { public MetadataInjector getMetadataInjector() { return new MetadataInjector() { @Override - public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) { + public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex, String actionId) { throw new UnsupportedOperationException(); } From 648a895e018ff7da95a0523a76bf848b0fc851c2 Mon Sep 17 00:00:00 2001 From: George Gensure Date: Tue, 28 Apr 2020 10:06:58 -0400 Subject: [PATCH 2/6] Declare getActionId in TestMetadata --- .../devtools/build/lib/actions/ActionInputMapTest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java index 66afcb62871d04..f633f0aa16a8ad 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java @@ -187,6 +187,11 @@ public long getSize() { throw new UnsupportedOperationException(); } + @Override + public String getActionId() { + throw new UnsupportedOperationException(); + } + @Override public long getModifiedTime() { throw new UnsupportedOperationException(); From d595b826202413c2a4980dd355aab106de2d80cc Mon Sep 17 00:00:00 2001 From: George Gensure Date: Tue, 28 Apr 2020 10:41:54 -0400 Subject: [PATCH 3/6] Update skyframe tests to handle remote action id --- .../build/lib/skyframe/ActionMetadataHandlerTest.java | 8 ++++---- .../build/lib/skyframe/FilesystemValueCheckerTest.java | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) 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 a5c43a1bc10e92..c0b60940d782f1 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 @@ -332,7 +332,7 @@ public void resettingOutputs() throws Exception { assertThat(handler.getMetadata(artifact).getSize()).isEqualTo(10); // Inject a remote file of size 42. - handler.injectRemoteFile(artifact, new byte[] {1, 2, 3}, 42, 0); + handler.injectRemoteFile(artifact, new byte[] {1, 2, 3}, 42, 0, "ultimate-answer"); assertThat(handler.getMetadata(artifact).getSize()).isEqualTo(42); // Reset this output, which will make the handler stat the file again. @@ -358,7 +358,7 @@ public void injectRemoteArtifactMetadata() throws Exception { byte[] digest = new byte[] {1, 2, 3}; int size = 10; - handler.injectRemoteFile(artifact, digest, size, /* locationIndex= */ 1); + handler.injectRemoteFile(artifact, digest, size, /* locationIndex= */ 1, "action-id"); FileArtifactValue v = handler.getMetadata(artifact); assertThat(v).isNotNull(); @@ -386,8 +386,8 @@ public void injectRemoteTreeArtifactMetadata() throws Exception { outputRoot.getRoot().asPath()); handler.discardOutputMetadata(); - RemoteFileArtifactValue fooValue = new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 5, 1); - RemoteFileArtifactValue barValue = new RemoteFileArtifactValue(new byte[] {4, 5, 6}, 10, 1); + RemoteFileArtifactValue fooValue = new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 5, 1, "foo"); + RemoteFileArtifactValue barValue = new RemoteFileArtifactValue(new byte[] {4, 5, 6}, 10, 1, "bar"); Map children = ImmutableMap.builder() .put(PathFragment.create("foo"), fooValue) 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 0c8b8435c7034c..f12f1b6863dd8e 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 @@ -974,7 +974,7 @@ 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); + return new RemoteFileArtifactValue(hash.asBytes(), data.length, -1, "action-id"); } @Test From 40f7485ed4efcc639884c25a3881af1073a6ca33 Mon Sep 17 00:00:00 2001 From: George Gensure Date: Tue, 28 Apr 2020 10:47:07 -0400 Subject: [PATCH 4/6] Default getActionId and comment --- .../build/lib/actions/ActionCacheChecker.java | 5 -- .../build/lib/actions/FileArtifactValue.java | 51 ++++--------------- .../build/lib/actions/ActionInputMapTest.java | 5 -- 3 files changed, 9 insertions(+), 52 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 0dcc5cd49c0457..d8d32f858748e0 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 @@ -602,11 +602,6 @@ public long getSize() { return 0; } - @Override - public String getActionId() { - return ""; - } - @Override public long getModifiedTime() { return -1; 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 f0e9b2d6dfc6e5..46b1e79c164d92 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 @@ -82,8 +82,6 @@ public abstract class FileArtifactValue implements SkyValue, HasDigest { // TODO(ulfjack): Throw an exception if it's not a file. public abstract long getSize(); - public abstract String getActionId(); - /** * Returns the last modified time; see the documentation of {@link #getDigest} for when this can * and should be called. @@ -113,6 +111,15 @@ public int getLocationIndex() { return 0; } + /** + * Remote action source identifier for the file. + * + *

"" indicates that a remote action output was not the source of this artifact. + */ + public String getActionId() { + return ""; + } + /** Returns {@code true} if this is a special marker as opposed to a representing a real file. */ public boolean isMarkerValue() { return this instanceof Singleton; @@ -367,11 +374,6 @@ public long getSize() { return 0; } - @Override - public String getActionId() { - return ""; - } - @Override public boolean wasModifiedSinceDigest(Path path) throws IOException { return false; @@ -432,11 +434,6 @@ public long getSize() { return 0; } - @Override - public String getActionId() { - return ""; - } - @Override public boolean wasModifiedSinceDigest(Path path) { // TODO(ulfjack): Ideally, we'd attempt to detect intra-build modifications here. I'm @@ -502,11 +499,6 @@ public long getSize() { return size; } - @Override - public String getActionId() { - return ""; - } - @Override public boolean wasModifiedSinceDigest(Path path) throws IOException { if (proxy == null) { @@ -666,11 +658,6 @@ public long getSize() { return 0; } - @Override - public String getActionId() { - return ""; - } - @Override public long getModifiedTime() { throw new IllegalStateException(); @@ -749,11 +736,6 @@ public long getSize() { return data.length; } - @Override - public String getActionId() { - return ""; - } - @Override public long getModifiedTime() { throw new UnsupportedOperationException(); @@ -836,11 +818,6 @@ public long getSize() { return size; } - @Override - public String getActionId() { - return ""; - } - @Override public long getModifiedTime() { throw new UnsupportedOperationException(); @@ -864,11 +841,6 @@ public byte[] getDigest() { return null; } - @Override - public String getActionId() { - return ""; - } - @Override public FileContentsProxy getContentsProxy() { throw new UnsupportedOperationException(); @@ -912,11 +884,6 @@ public byte[] getDigest() { throw new UnsupportedOperationException(); } - @Override - public String getActionId() { - throw new UnsupportedOperationException(); - } - @Override public FileContentsProxy getContentsProxy() { throw new UnsupportedOperationException(); diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java index f633f0aa16a8ad..66afcb62871d04 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java @@ -187,11 +187,6 @@ public long getSize() { throw new UnsupportedOperationException(); } - @Override - public String getActionId() { - throw new UnsupportedOperationException(); - } - @Override public long getModifiedTime() { throw new UnsupportedOperationException(); From 1a3b79e7564190572271489d6ebae81fbdaedcaa Mon Sep 17 00:00:00 2001 From: George Gensure Date: Tue, 28 Apr 2020 11:39:30 -0400 Subject: [PATCH 5/6] Update tests since merge --- .../com/google/devtools/build/lib/remote/RemoteCacheTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java index c7695a84860c22..cfbbe7d55708af 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java @@ -1172,6 +1172,7 @@ public void testDownloadMinimalWithMissingInMemoryOutput() throws Exception { // act InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( + "action-id", r, ImmutableList.of(a1), inMemoryOutputPathFragment, @@ -1184,7 +1185,7 @@ public void testDownloadMinimalWithMissingInMemoryOutput() throws Exception { assertThat(inMemoryOutput).isNull(); // The in memory file metadata also should not have been injected. verify(injector, never()) - .injectRemoteFile(eq(a1), eq(toBinaryDigest(d1)), eq(d1.getSizeBytes()), anyInt()); + .injectRemoteFile(eq(a1), eq(toBinaryDigest(d1)), eq(d1.getSizeBytes()), anyInt(), any()); } @Test From a0aae21b8fc1dfa790dc7801786fa5fe95fa7a7f Mon Sep 17 00:00:00 2001 From: George Gensure Date: Tue, 28 Apr 2020 12:08:20 -0400 Subject: [PATCH 6/6] Remove entry 'into' error message copy --- .../devtools/build/lib/remote/RemoteActionInputFetcher.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java index a880718f022cb0..dadefa4ea8ee34 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java @@ -126,10 +126,9 @@ public void prefetchFiles( IOException annotatedException = new IOException( String.format( - "Failed to fetch file with hash '%s' into '%s' because it does not exist remotely." + "Failed to fetch file with hash '%s' because it does not exist remotely." + " --experimental_remote_outputs=minimal does not work if" + " your remote cache evicts files during builds.", - entry.getKey(), ((CacheNotFoundException) t).getMissingDigest().getHash())); bulkAnnotatedException.add(annotatedException); }