From 895f3077f0b720584891e7fe68e46bfe72ae2080 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 15 Mar 2023 07:13:15 -0700 Subject: [PATCH] Delete downloadFile in favor of prefetchFiles in AbstractActionInputPrefetcher. This simplifies the interface and makes it less likely that we get the subtle synchronization logic wrong in the future. Due to these changes, AbstractActionInputPrefetcher is now too low-level to handle the presentation of a BulkTransferException to the user, so the logic to convert it into an EnvironmentalExecException is lifted into AbstractSpawnStrategy. PiperOrigin-RevId: 516816172 Change-Id: I6e4b57fb90d9c84821dee36ae29c92cee02a906f --- .../build/lib/exec/AbstractSpawnStrategy.java | 39 +++++++++--- .../com/google/devtools/build/lib/exec/BUILD | 1 + .../remote/AbstractActionInputPrefetcher.java | 63 +------------------ .../google/devtools/build/lib/remote/BUILD | 2 + .../lib/remote/RemoteActionFileSystem.java | 31 +++++---- .../lib/remote/RemoteActionInputFetcher.java | 32 +--------- .../build/lib/remote/RemoteModule.java | 35 +++-------- .../remote/ToplevelArtifactsDownloader.java | 48 +++++++------- .../remote/ActionInputPrefetcherTestBase.java | 24 +++---- .../remote/RemoteActionFileSystemTest.java | 36 ++++++----- .../remote/RemoteActionInputFetcherTest.java | 13 ++-- 11 files changed, 125 insertions(+), 199 deletions(-) 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 206f92d2026848..913a3234b1d982 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 @@ -15,10 +15,12 @@ package com.google.devtools.build.lib.exec; import static com.google.common.util.concurrent.Futures.immediateVoidFuture; +import static com.google.common.util.concurrent.MoreExecutors.directExecutor; import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; +import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.ActionContext; import com.google.devtools.build.lib.actions.ActionExecutionContext; @@ -48,6 +50,7 @@ import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.SilentCloseable; +import com.google.devtools.build.lib.remote.common.BulkTransferException; import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code; @@ -246,13 +249,35 @@ public int getId() { public ListenableFuture prefetchInputs() throws IOException, ForbiddenActionInputException { if (Spawns.shouldPrefetchInputsForLocalExecution(spawn)) { - return actionExecutionContext - .getActionInputPrefetcher() - .prefetchFiles( - getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true) - .values(), - getMetadataProvider(), - Priority.MEDIUM); + return Futures.catchingAsync( + actionExecutionContext + .getActionInputPrefetcher() + .prefetchFiles( + getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true) + .values(), + getMetadataProvider(), + Priority.MEDIUM), + BulkTransferException.class, + (BulkTransferException e) -> { + if (BulkTransferException.allCausedByCacheNotFoundException(e)) { + var code = + executionOptions.useNewExitCodeForLostInputs + ? Code.REMOTE_CACHE_EVICTED + : Code.REMOTE_CACHE_FAILED; + throw new EnvironmentalExecException( + e, + FailureDetail.newBuilder() + .setMessage( + "Failed to fetch blobs because they do not exist remotely." + + " Build without the Bytes does not work if your remote" + + " cache evicts blobs during builds.") + .setSpawn(FailureDetails.Spawn.newBuilder().setCode(code)) + .build()); + } else { + throw e; + } + }, + directExecutor()); } return immediateVoidFuture(); diff --git a/src/main/java/com/google/devtools/build/lib/exec/BUILD b/src/main/java/com/google/devtools/build/lib/exec/BUILD index 168ac13f6786ef..36439c348f206c 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD @@ -27,6 +27,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/profiler", + "//src/main/java/com/google/devtools/build/lib/remote/common:bulk_transfer_exception", "//src/main/java/com/google/devtools/build/lib/util:command", "//src/main/java/com/google/devtools/build/lib/util/io", "//src/main/java/com/google/devtools/build/lib/vfs", 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 a97c3b053dde1b..df111978cebc21 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 @@ -22,7 +22,6 @@ import static com.google.devtools.build.lib.remote.util.RxFutures.toListenableFuture; import static com.google.devtools.build.lib.remote.util.RxUtils.mergeBulkTransfer; import static com.google.devtools.build.lib.remote.util.RxUtils.toTransferResult; -import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; @@ -277,11 +276,6 @@ protected abstract ListenableFuture doDownloadFile( protected void prefetchVirtualActionInput(VirtualActionInput input) throws IOException {} - /** Transforms the error encountered during the prefetch . */ - protected Completable onErrorResumeNext(Throwable error) { - return Completable.error(error); - } - /** * Fetches remotely stored action outputs, that are inputs to this spawn, and stores them under * their path in the output base. @@ -323,8 +317,7 @@ public ListenableFuture prefetchFiles( Completable prefetch = Completable.using( - () -> dirCtx, ctx -> mergeBulkTransfer(transfers), DirectoryContext::close) - .onErrorResumeNext(this::onErrorResumeNext); + () -> dirCtx, ctx -> mergeBulkTransfer(transfers), DirectoryContext::close); return toListenableFuture(prefetch); } @@ -427,30 +420,11 @@ private Symlink maybeGetSymlink( return null; } - /** - * Downloads file into the {@code path} with its metadata. - * - *

The file will be written into a temporary file and moved to the final destination after the - * download finished. - */ - private Completable downloadFileRx( - DirectoryContext dirCtx, - Path path, - @Nullable Path treeRoot, - @Nullable ActionInput actionInput, - FileArtifactValue metadata, - Priority priority) { - if (!canDownloadFile(path, metadata)) { - return Completable.complete(); - } - return downloadFileNoCheckRx(dirCtx, path, treeRoot, actionInput, metadata, priority); - } - private Completable downloadFileNoCheckRx( DirectoryContext dirCtx, Path path, @Nullable Path treeRoot, - @Nullable ActionInput actionInput, + ActionInput actionInput, FileArtifactValue metadata, Priority priority) { if (path.isSymbolicLink()) { @@ -492,7 +466,7 @@ private Completable downloadFileNoCheckRx( /* eager= */ false) .doOnError( error -> { - if (error instanceof CacheNotFoundException && actionInput != null) { + if (error instanceof CacheNotFoundException) { missingActionInputs.add(actionInput); } }); @@ -508,37 +482,6 @@ private Completable downloadFileNoCheckRx( })); } - /** - * Download file to the {@code path} with given metadata. Blocking await for the download to - * complete. - * - *

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) - throws IOException, InterruptedException { - getFromFuture(downloadFileAsync(path.asFragment(), actionInput, metadata, Priority.CRITICAL)); - } - - protected ListenableFuture downloadFileAsync( - PathFragment path, - @Nullable ActionInput actionInput, - FileArtifactValue metadata, - Priority priority) { - return toListenableFuture( - Completable.using( - DirectoryContext::new, - dirCtx -> - downloadFileRx( - dirCtx, - execRoot.getFileSystem().getPath(path), - /* treeRoot= */ null, - actionInput, - metadata, - priority), - DirectoryContext::close)); - } - private void finalizeDownload( DirectoryContext dirCtx, @Nullable Path treeRoot, Path tmpPath, Path finalPath) throws IOException { 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 3033522e8a1e07..3f28643f250012 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -207,6 +207,7 @@ java_library( deps = [ ":abstract_action_input_prefetcher", "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/actions:action_input_helper", "//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", @@ -220,6 +221,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/build/skyframe", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//third_party:flogger", 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 2998356786b342..e2c25cbf9d3a0d 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 @@ -20,12 +20,15 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.Streams.stream; +import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; 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.ActionInput; +import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionInputMap; +import com.google.devtools.build.lib.actions.ActionInputPrefetcher.Priority; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; @@ -566,10 +569,6 @@ public ActionInput getInput(String execPath) { return null; } - ActionInput getActionInput(PathFragment path) { - return getInput(path.relativeTo(execRoot).getPathString()); - } - @Nullable @Override public FileArtifactValue getMetadata(ActionInput input) throws IOException { @@ -605,7 +604,7 @@ private FileArtifactValue getMetadataByExecPath(PathFragment execPath) { } @Nullable - RemoteFileArtifactValue getRemoteMetadata(PathFragment path) { + private RemoteFileArtifactValue getRemoteMetadata(PathFragment path) { if (!isOutput(path)) { return null; } @@ -631,15 +630,21 @@ private TreeArtifactValue getRemoteTreeMetadata(PathFragment path) { } private void downloadFileIfRemote(PathFragment path) throws IOException { - FileArtifactValue m = getRemoteMetadata(path); - if (m != null) { - try { - inputFetcher.downloadFile(delegateFs.getPath(path), getActionInput(path), m); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new IOException( - String.format("Received interrupt while fetching file '%s'", path), e); + if (!isRemote(path)) { + return; + } + PathFragment execPath = path.relativeTo(execRoot); + try { + ActionInput input = getInput(execPath.getPathString()); + if (input == null) { + // For undeclared outputs, getInput returns null as there's no artifact associated with the + // path. Therefore, we synthesize one here just so we're able to call prefetchFiles. + input = ActionInputHelper.fromPath(execPath); } + getFromFuture(inputFetcher.prefetchFiles(ImmutableList.of(input), this, 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 28b4ccbb618c77..20bf04f8ce4e8b 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,25 +21,19 @@ 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.EnvironmentalExecException; 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.BulkTransferException; 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; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; -import com.google.devtools.build.lib.server.FailureDetails; -import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; -import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code; import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; -import io.reactivex.rxjava3.core.Completable; import java.io.IOException; import java.util.regex.Pattern; @@ -54,7 +48,6 @@ class RemoteActionInputFetcher extends AbstractActionInputPrefetcher { private final String buildRequestId; private final String commandId; private final RemoteCache remoteCache; - private final boolean useNewExitCodeForLostInputs; RemoteActionInputFetcher( Reporter reporter, @@ -64,13 +57,11 @@ class RemoteActionInputFetcher extends AbstractActionInputPrefetcher { Path execRoot, TempPathGenerator tempPathGenerator, ImmutableList patternsToDownload, - OutputPermissions outputPermissions, - boolean useNewExitCodeForLostInputs) { + OutputPermissions outputPermissions) { super(reporter, execRoot, tempPathGenerator, patternsToDownload, outputPermissions); this.buildRequestId = Preconditions.checkNotNull(buildRequestId); this.commandId = Preconditions.checkNotNull(commandId); this.remoteCache = Preconditions.checkNotNull(remoteCache); - this.useNewExitCodeForLostInputs = useNewExitCodeForLostInputs; } @Override @@ -141,25 +132,4 @@ public boolean isFinished() { return progress.finished(); } } - - @Override - protected Completable onErrorResumeNext(Throwable error) { - if (error instanceof BulkTransferException) { - if (((BulkTransferException) error).allCausedByCacheNotFoundException()) { - var code = - useNewExitCodeForLostInputs ? Code.REMOTE_CACHE_EVICTED : Code.REMOTE_CACHE_FAILED; - error = - new EnvironmentalExecException( - (BulkTransferException) error, - FailureDetail.newBuilder() - .setMessage( - "Failed to fetch blobs because they do not exist remotely." - + " Build without the Bytes does not work if your remote" - + " cache evicts blobs during builds") - .setSpawn(FailureDetails.Spawn.newBuilder().setCode(code)) - .build()); - } - } - return Completable.error(error); - } } 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 babbf4304f9f3a..5bffa74970717b 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 @@ -34,9 +34,7 @@ import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; -import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.analysis.AnalysisResult; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredTarget; @@ -61,7 +59,6 @@ import com.google.devtools.build.lib.exec.ModuleActionContextRegistry; import com.google.devtools.build.lib.exec.SpawnStrategyRegistry; import com.google.devtools.build.lib.remote.RemoteServerCapabilities.ServerCapabilitiesRequirement; -import com.google.devtools.build.lib.remote.ToplevelArtifactsDownloader.PathToMetadataConverter; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; import com.google.devtools.build.lib.remote.downloader.GrpcRemoteDownloader; @@ -965,8 +962,6 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB actionContextProvider.setTempPathGenerator(tempPathGenerator); - ExecutionOptions executionOptions = - Preconditions.checkNotNull(env.getOptions().getOptions(ExecutionOptions.class)); RemoteOptions remoteOptions = Preconditions.checkNotNull( env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions"); @@ -988,8 +983,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB env.getExecRoot(), tempPathGenerator, patternsToDownload, - outputPermissions, - executionOptions.useNewExitCodeForLostInputs); + outputPermissions); env.getEventBus().register(actionInputFetcher); builder.setActionInputPrefetcher(actionInputFetcher); actionContextProvider.setActionInputFetcher(actionInputFetcher); @@ -1000,28 +994,13 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB remoteOutputsMode.downloadToplevelOutputsOnly(), env.getSkyframeExecutor().getEvaluator(), actionInputFetcher, - new PathToMetadataConverter() { - @Nullable - @Override - public FileArtifactValue getMetadata(Path path) { - FileSystem fileSystem = path.getFileSystem(); - if (fileSystem instanceof RemoteActionFileSystem) { - return ((RemoteActionFileSystem) path.getFileSystem()) - .getRemoteMetadata(path.asFragment()); - } - return null; - } - - @Nullable - @Override - public ActionInput getActionInput(Path path) { - FileSystem fileSystem = path.getFileSystem(); - if (fileSystem instanceof RemoteActionFileSystem) { - return ((RemoteActionFileSystem) path.getFileSystem()) - .getActionInput(path.asFragment()); - } - return null; + env.getExecRoot().asFragment(), + (path) -> { + FileSystem fileSystem = path.getFileSystem(); + if (fileSystem instanceof RemoteActionFileSystem) { + return (RemoteActionFileSystem) path.getFileSystem(); } + return null; }); env.getEventBus().register(toplevelArtifactsDownloader); 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 e67a3ee779ba86..06ebe98c0493b7 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 @@ -19,6 +19,7 @@ import static com.google.common.util.concurrent.MoreExecutors.directExecutor; import static com.google.devtools.build.lib.packages.TargetUtils.isTestRuleName; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.eventbus.AllowConcurrentEvents; import com.google.common.eventbus.Subscribe; @@ -26,9 +27,11 @@ import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionInputPrefetcher.Priority; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.MetadataProvider; import com.google.devtools.build.lib.analysis.AspectCompleteEvent; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; @@ -44,6 +47,7 @@ import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.MemoizingEvaluator; import com.google.devtools.build.skyframe.SkyValue; import javax.annotation.Nullable; @@ -67,14 +71,16 @@ private enum CommandMode { private final boolean downloadToplevel; private final MemoizingEvaluator memoizingEvaluator; private final AbstractActionInputPrefetcher actionInputPrefetcher; - private final PathToMetadataConverter pathToMetadataConverter; + private final PathFragment execRoot; + private final PathToMetadataProvider pathToMetadataProvider; public ToplevelArtifactsDownloader( String commandName, boolean downloadToplevel, MemoizingEvaluator memoizingEvaluator, AbstractActionInputPrefetcher actionInputPrefetcher, - PathToMetadataConverter pathToMetadataConverter) { + PathFragment execRoot, + PathToMetadataProvider pathToMetadataProvider) { switch (commandName) { case "build": this.commandMode = CommandMode.BUILD; @@ -94,38 +100,38 @@ public ToplevelArtifactsDownloader( this.downloadToplevel = downloadToplevel; this.memoizingEvaluator = memoizingEvaluator; this.actionInputPrefetcher = actionInputPrefetcher; - this.pathToMetadataConverter = pathToMetadataConverter; + this.execRoot = execRoot; + this.pathToMetadataProvider = pathToMetadataProvider; } /** - * Interface that converts {@link Path} to metadata {@link FileArtifactValue}. + * Interface that converts a {@link Path} into a {@link MetadataProvider} suitable for retrieving + * metadata for that path. * - *

{@link ToplevelArtifactsDownloader} is only used with {@code ActionFileSystem} together. If - * we see a {@link Path}, its underlying file system must be {@code ActionFileSystem}. We use this - * interface to avoid passing in the actionFs implementation. + *

{@link ToplevelArtifactsDownloader} may only used in conjunction with filesystems that + * implement {@link MetadataProvider}. */ - public interface PathToMetadataConverter { + public interface PathToMetadataProvider { @Nullable - FileArtifactValue getMetadata(Path path); - - @Nullable - ActionInput getActionInput(Path path); + MetadataProvider getMetadataProvider(Path path); } private void downloadTestOutput(Path path) { // Since the event is fired within action execution, the skyframe doesn't know the outputs of - // test actions yet, so we can't get their metadata through skyframe. However, the fileSystem - // of the path is an ActionFileSystem, we use it to get the metadata for this file. + // test actions yet, so we can't get their metadata through skyframe. However, since the path + // belongs to a filesystem that implements MetadataProvider, we use it to get the metadata. // // If the test hit action cache, the filesystem is local filesystem because the actual test - // action didn't get the chance to execute. In this case the metadata is null which is fine - // because test outputs are already downloaded (otherwise it cannot hit the action cache). - FileArtifactValue metadata = pathToMetadataConverter.getMetadata(path); - ActionInput actionInput = pathToMetadataConverter.getActionInput(path); - if (metadata != null) { + // action didn't get the chance to execute. In this case the MetadataProvider is null, which + // is fine because test outputs are already downloaded (otherwise the action cache wouldn't + // have been hit). + MetadataProvider metadataProvider = pathToMetadataProvider.getMetadataProvider(path); + if (metadataProvider != null) { + // RemoteActionFileSystem#getInput returns null for undeclared test outputs. + ActionInput input = ActionInputHelper.fromPath(path.asFragment().relativeTo(execRoot)); ListenableFuture future = - actionInputPrefetcher.downloadFileAsync( - path.asFragment(), actionInput, metadata, Priority.LOW); + actionInputPrefetcher.prefetchFiles( + ImmutableList.of(input), metadataProvider, Priority.LOW); addCallback( future, new FutureCallback() { 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 32ae32cb48856d..d76c3ce8b797a1 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 @@ -18,6 +18,7 @@ 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.createTreeArtifactWithGeneratingAction; +import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertThrows; import static org.mockito.ArgumentMatchers.any; @@ -570,22 +571,7 @@ public void prefetchFiles_multipleThreads_downloadIsNotCancelledByOtherThreads() } @Test - public void downloadFile_downloadRemoteFiles() throws Exception { - Map metadata = new HashMap<>(); - Map cas = new HashMap<>(); - Artifact a1 = createRemoteArtifact("file1", "hello world", metadata, cas); - AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - - prefetcher.downloadFile(a1.getPath(), /* actionInput= */ null, metadata.get(a1)); - - assertThat(FileSystemUtils.readContent(a1.getPath(), UTF_8)).isEqualTo("hello world"); - assertThat(a1.getPath().isExecutable()).isTrue(); - assertThat(a1.getPath().isReadable()).isTrue(); - assertThat(a1.getPath().isWritable()).isFalse(); - } - - @Test - public void downloadFile_onInterrupt_deletePartialDownloadedFile() throws Exception { + public void prefetchFiles_onInterrupt_deletePartialDownloadedFile() throws Exception { Semaphore startSemaphore = new Semaphore(0); Semaphore endSemaphore = new Semaphore(0); Map metadata = new HashMap<>(); @@ -605,7 +591,11 @@ public void downloadFile_onInterrupt_deletePartialDownloadedFile() throws Except new Thread( () -> { try { - prefetcher.downloadFile(a1.getPath(), /* actionInput= */ null, metadata.get(a1)); + getFromFuture( + prefetcher.prefetchFiles( + ImmutableList.of(a1), + new StaticMetadataProvider(metadata), + Priority.MEDIUM)); } catch (IOException ignored) { // Intentionally left empty } catch (InterruptedException e) { 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 8037c557b046d5..c8de525c3513c6 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 @@ -16,6 +16,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; +import static com.google.common.util.concurrent.Futures.immediateVoidFuture; import static java.nio.charset.StandardCharsets.UTF_8; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -28,7 +29,11 @@ import com.google.common.collect.ImmutableList; 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.ActionInput; +import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionInputMap; +import com.google.devtools.build.lib.actions.ActionInputPrefetcher.Priority; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; @@ -50,6 +55,7 @@ import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.io.IOException; import java.util.Map; import org.junit.Before; @@ -113,10 +119,10 @@ protected PathFragment getOutputPath(String outputRootRelativePath) { return outputRoot.getRoot().asPath().getRelative(outputRootRelativePath).asFragment(); } - private static Answer mockDownloadFile(Path path, String contents) { + private static Answer> mockPrefetchFile(Path path, String contents) { return invocationOnMock -> { FileSystemUtils.writeContent(path, UTF_8, contents); - return null; + return immediateVoidFuture(); }; } @@ -142,9 +148,9 @@ public void testGetInputStream_fromInputArtifactData_forRemoteArtifact() throws ActionInputMap inputs = new ActionInputMap(1); Artifact artifact = createRemoteArtifact("remote-file", "remote contents", inputs); FileSystem actionFs = createActionFileSystem(inputs); - doAnswer(mockDownloadFile(artifact.getPath(), "remote contents")) + doAnswer(mockPrefetchFile(artifact.getPath(), "remote contents")) .when(inputFetcher) - .downloadFile(eq(artifact.getPath()), any(), eq(inputs.getMetadata(artifact))); + .prefetchFiles(eq(ImmutableList.of(artifact)), any(), eq(Priority.CRITICAL)); // act Path actionFsPath = actionFs.getPath(artifact.getPath().asFragment()); @@ -154,7 +160,7 @@ public void testGetInputStream_fromInputArtifactData_forRemoteArtifact() throws assertThat(actionFsPath.getFileSystem()).isSameInstanceAs(actionFs); assertThat(contents).isEqualTo("remote contents"); verify(inputFetcher) - .downloadFile(eq(artifact.getPath()), any(), eq(inputs.getMetadata(artifact))); + .prefetchFiles(eq(ImmutableList.of(artifact)), any(), eq(Priority.CRITICAL)); verifyNoMoreInteractions(inputFetcher); } @@ -163,11 +169,10 @@ public void testGetInputStream_fromRemoteOutputTree_forDeclaredOutput() throws E // arrange Artifact artifact = ActionsTestUtil.createArtifact(outputRoot, "out"); FileSystem actionFs = createActionFileSystem(new ActionInputMap(0), ImmutableList.of(artifact)); - FileArtifactValue metadata = - injectRemoteFile(actionFs, artifact.getPath().asFragment(), "remote contents"); - doAnswer(mockDownloadFile(artifact.getPath(), "remote contents")) + injectRemoteFile(actionFs, artifact.getPath().asFragment(), "remote contents"); + doAnswer(mockPrefetchFile(artifact.getPath(), "remote contents")) .when(inputFetcher) - .downloadFile(eq(artifact.getPath()), any(), eq(metadata)); + .prefetchFiles(eq(ImmutableList.of(artifact)), any(), eq(Priority.CRITICAL)); // act Path actionFsPath = actionFs.getPath(artifact.getPath().asFragment()); @@ -176,7 +181,8 @@ public void testGetInputStream_fromRemoteOutputTree_forDeclaredOutput() throws E // assert assertThat(actionFsPath.getFileSystem()).isSameInstanceAs(actionFs); assertThat(contents).isEqualTo("remote contents"); - verify(inputFetcher).downloadFile(eq(artifact.getPath()), any(), eq(metadata)); + verify(inputFetcher) + .prefetchFiles(eq(ImmutableList.of(artifact)), any(), eq(Priority.CRITICAL)); verifyNoMoreInteractions(inputFetcher); } @@ -184,11 +190,12 @@ public void testGetInputStream_fromRemoteOutputTree_forDeclaredOutput() throws E public void testGetInputStream_fromRemoteOutputTree_forUndeclaredOutput() throws Exception { // arrange Path path = outputRoot.getRoot().getRelative("out"); + ActionInput input = ActionInputHelper.fromPath(path.relativeTo(execRoot)); FileSystem actionFs = createActionFileSystem(); - FileArtifactValue metadata = injectRemoteFile(actionFs, path.asFragment(), "remote contents"); - doAnswer(mockDownloadFile(path, "remote contents")) + injectRemoteFile(actionFs, path.asFragment(), "remote contents"); + doAnswer(mockPrefetchFile(path, "remote contents")) .when(inputFetcher) - .downloadFile(eq(path), any(), eq(metadata)); + .prefetchFiles(eq(ImmutableList.of(input)), any(), eq(Priority.CRITICAL)); // act Path actionFsPath = actionFs.getPath(path.asFragment()); @@ -197,7 +204,7 @@ public void testGetInputStream_fromRemoteOutputTree_forUndeclaredOutput() throws // assert assertThat(actionFsPath.getFileSystem()).isSameInstanceAs(actionFs); assertThat(contents).isEqualTo("remote contents"); - verify(inputFetcher).downloadFile(eq(path), any(), eq(metadata)); + verify(inputFetcher).prefetchFiles(eq(ImmutableList.of(input)), any(), eq(Priority.CRITICAL)); verifyNoMoreInteractions(inputFetcher); } @@ -564,6 +571,7 @@ public void renameTo_onlyLocalFile_renameLocalFile() throws Exception { } @Override + @CanIgnoreReturnValue protected FileArtifactValue injectRemoteFile( FileSystem actionFs, PathFragment path, String content) throws IOException { byte[] contentBytes = content.getBytes(UTF_8); 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 36bbf24fbbadf8..b4d7f51b72c712 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 @@ -25,12 +25,12 @@ import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputPrefetcher.Priority; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.MetadataProvider; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.events.Reporter; +import com.google.devtools.build.lib.remote.common.BulkTransferException; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.InMemoryCacheClient; @@ -76,8 +76,7 @@ protected AbstractActionInputPrefetcher createPrefetcher(Map c execRoot, tempPathGenerator, ImmutableList.of(), - OutputPermissions.READONLY, - /* useNewExitCodeForLostInputs= */ false); + OutputPermissions.READONLY); } @Test @@ -94,8 +93,7 @@ public void testStagingVirtualActionInput() throws Exception { execRoot, tempPathGenerator, ImmutableList.of(), - OutputPermissions.READONLY, - /* useNewExitCodeForLostInputs= */ false); + OutputPermissions.READONLY); VirtualActionInput a = ActionsTestUtil.createVirtualActionInput("file1", "hello world"); // act @@ -123,8 +121,7 @@ public void testStagingEmptyVirtualActionInput() throws Exception { execRoot, tempPathGenerator, ImmutableList.of(), - OutputPermissions.READONLY, - /* useNewExitCodeForLostInputs= */ false); + OutputPermissions.READONLY); // act wait( @@ -145,7 +142,7 @@ public void prefetchFiles_missingFiles_failsWithSpecificMessage() throws Excepti var error = assertThrows( - ExecException.class, + BulkTransferException.class, () -> wait( prefetcher.prefetchFiles(