From 3ef8fb98e7becaac160003f5c24f35914b764bb9 Mon Sep 17 00:00:00 2001 From: George Gensure Date: Wed, 6 May 2020 09:49:48 -0700 Subject: [PATCH] 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. Closes #11236. PiperOrigin-RevId: 310170811 --- .../build/lib/actions/FileArtifactValue.java | 22 +++++++++++++++++- .../lib/actions/cache/MetadataInjector.java | 4 +++- .../lib/remote/RemoteActionInputFetcher.java | 12 +++++++--- .../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 | 5 ++-- .../build/lib/skyframe/OutputStore.java | 6 +++-- .../lib/actions/util/ActionsTestUtil.java | 3 ++- ...eStreamBuildEventArtifactUploaderTest.java | 2 +- .../remote/RemoteActionFileSystemTest.java | 2 +- .../remote/RemoteActionInputFetcherTest.java | 14 +++++------ .../build/lib/remote/RemoteCacheTests.java | 21 +++++++++++------ .../lib/remote/RemoteSpawnCacheTest.java | 11 +++++---- .../lib/remote/RemoteSpawnRunnerTest.java | 14 +++++++---- .../util/FakeSpawnExecutionContext.java | 3 ++- .../skyframe/ActionMetadataHandlerTest.java | 10 ++++---- .../skyframe/FilesystemValueCheckerTest.java | 2 +- 20 files changed, 123 insertions(+), 59 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index 8900a7a7861ff7..27a205fe1898aa 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 @@ -111,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; @@ -544,11 +553,17 @@ 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; + } + + public RemoteFileArtifactValue(byte[] digest, long size, int locationIndex) { + this(digest, size, locationIndex, /* actionId= */ ""); } @Override @@ -588,6 +603,11 @@ public long getSize() { return size; } + @Override + public String getActionId() { + return actionId; + } + @Override public long getModifiedTime() { 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..ee951364f30fd0 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,10 @@ 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 519b7f482d416e..4352d26ec7edf6 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.vfs.Path; import io.grpc.Context; import java.io.IOException; @@ -65,12 +67,13 @@ 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); } /** @@ -164,6 +167,9 @@ 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 95df36212eb43f..157d5d466e0f5a 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, @@ -570,7 +571,7 @@ public InMemoryOutput downloadMinimal( inMemoryOutput = output; } if (output instanceof Artifact) { - injectRemoteArtifact((Artifact) output, metadata, execRoot, metadataInjector); + injectRemoteArtifact((Artifact) output, metadata, execRoot, metadataInjector, actionId); } } @@ -594,7 +595,8 @@ private void injectRemoteArtifact( Artifact output, ActionResultMetadata metadata, Path execRoot, - MetadataInjector metadataInjector) + MetadataInjector metadataInjector, + String actionId) throws IOException { if (output.isTreeArtifact()) { DirectoryMetadata directory = @@ -617,7 +619,8 @@ private void injectRemoteArtifact( new RemoteFileArtifactValue( DigestUtil.toBinaryDigest(file.digest()), file.digest().getSizeBytes(), - /* locationIndex= */ 1); + /* locationIndex= */ 1, + actionId); childMetadata.put(p, r); } metadataInjector.injectRemoteDirectory( @@ -633,7 +636,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 62e519d8a3e4c5..1d480a83d2c9fd 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 4c3a617bb76245..b33338ac924da8 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 2b5a8931b0e08f..bd0e37887b2884 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..568650f1973b30 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,8 @@ 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 +552,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..914b8b5a527d0e 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,11 @@ 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..bc5af45de6e80d 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,8 @@ 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 530441a3a4f414..838e08782d6825 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; @@ -43,7 +44,6 @@ import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.devtools.common.options.Options; import com.google.protobuf.ByteString; -import io.grpc.Context; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.HashMap; @@ -85,7 +85,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 +108,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 +133,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 +157,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 +175,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 +198,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 4f2898ee07622c..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 @@ -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(); @@ -1166,6 +1172,7 @@ public void testDownloadMinimalWithMissingInMemoryOutput() throws Exception { // act InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( + "action-id", r, ImmutableList.of(a1), inMemoryOutputPathFragment, @@ -1178,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 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..1f35eba07fac98 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,8 @@ 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 +706,8 @@ 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 +715,8 @@ 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..a5b204bb732052 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,8 @@ 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 +912,8 @@ 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 +926,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 +940,8 @@ 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 +972,8 @@ 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..6b7ac85076417a 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,8 @@ 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(); } 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..47c45768be3edc 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,10 @@ 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