From 625dede11391fd9ce7a09db2988eef0b354e678d Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 12 Jun 2023 06:42:19 -0700 Subject: [PATCH] Add `ActionExecutionMetadata` as a parameter to `ActionInputPrefetcher#prefetchFiles`. Due to recent overhual of `prefetchFiles`, the call sites now always have the reference to `ActionExecutionMetadata`, which makes it possible to pass it to `prefetchFiles`. The reason why we want to access `ActionExecutionMetadata` inside `prefetchFiles` is to associate downloads with actions. For example: 1. When downloading from remote cache, we want to provide action metadata to the RPCs so that remote server can associate them with actions. `actionId` was removed from remote file metadata by f62a8b9f6a7e840c648f44f094390c3b85b236df because it is not the right action to associate with for downloads inside this context. We left the action metadata empty for RPCs until this change. https://github.com/bazelbuild/bazel/pull/17724#issuecomment-1542160017 2. When building with the bytes, we are able to report download progress to UI. However, `prefetchFiles` is the major way to download when building without the bytes and we can't report download progress on action basis. #17604 added the download report for background downloads, but now, we no longer download toplevel artifacts in background (a5dde12b0d945b54967969fafa4d5e40692f9a44). With this change, we are able to report download progress for each action, either when they download outputs, or prefetch inputs for building without the bytes. PiperOrigin-RevId: 539635790 Change-Id: Ic9cc1024f5d3560ac46bc462bd549dd81712d92f --- .../lib/actions/ActionInputPrefetcher.java | 6 +- .../build/lib/exec/AbstractSpawnStrategy.java | 1 + .../remote/AbstractActionInputPrefetcher.java | 12 +++- .../lib/remote/RemoteActionFileSystem.java | 7 ++- .../lib/remote/RemoteActionInputFetcher.java | 53 ++++------------ .../build/lib/remote/RemoteOutputService.java | 4 +- .../lib/skyframe/SkyframeActionExecutor.java | 8 ++- .../devtools/build/lib/vfs/OutputService.java | 2 + .../remote/ActionInputPrefetcherTestBase.java | 63 ++++++++++++------- .../remote/RemoteActionFileSystemTest.java | 16 ++--- .../remote/RemoteActionInputFetcherTest.java | 6 +- 11 files changed, 96 insertions(+), 82 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java index 16205eddb30194..8b65a8a5eec19e 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java @@ -34,6 +34,7 @@ public interface MetadataSupplier { new ActionInputPrefetcher() { @Override public ListenableFuture prefetchFiles( + ActionExecutionMetadata action, Iterable inputs, MetadataSupplier metadataSupplier, Priority priority) { @@ -79,7 +80,10 @@ public enum Priority { * @return future success if prefetch is finished or {@link IOException}. */ ListenableFuture prefetchFiles( - Iterable inputs, MetadataSupplier metadataSupplier, Priority priority); + ActionExecutionMetadata action, + Iterable inputs, + MetadataSupplier metadataSupplier, + Priority priority); /** * Whether the prefetcher requires the metadata for a tree artifact to be available whenever one diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java index 907039fd857481..2b0030fd187b7e 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java @@ -252,6 +252,7 @@ public ListenableFuture prefetchInputs() actionExecutionContext .getActionInputPrefetcher() .prefetchFiles( + spawn.getResourceOwner(), getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true) .values(), getInputMetadataProvider()::getInputMetadata, diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index 86a5ec7ad31a2f..d4533595848f76 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -29,6 +29,7 @@ import com.google.common.flogger.GoogleLogger; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputPrefetcher; import com.google.devtools.build.lib.actions.Artifact; @@ -263,6 +264,7 @@ private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) { * @param tempPath the temporary path which the input should be written to. */ protected abstract ListenableFuture doDownloadFile( + ActionExecutionMetadata action, Reporter reporter, Path tempPath, PathFragment execPath, @@ -285,6 +287,7 @@ protected void prefetchVirtualActionInput(VirtualActionInput input) throws IOExc */ @Override public ListenableFuture prefetchFiles( + ActionExecutionMetadata action, Iterable inputs, MetadataSupplier metadataSupplier, Priority priority) { @@ -308,7 +311,8 @@ public ListenableFuture prefetchFiles( Flowable transfers = Flowable.fromIterable(files) - .flatMapSingle(input -> prefetchFile(dirCtx, metadataSupplier, input, priority)); + .flatMapSingle( + input -> prefetchFile(action, dirCtx, metadataSupplier, input, priority)); Completable prefetch = Completable.using( @@ -318,6 +322,7 @@ public ListenableFuture prefetchFiles( } private Single prefetchFile( + ActionExecutionMetadata action, DirectoryContext dirCtx, MetadataSupplier metadataSupplier, ActionInput input, @@ -347,6 +352,7 @@ private Single prefetchFile( Completable result = downloadFileNoCheckRx( + action, dirCtx, execRoot.getRelative(execPath), treeRootExecPath != null ? execRoot.getRelative(treeRootExecPath) : null, @@ -421,6 +427,7 @@ private Symlink maybeGetSymlink( } private Completable downloadFileNoCheckRx( + ActionExecutionMetadata action, DirectoryContext dirCtx, Path path, @Nullable Path treeRoot, @@ -445,6 +452,7 @@ private Completable downloadFileNoCheckRx( toCompletable( () -> doDownloadFile( + action, reporter, tempPath, finalPath.relativeTo(execRoot), @@ -603,7 +611,7 @@ public void finalizeAction(Action action, OutputMetadataStore outputMetadataStor try (var s = Profiler.instance().profile(ProfilerTask.REMOTE_DOWNLOAD, "Download outputs")) { getFromFuture( prefetchFiles( - outputsToDownload, outputMetadataStore::getOutputMetadata, Priority.HIGH)); + action, outputsToDownload, outputMetadataStore::getOutputMetadata, Priority.HIGH)); } } } 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 0dae5c58245144..aeb757d255da8b 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 @@ -25,6 +25,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionInputMap; @@ -81,6 +82,7 @@ public class RemoteActionFileSystem extends DelegateFileSystem { private final RemoteActionInputFetcher inputFetcher; private final RemoteInMemoryFileSystem remoteOutputTree; + @Nullable private ActionExecutionMetadata action = null; @Nullable private MetadataInjector metadataInjector = null; RemoteActionFileSystem( @@ -121,7 +123,8 @@ private boolean isRemote(PathFragment path) { return getRemoteMetadata(path) != null; } - public void updateContext(MetadataInjector metadataInjector) { + public void updateContext(ActionExecutionMetadata action, MetadataInjector metadataInjector) { + this.action = action; this.metadataInjector = metadataInjector; } @@ -625,7 +628,7 @@ private void downloadFileIfRemote(PathFragment path) throws IOException { } getFromFuture( inputFetcher.prefetchFiles( - ImmutableList.of(input), this::getInputMetadata, Priority.CRITICAL)); + action, ImmutableList.of(input), this::getInputMetadata, Priority.CRITICAL)); } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new IOException(String.format("Received interrupt while fetching file '%s'", path), e); 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 2ce36bc8859f12..f560cd60e69bfb 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,18 +14,15 @@ package com.google.devtools.build.lib.remote; import static com.google.common.base.Preconditions.checkArgument; -import static com.google.devtools.build.lib.remote.common.ProgressStatusListener.NO_ACTION; import build.bazel.remote.execution.v2.Digest; import build.bazel.remote.execution.v2.RequestMetadata; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.ListenableFuture; +import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; -import com.google.devtools.build.lib.events.ExtendedEventHandler.FetchProgress; import com.google.devtools.build.lib.events.Reporter; -import com.google.devtools.build.lib.exec.SpawnProgressEvent; -import com.google.devtools.build.lib.remote.RemoteCache.DownloadProgressReporter; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.TempPathGenerator; @@ -79,6 +76,7 @@ protected boolean canDownloadFile(Path path, FileArtifactValue metadata) { @Override protected ListenableFuture doDownloadFile( + ActionExecutionMetadata action, Reporter reporter, Path tempPath, PathFragment execPath, @@ -87,47 +85,18 @@ protected ListenableFuture doDownloadFile( throws IOException { checkArgument(metadata.isRemote(), "Cannot download file that is not a remote file."); RequestMetadata requestMetadata = - TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "prefetcher", null); + TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "prefetcher", action); RemoteActionExecutionContext context = RemoteActionExecutionContext.create(requestMetadata); Digest digest = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize()); - DownloadProgressReporter downloadProgressReporter; - if (priority == Priority.LOW) { - // Only report download progress for toplevel outputs - downloadProgressReporter = - new DownloadProgressReporter( - /* includeFile= */ false, - progress -> reporter.post(new DownloadProgress(progress)), - execPath.toString(), - metadata.getSize()); - } else { - downloadProgressReporter = new DownloadProgressReporter(NO_ACTION, "", 0); - } - - return remoteCache.downloadFile(context, tempPath, digest, downloadProgressReporter); - } - - public static class DownloadProgress implements FetchProgress { - private final SpawnProgressEvent progress; - - public DownloadProgress(SpawnProgressEvent progress) { - this.progress = progress; - } - - @Override - public String getResourceIdentifier() { - return progress.progressId(); - } - - @Override - public String getProgress() { - return progress.progress(); - } - - @Override - public boolean isFinished() { - return progress.finished(); - } + return remoteCache.downloadFile( + context, + tempPath, + digest, + new RemoteCache.DownloadProgressReporter( + progress -> progress.postTo(reporter, action), + execPath.toString(), + digest.getSizeBytes())); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java index 996d1ca65616bc..9fcdb59e356a2d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.eventbus.Subscribe; import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionInputMap; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactPathResolver; @@ -99,11 +100,12 @@ public FileSystem createActionFileSystem( @Override public void updateActionFileSystemContext( + ActionExecutionMetadata action, FileSystem actionFileSystem, Environment env, MetadataInjector injector, ImmutableMap> filesets) { - ((RemoteActionFileSystem) actionFileSystem).updateContext(injector); + ((RemoteActionFileSystem) actionFileSystem).updateContext(action, injector); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 2c5bf381e34519..5d70ff315aad35 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -382,11 +382,13 @@ FileSystem createActionFileSystem( } private void updateActionFileSystemContext( + Action action, FileSystem actionFileSystem, Environment env, MetadataInjector metadataInjector, ImmutableMap> filesets) { - outputService.updateActionFileSystemContext(actionFileSystem, env, metadataInjector, filesets); + outputService.updateActionFileSystemContext( + action, actionFileSystem, env, metadataInjector, filesets); } void executionOver() { @@ -489,7 +491,8 @@ ActionExecutionValue executeAction( boolean hasDiscoveredInputs) throws ActionExecutionException, InterruptedException { if (actionFileSystem != null) { - updateActionFileSystemContext(actionFileSystem, env, metadataHandler, expandedFilesets); + updateActionFileSystemContext( + action, actionFileSystem, env, metadataHandler, expandedFilesets); } ActionExecutionContext actionExecutionContext = @@ -834,6 +837,7 @@ NestedSet discoverInputs( threadStateReceiverFactory.apply(actionLookupData)); if (actionFileSystem != null) { updateActionFileSystemContext( + action, actionFileSystem, env, THROWING_METADATA_INJECTOR_FOR_ACTIONFS, diff --git a/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java b/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java index 840a6aa142b4b8..b432033fbcac20 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionInputMap; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactPathResolver; @@ -196,6 +197,7 @@ default FileSystem createActionFileSystem( * @param filesets The Fileset symlinks known for this action. */ default void updateActionFileSystemContext( + ActionExecutionMetadata action, FileSystem actionFileSystem, Environment env, MetadataInjector injector, 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 9f636e4af0bc68..22aba982d61078 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 @@ -17,6 +17,7 @@ import static com.google.common.base.Throwables.throwIfInstanceOf; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; +import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.NULL_ACTION_OWNER; import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.createTreeArtifactWithGeneratingAction; import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; import static java.nio.charset.StandardCharsets.UTF_8; @@ -24,10 +25,12 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -35,6 +38,7 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; +import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputPrefetcher.MetadataSupplier; import com.google.devtools.build.lib.actions.ActionInputPrefetcher.Priority; @@ -78,8 +82,14 @@ public abstract class ActionInputPrefetcherTestBase { protected ArtifactRoot artifactRoot; protected TempPathGenerator tempPathGenerator; + protected ActionExecutionMetadata action; + @Before public void setUp() throws IOException { + action = mock(ActionExecutionMetadata.class); + when(action.getMnemonic()).thenReturn("DummyAction"); + when(action.getOwner()).thenReturn(NULL_ACTION_OWNER); + fs = SpiedFileSystem.createInMemorySpy(); execRoot = fs.getPath("/exec"); execRoot.createDirectoryAndParents(); @@ -201,9 +211,9 @@ public void prefetchFiles_fileExists_doNotDownload() FileSystemUtils.writeContent(a.getPath(), "hello world".getBytes(UTF_8)); AbstractActionInputPrefetcher prefetcher = spy(createPrefetcher(cas)); - wait(prefetcher.prefetchFiles(metadata.keySet(), metadata::get, Priority.MEDIUM)); + wait(prefetcher.prefetchFiles(action, metadata.keySet(), metadata::get, Priority.MEDIUM)); - verify(prefetcher, never()).doDownloadFile(any(), any(), any(), any(), any()); + verify(prefetcher, never()).doDownloadFile(eq(action), any(), any(), any(), any(), any()); assertThat(prefetcher.downloadedFiles()).containsExactly(a.getPath()); assertThat(prefetcher.downloadsInProgress()).isEmpty(); } @@ -217,9 +227,9 @@ public void prefetchFiles_fileExistsButContentMismatches_download() FileSystemUtils.writeContent(a.getPath(), "hello world local".getBytes(UTF_8)); AbstractActionInputPrefetcher prefetcher = spy(createPrefetcher(cas)); - wait(prefetcher.prefetchFiles(metadata.keySet(), metadata::get, Priority.MEDIUM)); + wait(prefetcher.prefetchFiles(action, metadata.keySet(), metadata::get, Priority.MEDIUM)); - verify(prefetcher).doDownloadFile(any(), any(), eq(a.getExecPath()), any(), any()); + verify(prefetcher).doDownloadFile(eq(action), any(), any(), eq(a.getExecPath()), any(), any()); assertThat(prefetcher.downloadedFiles()).containsExactly(a.getPath()); assertThat(prefetcher.downloadsInProgress()).isEmpty(); assertThat(FileSystemUtils.readContent(a.getPath(), UTF_8)).isEqualTo("hello world remote"); @@ -233,7 +243,7 @@ public void prefetchFiles_downloadRemoteFiles() throws Exception { Artifact a2 = createRemoteArtifact("file2", "fizz buzz", metadata, cas); AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - wait(prefetcher.prefetchFiles(metadata.keySet(), metadata::get, Priority.MEDIUM)); + wait(prefetcher.prefetchFiles(action, metadata.keySet(), metadata::get, Priority.MEDIUM)); assertThat(FileSystemUtils.readContent(a1.getPath(), UTF_8)).isEqualTo("hello world"); assertReadableNonWritableAndExecutable(a1.getPath()); @@ -251,7 +261,7 @@ public void prefetchFiles_downloadRemoteFiles_withMaterializationExecPath() thro Artifact a = createRemoteArtifact("file", "hello world", targetExecPath, metadata, cas); AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - wait(prefetcher.prefetchFiles(metadata.keySet(), metadata::get, Priority.MEDIUM)); + wait(prefetcher.prefetchFiles(action, metadata.keySet(), metadata::get, Priority.MEDIUM)); assertThat(a.getPath().isSymbolicLink()).isTrue(); assertThat(a.getPath().readSymbolicLink()) @@ -281,7 +291,7 @@ public void prefetchFiles_downloadRemoteTrees() throws Exception { AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - wait(prefetcher.prefetchFiles(children, metadata::get, Priority.MEDIUM)); + wait(prefetcher.prefetchFiles(action, children, metadata::get, Priority.MEDIUM)); assertThat(FileSystemUtils.readContent(firstChild.getPath(), UTF_8)).isEqualTo("content1"); assertThat(FileSystemUtils.readContent(secondChild.getPath(), UTF_8)).isEqualTo("content2"); @@ -313,7 +323,7 @@ public void prefetchFiles_downloadRemoteTrees_partial() throws Exception { wait( prefetcher.prefetchFiles( - ImmutableList.of(firstChild, secondChild), metadata::get, Priority.MEDIUM)); + action, ImmutableList.of(firstChild, secondChild), metadata::get, Priority.MEDIUM)); assertThat(firstChild.getPath().exists()).isFalse(); assertThat(FileSystemUtils.readContent(secondChild.getPath(), UTF_8)).isEqualTo("content2"); @@ -342,7 +352,7 @@ public void prefetchFiles_downloadRemoteTrees_withMaterializationExecPath() thro AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - wait(prefetcher.prefetchFiles(children, metadata::get, Priority.MEDIUM)); + wait(prefetcher.prefetchFiles(action, children, metadata::get, Priority.MEDIUM)); assertThat(tree.getPath().isSymbolicLink()).isTrue(); assertThat(tree.getPath().readSymbolicLink()) @@ -368,7 +378,10 @@ public void prefetchFiles_missingFiles_fails() throws Exception { assertThrows( Exception.class, - () -> wait(prefetcher.prefetchFiles(ImmutableList.of(a), metadata::get, Priority.MEDIUM))); + () -> + wait( + prefetcher.prefetchFiles( + action, ImmutableList.of(a), metadata::get, Priority.MEDIUM))); assertThat(prefetcher.downloadedFiles()).isEmpty(); assertThat(prefetcher.downloadsInProgress()).isEmpty(); @@ -385,7 +398,7 @@ public void prefetchFiles_ignoreNonRemoteFiles() throws Exception { ImmutableMap metadata = ImmutableMap.of(a, f); AbstractActionInputPrefetcher prefetcher = createPrefetcher(new HashMap<>()); - wait(prefetcher.prefetchFiles(ImmutableList.of(a), metadata::get, Priority.MEDIUM)); + wait(prefetcher.prefetchFiles(action, ImmutableList.of(a), metadata::get, Priority.MEDIUM)); assertThat(prefetcher.downloadedFiles()).isEmpty(); assertThat(prefetcher.downloadsInProgress()).isEmpty(); @@ -411,7 +424,7 @@ public void prefetchFiles_ignoreNonRemoteFiles_tree() throws Exception { AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - wait(prefetcher.prefetchFiles(children, metadata::get, Priority.MEDIUM)); + wait(prefetcher.prefetchFiles(action, children, metadata::get, Priority.MEDIUM)); assertThat(firstChild.getPath().exists()).isFalse(); assertThat(FileSystemUtils.readContent(secondChild.getPath(), UTF_8)).isEqualTo("content2"); @@ -439,7 +452,7 @@ public void prefetchFiles_treeFiles_minimizeFilesystemOperations() throws Except wait( prefetcher.prefetchFiles( - ImmutableList.of(firstChild, secondChild), metadata::get, Priority.MEDIUM)); + action, ImmutableList.of(firstChild, secondChild), metadata::get, Priority.MEDIUM)); verify(fs, times(1)).createWritableDirectory(tree.getPath().asFragment()); verify(fs, times(1)).createWritableDirectory(tree.getPath().getChild("subdir").asFragment()); @@ -464,7 +477,7 @@ public void prefetchFiles_multipleThreads_downloadIsCancelled() throws Exception try { wait( prefetcher.prefetchFiles( - ImmutableList.of(artifact), metadata::get, Priority.MEDIUM)); + action, ImmutableList.of(artifact), metadata::get, Priority.MEDIUM)); } catch (IOException | ExecException | InterruptedException ignored) { // do nothing } @@ -476,7 +489,7 @@ public void prefetchFiles_multipleThreads_downloadIsCancelled() throws Exception try { wait( prefetcher.prefetchFiles( - ImmutableList.of(artifact), metadata::get, Priority.MEDIUM)); + action, ImmutableList.of(artifact), metadata::get, Priority.MEDIUM)); } catch (IOException | ExecException | InterruptedException ignored) { // do nothing } @@ -514,7 +527,7 @@ public void prefetchFiles_multipleThreads_downloadIsNotCancelledByOtherThreads() try { wait( prefetcher.prefetchFiles( - ImmutableList.of(artifact), metadata::get, Priority.MEDIUM)); + action, ImmutableList.of(artifact), metadata::get, Priority.MEDIUM)); } catch (IOException | ExecException | InterruptedException ignored) { // do nothing } @@ -527,7 +540,7 @@ public void prefetchFiles_multipleThreads_downloadIsNotCancelledByOtherThreads() try { wait( prefetcher.prefetchFiles( - ImmutableList.of(artifact), metadata::get, Priority.MEDIUM)); + action, ImmutableList.of(artifact), metadata::get, Priority.MEDIUM)); successful.set(true); } catch (IOException | ExecException | InterruptedException ignored) { // do nothing @@ -571,7 +584,7 @@ public void prefetchFile_interruptingMetadataSupplier_interruptsDownload() throw ListenableFuture future = prefetcher.prefetchFiles( - ImmutableList.of(a1), interruptedMetadataSupplier, Priority.MEDIUM); + action, ImmutableList.of(a1), interruptedMetadataSupplier, Priority.MEDIUM); assertThrows(CancellationException.class, future::get); } @@ -598,7 +611,8 @@ public void prefetchFiles_onInterrupt_deletePartialDownloadedFile() throws Excep () -> { try { getFromFuture( - prefetcher.prefetchFiles(ImmutableList.of(a1), metadata::get, Priority.MEDIUM)); + prefetcher.prefetchFiles( + action, ImmutableList.of(a1), metadata::get, Priority.MEDIUM)); } catch (IOException ignored) { // Intentionally left empty } catch (InterruptedException e) { @@ -625,7 +639,10 @@ public void missingInputs_addedToList() { assertThrows( Exception.class, - () -> wait(prefetcher.prefetchFiles(metadata.keySet(), metadata::get, Priority.MEDIUM))); + () -> + wait( + prefetcher.prefetchFiles( + action, metadata.keySet(), metadata::get, Priority.MEDIUM))); assertThat(prefetcher.getMissingActionInputs()).contains(a); } @@ -656,8 +673,8 @@ protected static void mockDownload( throws IOException { doAnswer( invocation -> { - Path path = invocation.getArgument(1); - FileArtifactValue metadata = invocation.getArgument(3); + Path path = invocation.getArgument(2); + FileArtifactValue metadata = invocation.getArgument(4); byte[] content = cas.get(HashCode.fromBytes(metadata.getDigest())); if (content == null) { return Futures.immediateFailedFuture(new IOException("Not found")); @@ -666,7 +683,7 @@ protected static void mockDownload( return resultSupplier.get(); }) .when(prefetcher) - .doDownloadFile(any(), any(), any(), any(), any()); + .doDownloadFile(any(), any(), any(), any(), any(), any()); } private void assertReadableNonWritableAndExecutable(Path path) throws IOException { 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 b1d011f15382b6..70e3c9b16cd962 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 @@ -31,6 +31,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.hash.HashCode; import com.google.common.util.concurrent.ListenableFuture; +import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionInputMap; @@ -105,7 +106,7 @@ protected RemoteActionFileSystem createActionFileSystem( outputs, fileCache, inputFetcher); - remoteActionFileSystem.updateContext(metadataInjector); + remoteActionFileSystem.updateContext(mock(ActionExecutionMetadata.class), metadataInjector); remoteActionFileSystem.createDirectoryAndParents(outputRoot.getRoot().asPath().asFragment()); return remoteActionFileSystem; } @@ -156,7 +157,7 @@ public void testGetInputStream_fromInputArtifactData_forRemoteArtifact() throws FileSystem actionFs = createActionFileSystem(inputs); doAnswer(mockPrefetchFile(artifact.getPath(), "remote contents")) .when(inputFetcher) - .prefetchFiles(eq(ImmutableList.of(artifact)), any(), eq(Priority.CRITICAL)); + .prefetchFiles(any(), eq(ImmutableList.of(artifact)), any(), eq(Priority.CRITICAL)); // act Path actionFsPath = actionFs.getPath(artifact.getPath().asFragment()); @@ -166,7 +167,7 @@ public void testGetInputStream_fromInputArtifactData_forRemoteArtifact() throws assertThat(actionFsPath.getFileSystem()).isSameInstanceAs(actionFs); assertThat(contents).isEqualTo("remote contents"); verify(inputFetcher) - .prefetchFiles(eq(ImmutableList.of(artifact)), any(), eq(Priority.CRITICAL)); + .prefetchFiles(any(), eq(ImmutableList.of(artifact)), any(), eq(Priority.CRITICAL)); verifyNoMoreInteractions(inputFetcher); } @@ -178,7 +179,7 @@ public void testGetInputStream_fromRemoteOutputTree_forDeclaredOutput() throws E injectRemoteFile(actionFs, artifact.getPath().asFragment(), "remote contents"); doAnswer(mockPrefetchFile(artifact.getPath(), "remote contents")) .when(inputFetcher) - .prefetchFiles(eq(ImmutableList.of(artifact)), any(), eq(Priority.CRITICAL)); + .prefetchFiles(any(), eq(ImmutableList.of(artifact)), any(), eq(Priority.CRITICAL)); // act Path actionFsPath = actionFs.getPath(artifact.getPath().asFragment()); @@ -188,7 +189,7 @@ public void testGetInputStream_fromRemoteOutputTree_forDeclaredOutput() throws E assertThat(actionFsPath.getFileSystem()).isSameInstanceAs(actionFs); assertThat(contents).isEqualTo("remote contents"); verify(inputFetcher) - .prefetchFiles(eq(ImmutableList.of(artifact)), any(), eq(Priority.CRITICAL)); + .prefetchFiles(any(), eq(ImmutableList.of(artifact)), any(), eq(Priority.CRITICAL)); verifyNoMoreInteractions(inputFetcher); } @@ -201,7 +202,7 @@ public void testGetInputStream_fromRemoteOutputTree_forUndeclaredOutput() throws injectRemoteFile(actionFs, path.asFragment(), "remote contents"); doAnswer(mockPrefetchFile(path, "remote contents")) .when(inputFetcher) - .prefetchFiles(eq(ImmutableList.of(input)), any(), eq(Priority.CRITICAL)); + .prefetchFiles(any(), eq(ImmutableList.of(input)), any(), eq(Priority.CRITICAL)); // act Path actionFsPath = actionFs.getPath(path.asFragment()); @@ -210,7 +211,8 @@ public void testGetInputStream_fromRemoteOutputTree_forUndeclaredOutput() throws // assert assertThat(actionFsPath.getFileSystem()).isSameInstanceAs(actionFs); assertThat(contents).isEqualTo("remote contents"); - verify(inputFetcher).prefetchFiles(eq(ImmutableList.of(input)), any(), eq(Priority.CRITICAL)); + verify(inputFetcher) + .prefetchFiles(any(), eq(ImmutableList.of(input)), any(), eq(Priority.CRITICAL)); verifyNoMoreInteractions(inputFetcher); } 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 1d0d2cefdc5012..1a6b4fa5e7348a 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 @@ -100,7 +100,7 @@ public void testStagingVirtualActionInput() throws Exception { // act wait( actionInputFetcher.prefetchFiles( - ImmutableList.of(a), (ActionInput unused) -> null, Priority.MEDIUM)); + action, ImmutableList.of(a), (ActionInput unused) -> null, Priority.MEDIUM)); // assert Path p = execRoot.getRelative(a.getExecPath()); @@ -128,6 +128,7 @@ public void testStagingEmptyVirtualActionInput() throws Exception { // act wait( actionInputFetcher.prefetchFiles( + action, ImmutableList.of(VirtualActionInput.EMPTY_MARKER), (ActionInput unused) -> null, Priority.MEDIUM)); @@ -148,7 +149,8 @@ public void prefetchFiles_missingFiles_failsWithSpecificMessage() throws Excepti BulkTransferException.class, () -> wait( - prefetcher.prefetchFiles(ImmutableList.of(a), metadata::get, Priority.MEDIUM))); + prefetcher.prefetchFiles( + action, ImmutableList.of(a), metadata::get, Priority.MEDIUM))); assertThat(prefetcher.downloadedFiles()).isEmpty(); assertThat(prefetcher.downloadsInProgress()).isEmpty();