From 4d56751460f4665bd6471655b8a2d094fb602b9d 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 | 8 +- .../build/lib/exec/AbstractSpawnStrategy.java | 1 + .../remote/AbstractActionInputPrefetcher.java | 27 ++++-- .../google/devtools/build/lib/remote/BUILD | 3 +- .../lib/remote/RemoteActionFileSystem.java | 7 +- .../lib/remote/RemoteActionInputFetcher.java | 4 +- .../build/lib/remote/RemoteOutputService.java | 4 +- .../remote/ToplevelArtifactsDownloader.java | 97 +++++++++++++------ .../lib/skyframe/SkyframeActionExecutor.java | 8 +- .../devtools/build/lib/vfs/OutputService.java | 2 + .../remote/ActionInputPrefetcherTestBase.java | 66 ++++++++----- .../remote/RemoteActionFileSystemTest.java | 9 +- .../remote/RemoteActionInputFetcherTest.java | 6 +- 13 files changed, 169 insertions(+), 73 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 fb2b07c9ae4cde..cd43b4431402d7 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 @@ -24,7 +24,9 @@ public interface ActionInputPrefetcher { new ActionInputPrefetcher() { @Override public ListenableFuture prefetchFiles( - Iterable inputs, MetadataProvider metadataProvider) { + ActionExecutionMetadata action, + Iterable inputs, + MetadataProvider metadataProvider) { // Do nothing. return immediateVoidFuture(); } @@ -43,7 +45,9 @@ public boolean supportsPartialTreeArtifactInputs() { * @return future success if prefetch is finished or {@link IOException}. */ ListenableFuture prefetchFiles( - Iterable inputs, MetadataProvider metadataProvider); + ActionExecutionMetadata action, + Iterable inputs, + MetadataProvider metadataProvider); /** * Whether the prefetcher is able to fetch individual files in a tree artifact without fetching 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 01ea75faba166f..84b12d11efce26 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 @@ -246,6 +246,7 @@ public ListenableFuture prefetchInputs() return actionExecutionContext .getActionInputPrefetcher() .prefetchFiles( + spawn.getResourceOwner(), getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true) .values(), getMetadataProvider()); 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 3f57b1b5760ded..a037811a8e70e2 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 @@ -33,6 +33,7 @@ import com.google.common.util.concurrent.FutureCallback; 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; @@ -292,6 +293,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, @@ -317,11 +319,13 @@ protected Completable onErrorResumeNext(Throwable error) { */ @Override public ListenableFuture prefetchFiles( + ActionExecutionMetadata action, Iterable inputs, MetadataProvider metadataProvider) { - return prefetchFiles(inputs, metadataProvider, Priority.MEDIUM); + return prefetchFiles(action, inputs, metadataProvider, Priority.MEDIUM); } protected ListenableFuture prefetchFiles( + ActionExecutionMetadata action, Iterable inputs, MetadataProvider metadataProvider, Priority priority) { @@ -346,7 +350,7 @@ protected ListenableFuture prefetchFiles( Flowable transfers = Flowable.fromIterable(files) .flatMapSingle( - input -> toTransferResult(prefetchFile(dirCtx, metadataProvider, input, priority))); + input -> toTransferResult(prefetchFile(action, dirCtx, metadataProvider, input, priority))); Completable prefetch = Completable.using( @@ -357,6 +361,7 @@ protected ListenableFuture prefetchFiles( } private Completable prefetchFile( + ActionExecutionMetadata action, DirectoryContext dirCtx, MetadataProvider metadataProvider, ActionInput input, @@ -386,6 +391,7 @@ private Completable prefetchFile( Completable result = downloadFileNoCheckRx( + action, dirCtx, execRoot.getRelative(execPath), treeRootExecPath != null ? execRoot.getRelative(treeRootExecPath) : null, @@ -461,6 +467,7 @@ private Symlink maybeGetSymlink( * download finished. */ private Completable downloadFileRx( + ActionExecutionMetadata action, DirectoryContext dirCtx, Path path, @Nullable Path treeRoot, @@ -470,10 +477,11 @@ private Completable downloadFileRx( if (!canDownloadFile(path, metadata)) { return Completable.complete(); } - return downloadFileNoCheckRx(dirCtx, path, treeRoot, actionInput, metadata, priority); + return downloadFileNoCheckRx(action, dirCtx, path, treeRoot, actionInput, metadata, priority); } private Completable downloadFileNoCheckRx( + ActionExecutionMetadata action, DirectoryContext dirCtx, Path path, @Nullable Path treeRoot, @@ -498,6 +506,7 @@ private Completable downloadFileNoCheckRx( toCompletable( () -> doDownloadFile( + action, reporter, tempPath, finalPath.relativeTo(execRoot), @@ -542,12 +551,15 @@ private Completable downloadFileNoCheckRx( *

The file will be written into a temporary file and moved to the final destination after the * download finished. */ - public void downloadFile(Path path, @Nullable ActionInput actionInput, FileArtifactValue metadata) + public void downloadFile( + ActionExecutionMetadata action, + Path path, @Nullable ActionInput actionInput, FileArtifactValue metadata) throws IOException, InterruptedException { - getFromFuture(downloadFileAsync(path.asFragment(), actionInput, metadata, Priority.CRITICAL)); + getFromFuture(downloadFileAsync(action, path.asFragment(), actionInput, metadata, Priority.CRITICAL)); } protected ListenableFuture downloadFileAsync( + ActionExecutionMetadata action, PathFragment path, @Nullable ActionInput actionInput, FileArtifactValue metadata, @@ -557,6 +569,7 @@ protected ListenableFuture downloadFileAsync( DirectoryContext::new, dirCtx -> downloadFileRx( + action, dirCtx, execRoot.getFileSystem().getPath(path), /* treeRoot= */ null, @@ -695,7 +708,7 @@ public void finalizeAction(Action action, MetadataHandler metadataHandler) { } if (!inputsToDownload.isEmpty()) { - var future = prefetchFiles(inputsToDownload, metadataHandler, Priority.HIGH); + var future = prefetchFiles(action, inputsToDownload, metadataHandler, Priority.HIGH); addCallback( future, new FutureCallback() { @@ -716,7 +729,7 @@ public void onFailure(Throwable throwable) { } if (!outputsToDownload.isEmpty()) { - var future = prefetchFiles(outputsToDownload, metadataHandler, Priority.LOW); + var future = prefetchFiles(action, outputsToDownload, metadataHandler, Priority.LOW); addCallback( future, new FutureCallback() { diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index 0f2e76ed83aecf..4dd5629f1b1a49 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -9,8 +9,8 @@ filegroup( srcs = glob(["*"]) + [ "//src/main/java/com/google/devtools/build/lib/remote/circuitbreaker:srcs", "//src/main/java/com/google/devtools/build/lib/remote/common:srcs", - "//src/main/java/com/google/devtools/build/lib/remote/downloader:srcs", "//src/main/java/com/google/devtools/build/lib/remote/disk:srcs", + "//src/main/java/com/google/devtools/build/lib/remote/downloader:srcs", "//src/main/java/com/google/devtools/build/lib/remote/grpc:srcs", "//src/main/java/com/google/devtools/build/lib/remote/http:srcs", "//src/main/java/com/google/devtools/build/lib/remote/logging:srcs", @@ -206,6 +206,7 @@ java_library( srcs = ["ToplevelArtifactsDownloader.java"], deps = [ ":abstract_action_input_prefetcher", + "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java index dfd21d7d79e602..de9736a995d858 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 @@ -24,6 +24,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.ActionInputMap; import com.google.devtools.build.lib.actions.Artifact; @@ -77,6 +78,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( @@ -111,7 +113,8 @@ boolean isRemote(Path path) { return getRemoteMetadata(path.asFragment()) != null; } - public void updateContext(MetadataInjector metadataInjector) { + public void updateContext(ActionExecutionMetadata action, MetadataInjector metadataInjector) { + this.action = action; this.metadataInjector = metadataInjector; } @@ -579,7 +582,7 @@ private void downloadFileIfRemote(PathFragment path) throws IOException { FileArtifactValue m = getRemoteMetadata(path); if (m != null) { try { - inputFetcher.downloadFile(delegateFs.getPath(path), getActionInput(path), m); + inputFetcher.downloadFile(action, delegateFs.getPath(path), getActionInput(path), m); } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new IOException( 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 e13606c05cd3e1..04d6fb042cadf0 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 @@ -21,6 +21,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.util.concurrent.ListenableFuture; +import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; @@ -96,6 +97,7 @@ protected boolean canDownloadFile(Path path, FileArtifactValue metadata) { @Override protected ListenableFuture doDownloadFile( + ActionExecutionMetadata action, Reporter reporter, Path tempPath, PathFragment execPath, @@ -104,7 +106,7 @@ 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()); 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 2ea7a0f2aa9219..7cf03eb399862f 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 @@ -20,6 +20,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; @@ -84,11 +85,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/remote/ToplevelArtifactsDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java index c266070a881374..75ad886bb3d5f2 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java @@ -25,8 +25,11 @@ import com.google.common.flogger.GoogleLogger; import com.google.common.util.concurrent.FutureCallback; 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.ActionLookupValue; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.analysis.AspectCompleteEvent; import com.google.devtools.build.lib.analysis.ConfiguredTarget; @@ -46,6 +49,7 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.skyframe.MemoizingEvaluator; import com.google.devtools.build.skyframe.SkyValue; +import java.util.HashMap; import javax.annotation.Nullable; /** @@ -122,10 +126,17 @@ private void downloadTestOutput(Path path) { // because test outputs are already downloaded (otherwise it cannot hit the action cache). FileArtifactValue metadata = pathToMetadataConverter.getMetadata(path); ActionInput actionInput = pathToMetadataConverter.getActionInput(path); + ActionExecutionMetadata action; + try { + action = getAction(actionInput); + } catch (InterruptedException ignored) { + Thread.currentThread().interrupt(); + action = null; + } if (metadata != null) { ListenableFuture future = actionInputPrefetcher.downloadFileAsync( - path.asFragment(), actionInput, metadata, Priority.LOW); + action, path.asFragment(), actionInput, metadata, Priority.LOW); addCallback( future, new FutureCallback() { @@ -222,7 +233,8 @@ private boolean shouldDownloadToplevelOutputs(ConfiguredTargetKey configuredTarg private void downloadTargetOutputs( ImmutableMap outputGroups, @Nullable Runfiles runfiles) { - var builder = ImmutableMap.builder(); + HashMap> builder = + new HashMap<>(); try { for (ArtifactsInOutputGroup outputs : outputGroups.values()) { if (!outputs.areImportant()) { @@ -239,27 +251,31 @@ private void downloadTargetOutputs( return; } - var outputsAndMetadata = builder.buildKeepingLast(); - ListenableFuture future = - actionInputPrefetcher.prefetchFiles( - outputsAndMetadata.keySet().stream() - .filter(ToplevelArtifactsDownloader::isNonTreeArtifact) - .collect(toImmutableSet()), - new StaticMetadataProvider(outputsAndMetadata), - Priority.LOW); - - addCallback( - future, - new FutureCallback() { - @Override - public void onSuccess(Void unused) {} - - @Override - public void onFailure(Throwable throwable) { - logger.atWarning().withCause(throwable).log("Failed to download toplevel artifacts."); - } - }, - directExecutor()); + for (var entry : builder.entrySet()) { + var action = entry.getKey(); + var outputsAndMetadata = entry.getValue(); + ListenableFuture future = + actionInputPrefetcher.prefetchFiles( + action, + outputsAndMetadata.keySet().stream() + .filter(ToplevelArtifactsDownloader::isNonTreeArtifact) + .collect(toImmutableSet()), + new StaticMetadataProvider(outputsAndMetadata), + Priority.LOW); + + addCallback( + future, + new FutureCallback() { + @Override + public void onSuccess(Void unused) {} + + @Override + public void onFailure(Throwable throwable) { + logger.atWarning().withCause(throwable).log("Failed to download toplevel artifacts."); + } + }, + directExecutor()); + } } private static boolean isNonTreeArtifact(ActionInput actionInput) { @@ -267,7 +283,8 @@ private static boolean isNonTreeArtifact(ActionInput actionInput) { } private void appendRunfiles( - @Nullable Runfiles runfiles, ImmutableMap.Builder builder) + @Nullable Runfiles runfiles, + HashMap> builder) throws InterruptedException { if (runfiles == null) { return; @@ -279,17 +296,41 @@ private void appendRunfiles( } private void appendArtifact( - Artifact artifact, ImmutableMap.Builder builder) + Artifact artifact, + HashMap> builder) throws InterruptedException { + var action = getAction(artifact); + if (action == null) { + return; + } + + var builderForAction = builder.computeIfAbsent(action, unused -> new HashMap<>()); + SkyValue value = memoizingEvaluator.getExistingValue(Artifact.key(artifact)); if (value instanceof ActionExecutionValue) { FileArtifactValue metadata = ((ActionExecutionValue) value).getAllFileValues().get(artifact); if (metadata != null) { - builder.put(artifact, metadata); + builderForAction.put(artifact, metadata); } } else if (value instanceof TreeArtifactValue) { - builder.put(artifact, ((TreeArtifactValue) value).getMetadata()); - builder.putAll(((TreeArtifactValue) value).getChildValues()); + builderForAction.put(artifact, ((TreeArtifactValue) value).getMetadata()); + builderForAction.putAll(((TreeArtifactValue) value).getChildValues()); } } + + @Nullable + private ActionExecutionMetadata getAction(ActionInput artifact) throws InterruptedException { + if (!(artifact instanceof DerivedArtifact)) { + return null; + } + var actionLookupData = ((DerivedArtifact) artifact).getGeneratingActionKey(); + var actionLookupValue = + (ActionLookupValue) + memoizingEvaluator.getExistingValue(actionLookupData.getActionLookupKey()); + if (actionLookupValue == null) { + return null; + } + + return actionLookupValue.getAction(actionLookupData.getActionIndex()); + } } 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 08e941c591142b..7c40c8dff0a32b 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 @@ -358,11 +358,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() { @@ -465,7 +467,8 @@ ActionExecutionValue executeAction( boolean hasDiscoveredInputs) throws ActionExecutionException, InterruptedException { if (actionFileSystem != null) { - updateActionFileSystemContext(actionFileSystem, env, metadataHandler, expandedFilesets); + updateActionFileSystemContext( + action, actionFileSystem, env, metadataHandler, expandedFilesets); } ActionExecutionContext actionExecutionContext = @@ -813,6 +816,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 53517ef2d16434..45ce3d779ea40f 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; @@ -199,6 +200,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 c4528bd348a5a0..de4da2d17718a7 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,16 +17,19 @@ 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 java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertThrows; 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; @@ -34,6 +37,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.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; @@ -76,8 +80,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(); @@ -196,9 +206,9 @@ public void prefetchFiles_fileExists_doNotDownload() MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); AbstractActionInputPrefetcher prefetcher = spy(createPrefetcher(cas)); - wait(prefetcher.prefetchFiles(metadata.keySet(), metadataProvider)); + wait(prefetcher.prefetchFiles(action, metadata.keySet(), metadataProvider)); - 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(); } @@ -213,9 +223,9 @@ public void prefetchFiles_fileExistsButContentMismatches_download() MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); AbstractActionInputPrefetcher prefetcher = spy(createPrefetcher(cas)); - wait(prefetcher.prefetchFiles(metadata.keySet(), metadataProvider)); + wait(prefetcher.prefetchFiles(action, metadata.keySet(), metadataProvider)); - 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"); @@ -230,7 +240,7 @@ public void prefetchFiles_downloadRemoteFiles() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - wait(prefetcher.prefetchFiles(metadata.keySet(), metadataProvider)); + wait(prefetcher.prefetchFiles(action, metadata.keySet(), metadataProvider)); assertThat(FileSystemUtils.readContent(a1.getPath(), UTF_8)).isEqualTo("hello world"); assertReadableNonWritableAndExecutable(a1.getPath()); @@ -249,7 +259,7 @@ public void prefetchFiles_downloadRemoteFiles_withMaterializationExecPath() thro MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - wait(prefetcher.prefetchFiles(metadata.keySet(), metadataProvider)); + wait(prefetcher.prefetchFiles(action, metadata.keySet(), metadataProvider)); assertThat(a.getPath().isSymbolicLink()).isTrue(); assertThat(a.getPath().readSymbolicLink()) @@ -280,7 +290,7 @@ public void prefetchFiles_downloadRemoteTrees() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - wait(prefetcher.prefetchFiles(children, metadataProvider)); + wait(prefetcher.prefetchFiles(action, children, metadataProvider)); assertThat(FileSystemUtils.readContent(firstChild.getPath(), UTF_8)).isEqualTo("content1"); assertThat(FileSystemUtils.readContent(secondChild.getPath(), UTF_8)).isEqualTo("content2"); @@ -311,7 +321,9 @@ public void prefetchFiles_downloadRemoteTrees_partial() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - wait(prefetcher.prefetchFiles(ImmutableList.of(firstChild, secondChild), metadataProvider)); + wait( + prefetcher.prefetchFiles( + action, ImmutableList.of(firstChild, secondChild), metadataProvider)); assertThat(firstChild.getPath().exists()).isFalse(); assertThat(FileSystemUtils.readContent(secondChild.getPath(), UTF_8)).isEqualTo("content2"); @@ -341,7 +353,7 @@ public void prefetchFiles_downloadRemoteTrees_withMaterializationExecPath() thro MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - wait(prefetcher.prefetchFiles(children, metadataProvider)); + wait(prefetcher.prefetchFiles(action, children, metadataProvider)); assertThat(tree.getPath().isSymbolicLink()).isTrue(); assertThat(tree.getPath().readSymbolicLink()) @@ -368,7 +380,7 @@ public void prefetchFiles_missingFiles_fails() throws Exception { assertThrows( Exception.class, - () -> wait(prefetcher.prefetchFiles(ImmutableList.of(a), metadataProvider))); + () -> wait(prefetcher.prefetchFiles(action, ImmutableList.of(a), metadataProvider))); assertThat(prefetcher.downloadedFiles()).isEmpty(); assertThat(prefetcher.downloadsInProgress()).isEmpty(); @@ -385,7 +397,7 @@ public void prefetchFiles_ignoreNonRemoteFiles() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(ImmutableMap.of(a, f)); AbstractActionInputPrefetcher prefetcher = createPrefetcher(new HashMap<>()); - wait(prefetcher.prefetchFiles(ImmutableList.of(a), metadataProvider)); + wait(prefetcher.prefetchFiles(action, ImmutableList.of(a), metadataProvider)); assertThat(prefetcher.downloadedFiles()).isEmpty(); assertThat(prefetcher.downloadsInProgress()).isEmpty(); @@ -412,7 +424,7 @@ public void prefetchFiles_ignoreNonRemoteFiles_tree() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - wait(prefetcher.prefetchFiles(children, metadataProvider)); + wait(prefetcher.prefetchFiles(action, children, metadataProvider)); assertThat(firstChild.getPath().exists()).isFalse(); assertThat(FileSystemUtils.readContent(secondChild.getPath(), UTF_8)).isEqualTo("content2"); @@ -439,7 +451,9 @@ public void prefetchFiles_treeFiles_minimizeFilesystemOperations() throws Except MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - wait(prefetcher.prefetchFiles(ImmutableList.of(firstChild, secondChild), metadataProvider)); + wait( + prefetcher.prefetchFiles( + action, ImmutableList.of(firstChild, secondChild), metadataProvider)); verify(fs, times(1)).createWritableDirectory(tree.getPath().asFragment()); verify(fs, times(1)).createWritableDirectory(tree.getPath().getChild("subdir").asFragment()); @@ -463,7 +477,8 @@ public void prefetchFiles_multipleThreads_downloadIsCancelled() throws Exception new Thread( () -> { try { - wait(prefetcher.prefetchFiles(ImmutableList.of(artifact), metadataProvider)); + wait( + prefetcher.prefetchFiles(action, ImmutableList.of(artifact), metadataProvider)); } catch (IOException | ExecException | InterruptedException ignored) { // do nothing } @@ -473,7 +488,8 @@ public void prefetchFiles_multipleThreads_downloadIsCancelled() throws Exception new Thread( () -> { try { - wait(prefetcher.prefetchFiles(ImmutableList.of(artifact), metadataProvider)); + wait( + prefetcher.prefetchFiles(action, ImmutableList.of(artifact), metadataProvider)); } catch (IOException | ExecException | InterruptedException ignored) { // do nothing } @@ -510,7 +526,8 @@ public void prefetchFiles_multipleThreads_downloadIsNotCancelledByOtherThreads() new Thread( () -> { try { - wait(prefetcher.prefetchFiles(ImmutableList.of(artifact), metadataProvider)); + wait( + prefetcher.prefetchFiles(action, ImmutableList.of(artifact), metadataProvider)); } catch (IOException | ExecException | InterruptedException ignored) { // do nothing } @@ -521,7 +538,8 @@ public void prefetchFiles_multipleThreads_downloadIsNotCancelledByOtherThreads() new Thread( () -> { try { - wait(prefetcher.prefetchFiles(ImmutableList.of(artifact), metadataProvider)); + wait( + prefetcher.prefetchFiles(action, ImmutableList.of(artifact), metadataProvider)); successful.set(true); } catch (IOException | ExecException | InterruptedException ignored) { // do nothing @@ -558,7 +576,7 @@ public void downloadFile_downloadRemoteFiles() throws Exception { Artifact a1 = createRemoteArtifact("file1", "hello world", metadata, cas); AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - prefetcher.downloadFile(a1.getPath(), /* actionInput= */ null, metadata.get(a1)); + prefetcher.downloadFile(action, a1.getPath(), /* actionInput= */ null, metadata.get(a1)); assertThat(FileSystemUtils.readContent(a1.getPath(), UTF_8)).isEqualTo("hello world"); assertThat(a1.getPath().isExecutable()).isTrue(); @@ -587,7 +605,8 @@ public void downloadFile_onInterrupt_deletePartialDownloadedFile() throws Except new Thread( () -> { try { - prefetcher.downloadFile(a1.getPath(), /* actionInput= */ null, metadata.get(a1)); + prefetcher.downloadFile( + action, a1.getPath(), /* actionInput= */ null, metadata.get(a1)); } catch (IOException ignored) { // Intentionally left empty } catch (InterruptedException e) { @@ -614,7 +633,8 @@ public void missingInputs_addedToList() { AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); assertThrows( - Exception.class, () -> wait(prefetcher.prefetchFiles(metadata.keySet(), metadataProvider))); + Exception.class, + () -> wait(prefetcher.prefetchFiles(action, metadata.keySet(), metadataProvider))); assertThat(prefetcher.getMissingActionInputs()).contains(a); } @@ -645,8 +665,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")); @@ -655,7 +675,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 9b33e0778a2a3d..1129fef2f11932 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 @@ -27,6 +27,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.hash.HashCode; import com.google.common.util.concurrent.Futures; +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.Artifact.SpecialArtifact; @@ -84,7 +85,7 @@ protected RemoteActionFileSystem createActionFileSystem( inputs, outputs, inputFetcher); - remoteActionFileSystem.updateContext(metadataInjector); + remoteActionFileSystem.updateContext(mock(ActionExecutionMetadata.class), metadataInjector); remoteActionFileSystem.createDirectoryAndParents(outputRoot.getRoot().asPath().asFragment()); return remoteActionFileSystem; } @@ -118,7 +119,8 @@ public void testGetInputStream() throws Exception { return Futures.immediateFuture(null); }) .when(inputFetcher) - .downloadFile(eq(remoteArtifact.getPath()), any(), eq(inputs.getMetadata(remoteArtifact))); + .downloadFile( + any(), eq(remoteArtifact.getPath()), any(), eq(inputs.getMetadata(remoteArtifact))); // act Path remoteActionFsPath = actionFs.getPath(remoteArtifact.getPath().asFragment()); @@ -133,7 +135,8 @@ public void testGetInputStream() throws Exception { assertThat(actualRemoteContents).isEqualTo("remote contents"); assertThat(actualLocalContents).isEqualTo("local contents"); verify(inputFetcher) - .downloadFile(eq(remoteArtifact.getPath()), any(), eq(inputs.getMetadata(remoteArtifact))); + .downloadFile( + any(), eq(remoteArtifact.getPath()), any(), eq(inputs.getMetadata(remoteArtifact))); 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 fca68db8b6f160..fcb1485ede0b69 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 @@ -98,7 +98,7 @@ public void testStagingVirtualActionInput() throws Exception { VirtualActionInput a = ActionsTestUtil.createVirtualActionInput("file1", "hello world"); // act - wait(actionInputFetcher.prefetchFiles(ImmutableList.of(a), metadataProvider)); + wait(actionInputFetcher.prefetchFiles(action, ImmutableList.of(a), metadataProvider)); // assert Path p = execRoot.getRelative(a.getExecPath()); @@ -128,7 +128,7 @@ public void testStagingEmptyVirtualActionInput() throws Exception { // act wait( actionInputFetcher.prefetchFiles( - ImmutableList.of(VirtualActionInput.EMPTY_MARKER), metadataProvider)); + action, ImmutableList.of(VirtualActionInput.EMPTY_MARKER), metadataProvider)); // assert that nothing happened assertThat(actionInputFetcher.downloadedFiles()).isEmpty(); @@ -145,7 +145,7 @@ public void prefetchFiles_missingFiles_failsWithSpecificMessage() throws Excepti var error = assertThrows( ExecException.class, - () -> wait(prefetcher.prefetchFiles(ImmutableList.of(a), metadataProvider))); + () -> wait(prefetcher.prefetchFiles(action, ImmutableList.of(a), metadataProvider))); assertThat(prefetcher.downloadedFiles()).isEmpty(); assertThat(prefetcher.downloadsInProgress()).isEmpty();