diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java index 1e9b65026482e6..438210dbc4f8f1 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java @@ -46,6 +46,7 @@ import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.concurrent.ThreadSafety; +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.AbstractRemoteActionCache.ActionResultMetadata.DirectoryMetadata; @@ -70,6 +71,7 @@ import java.io.OutputStream; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.Comparator; import java.util.HashMap; import java.util.List; @@ -83,6 +85,12 @@ @ThreadSafety.ThreadSafe public abstract class AbstractRemoteActionCache implements AutoCloseable { + /** See {@link SpawnExecutionContext#lockOutputFiles()}. */ + @FunctionalInterface + interface OutputFilesLocker { + void lock() throws InterruptedException; + } + private static final ListenableFuture COMPLETED_SUCCESS = SettableFuture.create(); private static final ListenableFuture EMPTY_BYTES = SettableFuture.create(); @@ -161,16 +169,24 @@ public void onFailure(Throwable t) { return outerF; } + private static Path toTmpDownloadPath(Path actualPath) { + return actualPath.getParentDirectory().getRelative(actualPath.getBaseName() + ".tmp"); + } + /** * Download the output files and directory trees of a remotely executed action to the local * machine, as well stdin / stdout to the given files. * *

In case of failure, this method deletes any output files it might have already created. * + * @param outputFilesLocker ensures that we are the only ones writing to the output files when + * using the dynamic spawn strategy. + * * @throws IOException in case of a cache miss or if the remote cache is unavailable. * @throws ExecException in case clean up after a failed download failed. */ - public void download(ActionResult result, Path execRoot, FileOutErr origOutErr) + public void download(ActionResult result, Path execRoot, FileOutErr origOutErr, + OutputFilesLocker outputFilesLocker) throws ExecException, IOException, InterruptedException { ActionResultMetadata metadata = parseActionResultMetadata(result, execRoot); @@ -182,7 +198,8 @@ public void download(ActionResult result, Path execRoot, FileOutErr origOutErr) .map( (file) -> { try { - ListenableFuture download = downloadFile(file.path(), file.digest()); + ListenableFuture download = + downloadFile(toTmpDownloadPath(file.path()), file.digest()); return Futures.transform(download, (d) -> file, directExecutor()); } catch (IOException e) { return Futures.immediateFailedFuture(e); @@ -209,10 +226,8 @@ public void download(ActionResult result, Path execRoot, FileOutErr origOutErr) for (ListenableFuture download : downloads) { try { - FileMetadata outputFile = getFromFuture(download); - if (outputFile != null) { - outputFile.path().setExecutable(outputFile.isExecutable()); - } + // Wait for all downloads to finish. + getFromFuture(download); } catch (IOException e) { downloadException = downloadException == null ? e : downloadException; } catch (InterruptedException e) { @@ -222,10 +237,9 @@ public void download(ActionResult result, Path execRoot, FileOutErr origOutErr) if (downloadException != null || interruptedException != null) { try { - // Delete any (partially) downloaded output files, since any subsequent local execution - // of this action may expect none of the output files to exist. + // Delete any (partially) downloaded output files. for (OutputFile file : result.getOutputFilesList()) { - execRoot.getRelative(file.getPath()).delete(); + toTmpDownloadPath(execRoot.getRelative(file.getPath())).delete(); } for (OutputDirectory directory : result.getOutputDirectoriesList()) { // Only delete the directories below the output directories because the output @@ -261,6 +275,12 @@ public void download(ActionResult result, Path execRoot, FileOutErr origOutErr) tmpOutErr.clearErr(); } + // Ensure that we are the only ones writing to the output files when using the dynamic spawn + // strategy. + outputFilesLocker.lock(); + + moveOutputsToFinalLocation(downloads); + List symlinksInDirectories = new ArrayList<>(); for (Entry entry : metadata.directories()) { entry.getKey().createDirectoryAndParents(); @@ -275,6 +295,36 @@ public void download(ActionResult result, Path execRoot, FileOutErr origOutErr) createSymlinks(symlinks); } + /** + * Copies moves the downloaded outputs from their download location to their declared location. + */ + private void moveOutputsToFinalLocation(List> downloads) + throws IOException, InterruptedException { + List finishedDownloads = new ArrayList<>(downloads.size()); + for (ListenableFuture finishedDownload : downloads) { + FileMetadata outputFile = getFromFuture(finishedDownload); + if (outputFile != null) { + finishedDownloads.add(outputFile); + } + } + /* + * Sort the list lexicographically based on its temporary download path in order to avoid + * filename clashes when moving the files: + * + * Consider an action that produces two outputs foo and foo.tmp. These outputs would initially + * be downloaded to foo.tmp and foo.tmp.tmp. When renaming them to foo and foo.tmp we need to + * ensure that rename(foo.tmp, foo) happens before rename(foo.tmp.tmp, foo.tmp). We ensure this + * by doing the renames in lexicographical order of the download names. + */ + Collections.sort(finishedDownloads, Comparator.comparing(f -> toTmpDownloadPath(f.path()))); + + // Move the output files from their temporary name to the actual output file name. + for (FileMetadata outputFile : finishedDownloads) { + FileSystemUtils.moveFile(toTmpDownloadPath(outputFile.path()), outputFile.path()); + outputFile.path().setExecutable(outputFile.isExecutable()); + } + } + private void createSymlinks(Iterable symlinks) throws IOException { for (SymlinkMetadata symlink : symlinks) { if (symlink.target().isAbsolute()) { @@ -376,6 +426,8 @@ private List> downloadOutErr(ActionResult result, * @param execRoot the execution root * @param metadataInjector the action's metadata injector that allows this method to inject * metadata about an action output instead of downloading the output + * @param outputFilesLocker ensures that we are the only ones writing to the output files when + * using the dynamic spawn strategy. * @throws IOException in case of failure * @throws InterruptedException in case of receiving an interrupt */ @@ -386,8 +438,8 @@ public InMemoryOutput downloadMinimal( @Nullable PathFragment inMemoryOutputPath, OutErr outErr, Path execRoot, - MetadataInjector metadataInjector) - throws IOException, InterruptedException { + MetadataInjector metadataInjector, + OutputFilesLocker outputFilesLocker) throws IOException, InterruptedException { Preconditions.checkState( result.getExitCode() == 0, "injecting remote metadata is only supported for successful actions (exit code 0)."); @@ -403,6 +455,10 @@ public InMemoryOutput downloadMinimal( + "--experimental_remote_download_outputs=minimal"); } + // Ensure that when using dynamic spawn strategy that we are the only ones writing to the + // output files. + outputFilesLocker.lock(); + ActionInput inMemoryOutput = null; Digest inMemoryOutputDigest = null; for (ActionInput output : outputs) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index 4bd91ae1ac9479..3c0cf0e03cea0e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java @@ -167,7 +167,8 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) if (downloadOutputs) { try (SilentCloseable c = prof.profile(ProfilerTask.REMOTE_DOWNLOAD, "download outputs")) { - remoteCache.download(result, execRoot, context.getFileOutErr()); + remoteCache.download(result, execRoot, context.getFileOutErr(), + context::lockOutputFiles); } } else { PathFragment inMemoryOutputPath = getInMemoryOutputPath(spawn); @@ -181,7 +182,8 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) inMemoryOutputPath, context.getFileOutErr(), execRoot, - context.getMetadataInjector()); + context.getMetadataInjector(), + context::lockOutputFiles); } } SpawnResult spawnResult = diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index 3e57b5c989c693..4a2ab8577c463a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -312,7 +312,8 @@ private SpawnResult downloadAndFinalizeSpawnResult( InMemoryOutput inMemoryOutput = null; if (downloadOutputs) { try (SilentCloseable c = Profiler.instance().profile(REMOTE_DOWNLOAD, "download outputs")) { - remoteCache.download(actionResult, execRoot, context.getFileOutErr()); + remoteCache.download(actionResult, execRoot, context.getFileOutErr(), + context::lockOutputFiles); } } else { PathFragment inMemoryOutputPath = getInMemoryOutputPath(spawn); @@ -325,7 +326,8 @@ private SpawnResult downloadAndFinalizeSpawnResult( inMemoryOutputPath, context.getFileOutErr(), execRoot, - context.getMetadataInjector()); + context.getMetadataInjector(), + context::lockOutputFiles); } } return createSpawnResult(actionResult.getExitCode(), cacheHit, getName(), inMemoryOutput); @@ -405,10 +407,11 @@ private SpawnResult execLocallyAndUploadOrFail( return execLocallyAndUpload( spawn, context, inputMap, remoteCache, actionKey, action, command, uploadLocalResults); } - return handleError(cause, context.getFileOutErr(), actionKey); + return handleError(cause, context.getFileOutErr(), actionKey, context); } - private SpawnResult handleError(IOException exception, FileOutErr outErr, ActionKey actionKey) + private SpawnResult handleError(IOException exception, FileOutErr outErr, ActionKey actionKey, + SpawnExecutionContext context) throws ExecException, InterruptedException, IOException { if (exception.getCause() instanceof ExecutionStatusException) { ExecutionStatusException e = (ExecutionStatusException) exception.getCause(); @@ -417,7 +420,7 @@ private SpawnResult handleError(IOException exception, FileOutErr outErr, Action maybeDownloadServerLogs(resp, actionKey); if (resp.hasResult()) { // We try to download all (partial) results even on server error, for debuggability. - remoteCache.download(resp.getResult(), execRoot, outErr); + remoteCache.download(resp.getResult(), execRoot, outErr, context::lockOutputFiles); } } if (e.isExecutionTimeout()) { diff --git a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java index 2c4b95bb6c1e57..68d2d53d47fd74 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java +++ b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java @@ -20,6 +20,7 @@ import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -34,6 +35,7 @@ import build.bazel.remote.execution.v2.OutputFile; import build.bazel.remote.execution.v2.SymlinkNode; import build.bazel.remote.execution.v2.Tree; +import com.google.common.base.Charsets; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -52,6 +54,7 @@ import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.clock.JavaClock; +import com.google.devtools.build.lib.remote.AbstractRemoteActionCache.OutputFilesLocker; import com.google.devtools.build.lib.remote.AbstractRemoteActionCache.UploadManifest; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; @@ -86,12 +89,17 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.Mock; import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; /** Tests for {@link AbstractRemoteActionCache}. */ @RunWith(JUnit4.class) public class AbstractRemoteActionCacheTests { + @Mock + private OutputFilesLocker outputFilesLocker; + private FileSystem fs; private Path execRoot; ArtifactRoot artifactRoot; @@ -106,6 +114,7 @@ public static void beforeEverything() { @Before public void setUp() throws Exception { + MockitoAnnotations.initMocks(this); fs = new InMemoryFileSystem(new JavaClock(), DigestHashFunction.SHA256); execRoot = fs.getPath("/execroot"); execRoot.createDirectoryAndParents(); @@ -531,10 +540,11 @@ public void downloadRelativeFileSymlink() throws Exception { ActionResult.Builder result = ActionResult.newBuilder(); result.addOutputFileSymlinksBuilder().setPath("a/b/link").setTarget("../../foo"); // Doesn't check for dangling links, hence download succeeds. - cache.download(result.build(), execRoot, null); + cache.download(result.build(), execRoot, null, outputFilesLocker); Path path = execRoot.getRelative("a/b/link"); assertThat(path.isSymbolicLink()).isTrue(); assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("../../foo")); + verify(outputFilesLocker).lock(); } @Test @@ -543,10 +553,11 @@ public void downloadRelativeDirectorySymlink() throws Exception { ActionResult.Builder result = ActionResult.newBuilder(); result.addOutputDirectorySymlinksBuilder().setPath("a/b/link").setTarget("foo"); // Doesn't check for dangling links, hence download succeeds. - cache.download(result.build(), execRoot, null); + cache.download(result.build(), execRoot, null, outputFilesLocker); Path path = execRoot.getRelative("a/b/link"); assertThat(path.isSymbolicLink()).isTrue(); assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("foo")); + verify(outputFilesLocker).lock(); } @Test @@ -562,10 +573,11 @@ public void downloadRelativeSymlinkInDirectory() throws Exception { ActionResult.Builder result = ActionResult.newBuilder(); result.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); // Doesn't check for dangling links, hence download succeeds. - cache.download(result.build(), execRoot, null); + cache.download(result.build(), execRoot, null, outputFilesLocker); Path path = execRoot.getRelative("dir/link"); assertThat(path.isSymbolicLink()).isTrue(); assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("../foo")); + verify(outputFilesLocker).lock(); } @Test @@ -574,9 +586,10 @@ public void downloadAbsoluteDirectorySymlinkError() throws Exception { ActionResult.Builder result = ActionResult.newBuilder(); result.addOutputDirectorySymlinksBuilder().setPath("foo").setTarget("/abs/link"); IOException expected = - assertThrows(IOException.class, () -> cache.download(result.build(), execRoot, null)); + assertThrows(IOException.class, () -> cache.download(result.build(), execRoot, null, outputFilesLocker)); assertThat(expected).hasMessageThat().contains("/abs/link"); assertThat(expected).hasMessageThat().contains("absolute path"); + verify(outputFilesLocker).lock(); } @Test @@ -585,9 +598,10 @@ public void downloadAbsoluteFileSymlinkError() throws Exception { ActionResult.Builder result = ActionResult.newBuilder(); result.addOutputFileSymlinksBuilder().setPath("foo").setTarget("/abs/link"); IOException expected = - assertThrows(IOException.class, () -> cache.download(result.build(), execRoot, null)); + assertThrows(IOException.class, () -> cache.download(result.build(), execRoot, null, outputFilesLocker)); assertThat(expected).hasMessageThat().contains("/abs/link"); assertThat(expected).hasMessageThat().contains("absolute path"); + verify(outputFilesLocker).lock(); } @Test @@ -603,10 +617,11 @@ public void downloadAbsoluteSymlinkInDirectoryError() throws Exception { ActionResult.Builder result = ActionResult.newBuilder(); result.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); IOException expected = - assertThrows(IOException.class, () -> cache.download(result.build(), execRoot, null)); + assertThrows(IOException.class, () -> cache.download(result.build(), execRoot, null, outputFilesLocker)); assertThat(expected).hasMessageThat().contains("dir/link"); assertThat(expected).hasMessageThat().contains("/foo"); assertThat(expected).hasMessageThat().contains("absolute path"); + verify(outputFilesLocker).lock(); } @Test @@ -623,11 +638,12 @@ public void downloadFailureMaintainsDirectories() throws Exception { result.addOutputFiles( OutputFile.newBuilder().setPath("outputdir/outputfile").setDigest(outputFileDigest)); result.addOutputFiles(OutputFile.newBuilder().setPath("otherfile").setDigest(otherFileDigest)); - assertThrows(IOException.class, () -> cache.download(result.build(), execRoot, null)); + assertThrows(IOException.class, () -> cache.download(result.build(), execRoot, null, outputFilesLocker)); assertThat(cache.getNumFailedDownloads()).isEqualTo(1); assertThat(execRoot.getRelative("outputdir").exists()).isTrue(); assertThat(execRoot.getRelative("outputdir/outputfile").exists()).isFalse(); assertThat(execRoot.getRelative("otherfile").exists()).isFalse(); + verify(outputFilesLocker, never()).lock(); } @Test @@ -654,11 +670,12 @@ public void onErrorWaitForRemainingDownloadsToComplete() throws Exception { IOException e = assertThrows( IOException.class, - () -> cache.download(result, execRoot, new FileOutErr(stdout, stderr))); + () -> cache.download(result, execRoot, new FileOutErr(stdout, stderr), outputFilesLocker)); assertThat(cache.getNumSuccessfulDownloads()).isEqualTo(2); assertThat(cache.getNumFailedDownloads()).isEqualTo(1); assertThat(cache.getDownloadQueueSize()).isEqualTo(3); assertThat(Throwables.getRootCause(e)).hasMessageThat().isEqualTo("download failed"); + verify(outputFilesLocker, never()).lock(); } @Test @@ -684,7 +701,7 @@ public void testDownloadWithStdoutStderrOnSuccess() throws Exception { .setStderrDigest(digestStderr) .build(); - cache.download(result, execRoot, spyOutErr); + cache.download(result, execRoot, spyOutErr, outputFilesLocker); verify(spyOutErr, Mockito.times(2)).childOutErr(); verify(spyChildOutErr).clearOut(); @@ -698,6 +715,8 @@ public void testDownloadWithStdoutStderrOnSuccess() throws Exception { } catch (IOException err) { throw new AssertionError("outErr should still be writable after download finished.", err); } + + verify(outputFilesLocker).lock(); } @Test @@ -724,7 +743,7 @@ public void testDownloadWithStdoutStderrOnFailure() throws Exception { .setStderrDigest(digestStderr) .build(); IOException e = - assertThrows(IOException.class, () -> cache.download(result, execRoot, spyOutErr)); + assertThrows(IOException.class, () -> cache.download(result, execRoot, spyOutErr, outputFilesLocker)); verify(spyOutErr, Mockito.times(2)).childOutErr(); verify(spyChildOutErr).clearOut(); verify(spyChildOutErr).clearErr(); @@ -737,6 +756,38 @@ public void testDownloadWithStdoutStderrOnFailure() throws Exception { } catch (IOException err) { throw new AssertionError("outErr should still be writable after download failed.", err); } + + verify(outputFilesLocker, never()).lock(); + } + + + @Test + public void testDownloadClashes() throws Exception { + // Test that injecting the metadata for a remote output file works + + // arrange + DefaultRemoteActionCache remoteCache = newTestCache(); + Digest d1 = remoteCache.addContents("content1"); + Digest d2 = remoteCache.addContents("content2"); + ActionResult r = + ActionResult.newBuilder() + .setExitCode(0) + .addOutputFiles(OutputFile.newBuilder().setPath("outputs/foo.tmp").setDigest(d1)) + .addOutputFiles(OutputFile.newBuilder().setPath("outputs/foo").setDigest(d2)) + .build(); + + Artifact a1 = ActionsTestUtil.createArtifact(artifactRoot, "foo.tmp"); + Artifact a2 = ActionsTestUtil.createArtifact(artifactRoot, "foo"); + + // act + + remoteCache.download(r, execRoot, new FileOutErr(), outputFilesLocker); + + // assert + + assertThat(FileSystemUtils.readContent(a1.getPath(), Charsets.UTF_8)).isEqualTo("content1"); + assertThat(FileSystemUtils.readContent(a2.getPath(), Charsets.UTF_8)).isEqualTo("content2"); + verify(outputFilesLocker).lock(); } @Test @@ -767,7 +818,8 @@ public void testDownloadMinimalFiles() throws Exception { /* inMemoryOutputPath= */ null, new FileOutErr(), execRoot, - injector); + injector, + outputFilesLocker); // assert assertThat(inMemoryOutput).isNull(); @@ -778,6 +830,8 @@ public void testDownloadMinimalFiles() throws Exception { Path outputBase = artifactRoot.getRoot().asPath(); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); + + verify(outputFilesLocker).lock(); } @Test @@ -826,7 +880,8 @@ public void testDownloadMinimalDirectory() throws Exception { /* inMemoryOutputPath= */ null, new FileOutErr(), execRoot, - injector); + injector, + outputFilesLocker); // assert assertThat(inMemoryOutput).isNull(); @@ -844,6 +899,8 @@ public void testDownloadMinimalDirectory() throws Exception { Path outputBase = artifactRoot.getRoot().asPath(); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); + + verify(outputFilesLocker).lock(); } @Test @@ -896,8 +953,11 @@ public void testDownloadMinimalDirectoryFails() throws Exception { /* inMemoryOutputPath= */ null, new FileOutErr(), execRoot, - injector)); + injector, + outputFilesLocker)); assertThat(e).isEqualTo(downloadTreeException); + + verify(outputFilesLocker, never()).lock(); } @Test @@ -920,7 +980,8 @@ public void testDownloadMinimalWithStdoutStderr() throws Exception { // act InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( - r, ImmutableList.of(), /* inMemoryOutputPath= */ null, outErr, execRoot, injector); + r, ImmutableList.of(), /* inMemoryOutputPath= */ null, outErr, execRoot, injector, + outputFilesLocker); // assert assertThat(inMemoryOutput).isNull(); @@ -929,6 +990,8 @@ public void testDownloadMinimalWithStdoutStderr() throws Exception { Path outputBase = artifactRoot.getRoot().asPath(); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); + + verify(outputFilesLocker).lock(); } @Test @@ -959,7 +1022,8 @@ public void testDownloadMinimalWithInMemoryOutput() throws Exception { inMemoryOutputPathFragment, new FileOutErr(), execRoot, - injector); + injector, + outputFilesLocker); // assert assertThat(inMemoryOutput).isNotNull(); @@ -974,6 +1038,8 @@ public void testDownloadMinimalWithInMemoryOutput() throws Exception { Path outputBase = artifactRoot.getRoot().asPath(); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); + + verify(outputFilesLocker).lock(); } private DefaultRemoteActionCache newTestCache() { diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java index 68a2d457e83846..014145a4069882 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java @@ -346,7 +346,7 @@ public void testDownloadAllResults() throws Exception { result.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); result.addOutputFilesBuilder().setPath("b/empty").setDigest(emptyDigest); result.addOutputFilesBuilder().setPath("a/bar").setDigest(barDigest).setIsExecutable(true); - client.download(result.build(), execRoot, null); + client.download(result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/foo"))).isEqualTo(fooDigest); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("b/empty"))).isEqualTo(emptyDigest); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/bar"))).isEqualTo(barDigest); @@ -379,7 +379,7 @@ public void testDownloadDirectory() throws Exception { ActionResult.Builder result = ActionResult.newBuilder(); result.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); result.addOutputDirectoriesBuilder().setPath("a/bar").setTreeDigest(barTreeDigest); - client.download(result.build(), execRoot, null); + client.download(result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/foo"))).isEqualTo(fooDigest); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/bar/qux"))).isEqualTo(quxDigest); @@ -397,7 +397,7 @@ public void testDownloadDirectoryEmpty() throws Exception { ActionResult.Builder result = ActionResult.newBuilder(); result.addOutputDirectoriesBuilder().setPath("a/bar").setTreeDigest(barTreeDigest); - client.download(result.build(), execRoot, null); + client.download(result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); assertThat(execRoot.getRelative("a/bar").isDirectory()).isTrue(); } @@ -436,7 +436,7 @@ public void testDownloadDirectoryNested() throws Exception { ActionResult.Builder result = ActionResult.newBuilder(); result.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); result.addOutputDirectoriesBuilder().setPath("a/bar").setTreeDigest(barTreeDigest); - client.download(result.build(), execRoot, null); + client.download(result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/foo"))).isEqualTo(fooDigest); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/bar/wobble/qux"))).isEqualTo(quxDigest); diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java index b89d6f06b325dc..8240ce1dbc4386 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java @@ -72,6 +72,7 @@ import com.google.devtools.build.lib.remote.RemoteRetrier.ExponentialBackoff; 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.FakeSpawnExecutionContext; import com.google.devtools.build.lib.remote.util.TestUtils; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.util.io.FileOutErr; @@ -127,14 +128,6 @@ public class GrpcRemoteExecutionClientTest { private static final DigestUtil DIGEST_UTIL = new DigestUtil(DigestHashFunction.SHA256); - private static final ArtifactExpander SIMPLE_ARTIFACT_EXPANDER = - new ArtifactExpander() { - @Override - public void expand(Artifact artifact, Collection output) { - output.add(artifact); - } - }; - private final MutableHandlerRegistry serviceRegistry = new MutableHandlerRegistry(); private FileSystem fs; private Path execRoot; @@ -159,70 +152,6 @@ public void expand(Artifact artifact, Collection output) { .build()) .build(); - private final SpawnExecutionContext simplePolicy = - new SpawnExecutionContext() { - @Override - public int getId() { - return 0; - } - - @Override - public void prefetchInputs() { - } - - @Override - public void lockOutputFiles() throws InterruptedException { - throw new UnsupportedOperationException(); - } - - @Override - public boolean speculating() { - return false; - } - - @Override - public MetadataProvider getMetadataProvider() { - return fakeFileCache; - } - - @Override - public ArtifactExpander getArtifactExpander() { - throw new UnsupportedOperationException(); - } - - @Override - public Duration getTimeout() { - return Duration.ZERO; - } - - @Override - public FileOutErr getFileOutErr() { - return outErr; - } - - @Override - public SortedMap getInputMapping( - boolean expandTreeArtifactsInRunfiles) throws IOException { - return new SpawnInputExpander(execRoot, /*strict*/ false) - .getInputMapping( - simpleSpawn, - SIMPLE_ARTIFACT_EXPANDER, - ArtifactPathResolver.IDENTITY, - fakeFileCache, - true); - } - - @Override - public void report(ProgressStatus state, String name) { - // TODO(ulfjack): Test that the right calls are made. - } - - @Override - public MetadataInjector getMetadataInjector() { - throw new UnsupportedOperationException(); - } - }; - @BeforeClass public static void beforeEverything() { retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); @@ -354,7 +283,10 @@ public void getActionResult( } }); - SpawnResult result = client.exec(simpleSpawn, simplePolicy); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); + + SpawnResult result = client.exec(simpleSpawn, policy); assertThat(result.setupSuccess()).isTrue(); assertThat(result.isCacheHit()).isTrue(); assertThat(result.exitCode()).isEqualTo(0); @@ -397,7 +329,9 @@ public void findMissingBlobs( } }); - SpawnResult result = client.exec(simpleSpawn, simplePolicy); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); + SpawnResult result = client.exec(simpleSpawn, policy); assertThat(result.exitCode()).isEqualTo(1); } @@ -436,7 +370,10 @@ public void findMissingBlobs( } }); - client.exec(simpleSpawn, simplePolicy); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); + + client.exec(simpleSpawn, policy); } @Test @@ -460,12 +397,15 @@ public void getActionResult( serviceRegistry.addService( new FakeImmutableCacheByteStreamImpl(stdOutDigest, "stdout", stdErrDigest, "stderr")); - SpawnResult result = client.exec(simpleSpawn, simplePolicy); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); + SpawnResult result = client.exec(simpleSpawn, policy); assertThat(result.setupSuccess()).isTrue(); assertThat(result.exitCode()).isEqualTo(0); assertThat(result.isCacheHit()).isTrue(); assertThat(outErr.outAsLatin1()).isEqualTo("stdout"); assertThat(outErr.errAsLatin1()).isEqualTo("stderr"); + } @Test @@ -485,7 +425,9 @@ public void getActionResult( } }); - SpawnResult result = client.exec(simpleSpawn, simplePolicy); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); + SpawnResult result = client.exec(simpleSpawn, policy); assertThat(result.setupSuccess()).isTrue(); assertThat(result.exitCode()).isEqualTo(0); assertThat(result.isCacheHit()).isTrue(); @@ -622,7 +564,9 @@ public void findMissingBlobs( serviceRegistry.addService( ServerInterceptors.intercept(mockByteStreamImpl, new RequestHeadersValidator())); - SpawnResult result = client.exec(simpleSpawn, simplePolicy); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); + SpawnResult result = client.exec(simpleSpawn, policy); assertThat(result.setupSuccess()).isTrue(); assertThat(result.exitCode()).isEqualTo(0); assertThat(result.isCacheHit()).isFalse(); @@ -777,7 +721,9 @@ public void findMissingBlobs( ArgumentMatchers.>any()); serviceRegistry.addService(mockByteStreamImpl); - SpawnResult result = client.exec(simpleSpawn, simplePolicy); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); + SpawnResult result = client.exec(simpleSpawn, policy); assertThat(result.setupSuccess()).isTrue(); assertThat(result.exitCode()).isEqualTo(0); assertThat(result.isCacheHit()).isFalse(); @@ -885,7 +831,9 @@ public void findMissingBlobs( ArgumentMatchers.>any()); serviceRegistry.addService(mockByteStreamImpl); - SpawnResult result = client.exec(simpleSpawn, simplePolicy); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); + SpawnResult result = client.exec(simpleSpawn, policy); assertThat(result.setupSuccess()).isTrue(); assertThat(result.exitCode()).isEqualTo(0); assertThat(result.isCacheHit()).isFalse(); @@ -918,8 +866,11 @@ public void getActionResult( } }); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); + SpawnExecException expected = - assertThrows(SpawnExecException.class, () -> client.exec(simpleSpawn, simplePolicy)); + assertThrows(SpawnExecException.class, () -> client.exec(simpleSpawn, policy)); assertThat(expected.getSpawnResult().status()) .isEqualTo(SpawnResult.Status.EXECUTION_FAILED_CATASTROPHICALLY); // Ensure we also got back the stack trace. @@ -939,8 +890,10 @@ public void getActionResult( } }); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); ExecException expected = - assertThrows(ExecException.class, () -> client.exec(simpleSpawn, simplePolicy)); + assertThrows(ExecException.class, () -> client.exec(simpleSpawn, policy)); assertThat(expected).hasMessageThat().contains("whoa"); // Error details. // Ensure we also got back the stack trace. assertThat(expected) @@ -997,8 +950,11 @@ public void read(ReadRequest request, StreamObserver responseObser } }); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); + SpawnExecException expected = - assertThrows(SpawnExecException.class, () -> client.exec(simpleSpawn, simplePolicy)); + assertThrows(SpawnExecException.class, () -> client.exec(simpleSpawn, policy)); assertThat(expected.getSpawnResult().status()) .isEqualTo(SpawnResult.Status.REMOTE_CACHE_FAILED); assertThat(expected).hasMessageThat().contains(DIGEST_UTIL.toString(stdOutDigest)); @@ -1058,8 +1014,10 @@ public void read(ReadRequest request, StreamObserver responseObser } }); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); SpawnExecException expected = - assertThrows(SpawnExecException.class, () -> client.exec(simpleSpawn, simplePolicy)); + assertThrows(SpawnExecException.class, () -> client.exec(simpleSpawn, policy)); assertThat(expected.getSpawnResult().status()) .isEqualTo(SpawnResult.Status.REMOTE_CACHE_FAILED); assertThat(expected).hasMessageThat().contains(DIGEST_UTIL.toString(stdOutDigest)); @@ -1150,7 +1108,10 @@ public void findMissingBlobs( } }); - SpawnResult result = client.exec(simpleSpawn, simplePolicy); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); + + SpawnResult result = client.exec(simpleSpawn, policy); assertThat(result.setupSuccess()).isTrue(); assertThat(result.exitCode()).isEqualTo(0); assertThat(result.isCacheHit()).isFalse(); @@ -1246,7 +1207,10 @@ public void findMissingBlobs( } }); - SpawnResult result = client.exec(simpleSpawn, simplePolicy); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(simpleSpawn, fakeFileCache, + execRoot, outErr); + + SpawnResult result = client.exec(simpleSpawn, policy); assertThat(result.setupSuccess()).isTrue(); assertThat(result.exitCode()).isEqualTo(0); assertThat(result.isCacheHit()).isFalse(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index c1e712af73c3b7..ac8feed607d937 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -57,6 +57,7 @@ import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.exec.util.FakeOwner; +import com.google.devtools.build.lib.remote.AbstractRemoteActionCache.OutputFilesLocker; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.options.RemoteOutputsMode; import com.google.devtools.build.lib.remote.util.DigestUtil; @@ -272,13 +273,13 @@ public Void answer(InvocationOnMock invocation) { } }) .when(remoteCache) - .download(actionResult, execRoot, outErr); + .download(eq(actionResult), eq(execRoot), eq(outErr), any()); CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy); assertThat(entry.hasResult()).isTrue(); SpawnResult result = entry.getResult(); // All other methods on RemoteActionCache have side effects, so we verify all of them. - verify(remoteCache).download(actionResult, execRoot, outErr); + verify(remoteCache).download(eq(actionResult), eq(execRoot), eq(outErr), any()); verify(remoteCache, never()) .upload( any(ActionKey.class), @@ -507,7 +508,7 @@ public ActionResult answer(InvocationOnMock invocation) { }); doThrow(new CacheNotFoundException(digest, digestUtil)) .when(remoteCache) - .download(actionResult, execRoot, outErr); + .download(eq(actionResult), eq(execRoot), eq(outErr), any()); CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy); assertThat(entry.hasResult()).isFalse(); @@ -586,7 +587,7 @@ public void testDownloadMinimal() throws Exception { // assert assertThat(cacheHandle.hasResult()).isTrue(); assertThat(cacheHandle.getResult().exitCode()).isEqualTo(0); - verify(remoteCache).downloadMinimal(any(), anyCollection(), any(), any(), any(), any()); + verify(remoteCache).downloadMinimal(any(), anyCollection(), any(), any(), any(), any(), any()); } @Test @@ -609,7 +610,7 @@ public void testDownloadMinimalIoError() throws Exception { ActionResult success = ActionResult.newBuilder().setExitCode(0).build(); when(remoteCache.getCachedActionResult(any())).thenReturn(success); - when(remoteCache.downloadMinimal(any(), anyCollection(), any(), any(), any(), any())) + when(remoteCache.downloadMinimal(any(), anyCollection(), any(), any(), any(), any(), any())) .thenThrow(downloadFailure); // act @@ -617,7 +618,7 @@ public void testDownloadMinimalIoError() throws Exception { // assert assertThat(cacheHandle.hasResult()).isFalse(); - verify(remoteCache).downloadMinimal(any(), anyCollection(), any(), any(), any(), any()); + verify(remoteCache).downloadMinimal(any(), anyCollection(), any(), any(), any(), any(), any()); assertThat(eventHandler.getEvents().size()).isEqualTo(1); Event evt = eventHandler.getEvents().get(0); assertThat(evt.getKind()).isEqualTo(EventKind.WARNING); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index d1fb3de3ce1505..419470616ea851 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -77,6 +77,7 @@ import com.google.devtools.build.lib.remote.options.RemoteOutputsMode; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey; +import com.google.devtools.build.lib.remote.util.FakeSpawnExecutionContext; import com.google.devtools.build.lib.util.ExitCode; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.DigestHashFunction; @@ -196,7 +197,8 @@ public void nonCachableSpawnsShouldNotBeCached_remote() throws Exception { /*outputs=*/ ImmutableList.of(), ResourceSet.ZERO); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); runner.exec(spawn, policy); @@ -236,7 +238,8 @@ public void nonCachableSpawnsShouldNotBeCached_local() throws Exception { /*outputs=*/ ImmutableList.of(), ResourceSet.ZERO); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); runner.exec(spawn, policy); @@ -255,7 +258,8 @@ public void failedLocalActionShouldNotBeUploaded() throws Exception { RemoteSpawnRunner runner = spy(newSpawnRunnerWithoutExecutor()); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnResult res = Mockito.mock(SpawnResult.class); when(res.exitCode()).thenReturn(1); @@ -289,7 +293,8 @@ public void treatFailedCachedActionAsCacheMiss_local() throws Exception { RemoteSpawnRunner runner = spy(newSpawnRunnerWithoutExecutor()); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnResult succeeded = new SpawnResult.Builder() @@ -313,7 +318,7 @@ public void treatFailedCachedActionAsCacheMiss_local() throws Exception { any(), /* uploadLocalResults= */ eq(true)); verify(cache).upload(any(), any(), any(), any(), any(), any()); - verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr)); + verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr), any()); } @Test @@ -332,7 +337,8 @@ public void treatFailedCachedActionAsCacheMiss_remote() throws Exception { .build(); when(executor.executeRemotely(any(ExecuteRequest.class))).thenReturn(succeeded); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); runner.exec(spawn, policy); @@ -355,7 +361,8 @@ public void printWarningIfCacheIsDown() throws Exception { RemoteSpawnRunner runner = newSpawnRunnerWithoutExecutor(reporter); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); when(cache.getCachedActionResult(any(ActionKey.class))) .thenThrow(new IOException("cache down")); @@ -394,7 +401,8 @@ public void noRemoteExecutorFallbackFails() throws Exception { RemoteSpawnRunner runner = newSpawnRunnerWithoutExecutor(); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); when(cache.getCachedActionResult(any(ActionKey.class))).thenReturn(null); @@ -417,7 +425,8 @@ public void remoteCacheErrorFallbackFails() throws Exception { RemoteSpawnRunner runner = newSpawnRunnerWithoutExecutor(); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); when(cache.getCachedActionResult(any(ActionKey.class))).thenThrow(new IOException()); @@ -440,7 +449,8 @@ public void testLocalFallbackFailureRemoteExecutorFailure() throws Exception { when(executor.executeRemotely(any(ExecuteRequest.class))).thenThrow(new IOException()); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); IOException err = new IOException("local execution error"); when(localRunner.exec(eq(spawn), eq(policy))).thenThrow(err); @@ -469,7 +479,8 @@ public void testHumanReadableServerLogsSavedForFailingAction() throws Exception when(cache.downloadFile(eq(logPath), eq(logDigest))).thenReturn(completed); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnResult res = runner.exec(spawn, policy); assertThat(res.status()).isEqualTo(Status.NON_ZERO_EXIT); @@ -498,7 +509,8 @@ public void testHumanReadableServerLogsSavedForFailingActionWithStatus() throws when(cache.downloadFile(eq(logPath), eq(logDigest))).thenReturn(completed); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnResult res = runner.exec(spawn, policy); assertThat(res.status()).isEqualTo(Status.TIMEOUT); @@ -521,13 +533,13 @@ public void testNonHumanReadableServerLogsNotSaved() throws Exception { .build()); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); - + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnResult res = runner.exec(spawn, policy); assertThat(res.status()).isEqualTo(Status.NON_ZERO_EXIT); verify(executor).executeRemotely(any(ExecuteRequest.class)); - verify(cache).download(eq(result), eq(execRoot), any(FileOutErr.class)); + verify(cache).download(eq(result), eq(execRoot), any(FileOutErr.class), any()); verify(cache, never()).downloadFile(any(Path.class), any(Digest.class)); } @@ -547,13 +559,14 @@ public void testServerLogsNotSavedForSuccessfulAction() throws Exception { .build()); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnResult res = runner.exec(spawn, policy); assertThat(res.status()).isEqualTo(Status.SUCCESS); verify(executor).executeRemotely(any(ExecuteRequest.class)); - verify(cache).download(eq(result), eq(execRoot), any(FileOutErr.class)); + verify(cache).download(eq(result), eq(execRoot), any(FileOutErr.class), any()); verify(cache, never()).downloadFile(any(Path.class), any(Digest.class)); } @@ -568,15 +581,16 @@ public void cacheDownloadFailureTriggersRemoteExecution() throws Exception { Exception downloadFailure = new CacheNotFoundException(Digest.getDefaultInstance(), digestUtil); doThrow(downloadFailure) .when(cache) - .download(eq(cachedResult), any(Path.class), any(FileOutErr.class)); + .download(eq(cachedResult), any(Path.class), any(FileOutErr.class), any()); ActionResult execResult = ActionResult.newBuilder().setExitCode(31).build(); ExecuteResponse succeeded = ExecuteResponse.newBuilder().setResult(execResult).build(); when(executor.executeRemotely(any(ExecuteRequest.class))).thenReturn(succeeded); - doNothing().when(cache).download(eq(execResult), any(Path.class), any(FileOutErr.class)); + doNothing().when(cache).download(eq(execResult), any(Path.class), any(FileOutErr.class), any()); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnResult res = runner.exec(spawn, policy); assertThat(res.status()).isEqualTo(Status.NON_ZERO_EXIT); @@ -604,12 +618,13 @@ public void resultsDownloadFailureTriggersRemoteExecutionWithSkipCacheLookup() t Exception downloadFailure = new CacheNotFoundException(Digest.getDefaultInstance(), digestUtil); doThrow(downloadFailure) .when(cache) - .download(eq(cachedResult), any(Path.class), any(FileOutErr.class)); - doNothing().when(cache).download(eq(execResult), any(Path.class), any(FileOutErr.class)); + .download(eq(cachedResult), any(Path.class), any(FileOutErr.class), any()); + doNothing().when(cache).download(eq(execResult), any(Path.class), any(FileOutErr.class), any()); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnResult res = runner.exec(spawn, policy); assertThat(res.status()).isEqualTo(Status.NON_ZERO_EXIT); @@ -647,13 +662,14 @@ public void testRemoteExecutionTimeout() throws Exception { Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnResult res = runner.exec(spawn, policy); assertThat(res.status()).isEqualTo(Status.TIMEOUT); verify(executor).executeRemotely(any(ExecuteRequest.class)); - verify(cache).download(eq(cachedResult), eq(execRoot), any(FileOutErr.class)); + verify(cache).download(eq(cachedResult), eq(execRoot), any(FileOutErr.class), any()); } @Test @@ -680,13 +696,14 @@ public void testRemoteExecutionTimeoutDoesNotTriggerFallback() throws Exception Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnResult res = runner.exec(spawn, policy); assertThat(res.status()).isEqualTo(Status.TIMEOUT); verify(executor).executeRemotely(any(ExecuteRequest.class)); - verify(cache).download(eq(cachedResult), eq(execRoot), any(FileOutErr.class)); + verify(cache).download(eq(cachedResult), eq(execRoot), any(FileOutErr.class), any()); verify(localRunner, never()).exec(eq(spawn), eq(policy)); } @@ -706,14 +723,15 @@ public void testRemoteExecutionCommandFailureDoesNotTriggerFallback() throws Exc Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnResult res = runner.exec(spawn, policy); assertThat(res.status()).isEqualTo(Status.NON_ZERO_EXIT); assertThat(res.exitCode()).isEqualTo(33); verify(executor).executeRemotely(any(ExecuteRequest.class)); - verify(cache, never()).download(eq(cachedResult), eq(execRoot), any(FileOutErr.class)); + verify(cache, never()).download(eq(cachedResult), eq(execRoot), any(FileOutErr.class), any()); verify(localRunner, never()).exec(eq(spawn), eq(policy)); } @@ -730,7 +748,8 @@ public void testExitCode_executorfailure() throws Exception { when(executor.executeRemotely(any(ExecuteRequest.class))).thenThrow(new IOException("reasons")); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnExecException e = assertThrows(SpawnExecException.class, () -> runner.exec(spawn, policy)); assertThat(e.getSpawnResult().exitCode()).isEqualTo(ExitCode.REMOTE_ERROR.getNumericExitCode()); @@ -749,7 +768,8 @@ public void testExitCode_executionfailure() throws Exception { when(cache.getCachedActionResult(any(ActionKey.class))).thenThrow(new IOException("reasons")); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnExecException e = assertThrows(SpawnExecException.class, () -> runner.exec(spawn, policy)); assertThat(e.getSpawnResult().exitCode()).isEqualTo(ExitCode.REMOTE_ERROR.getNumericExitCode()); @@ -797,7 +817,8 @@ public void testMaterializeParamFiles() throws Exception { ImmutableList.of(input), /*outputs=*/ ImmutableList.of(), ResourceSet.ZERO); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); SpawnResult res = runner.exec(spawn, policy); assertThat(res.status()).isEqualTo(Status.SUCCESS); Path paramFile = execRoot.getRelative("out/param_file"); @@ -842,7 +863,8 @@ public void testDownloadMinimalOnCacheHit() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); // act SpawnResult result = runner.exec(spawn, policy); @@ -850,8 +872,9 @@ public void testDownloadMinimalOnCacheHit() throws Exception { assertThat(result.status()).isEqualTo(Status.SUCCESS); // assert - verify(cache).downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any()); - verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr)); + verify(cache).downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any(), + any()); + verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr), any()); } @Test @@ -866,7 +889,8 @@ public void testDownloadMinimalOnCacheMiss() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); // act SpawnResult result = runner.exec(spawn, policy); @@ -875,8 +899,9 @@ public void testDownloadMinimalOnCacheMiss() throws Exception { // assert verify(executor).executeRemotely(any()); - verify(cache).downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any()); - verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr)); + verify(cache).downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any(), + any()); + verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr), any()); } @Test @@ -887,21 +912,23 @@ public void testDownloadMinimalIoError() throws Exception { ActionResult succeededAction = ActionResult.newBuilder().setExitCode(0).build(); when(cache.getCachedActionResult(any(ActionKey.class))).thenReturn(succeededAction); IOException downloadFailure = new IOException("downloadMinimal failed"); - when(cache.downloadMinimal(any(), anyCollection(), any(), any(), any(), any())) + when(cache.downloadMinimal(any(), anyCollection(), any(), any(), any(), any(), any())) .thenThrow(downloadFailure); RemoteSpawnRunner runner = newSpawnRunner(); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); // act SpawnExecException e = assertThrows(SpawnExecException.class, () -> runner.exec(spawn, policy)); assertThat(e.getMessage()).isEqualTo(downloadFailure.getMessage()); // assert - verify(cache).downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any()); - verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr)); + verify(cache).downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any(), + any()); + verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr), any()); } @Test @@ -920,7 +947,8 @@ public void testDownloadTopLevel() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(ImmutableSet.of(topLevelOutput)); Spawn spawn = newSimpleSpawn(topLevelOutput); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn, fakeFileCache, execRoot, + outErr); // act SpawnResult result = runner.exec(spawn, policy); @@ -928,9 +956,9 @@ public void testDownloadTopLevel() throws Exception { assertThat(result.status()).isEqualTo(Status.SUCCESS); // assert - verify(cache).download(eq(succeededAction), any(Path.class), eq(outErr)); + verify(cache).download(eq(succeededAction), any(Path.class), eq(outErr), any()); verify(cache, never()) - .downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any()); + .downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any(), any()); } private static Spawn newSimpleSpawn(Artifact... outputs) { @@ -994,100 +1022,4 @@ private RemoteSpawnRunner newSpawnRunnerWithoutExecutor(Reporter reporter) { reporter, /* topLevelOutputs= */ ImmutableSet.of()); } - - // TODO(buchgr): Extract a common class to be used for testing. - class FakeSpawnExecutionContext implements SpawnExecutionContext { - - private final ArtifactExpander artifactExpander = (artifact, output) -> output.add(artifact); - - private final Spawn spawn; - - FakeSpawnExecutionContext(Spawn spawn) { - this.spawn = spawn; - } - - @Override - public int getId() { - return 0; - } - - @Override - public void prefetchInputs() { - throw new UnsupportedOperationException(); - } - - @Override - public void lockOutputFiles() { - throw new UnsupportedOperationException(); - } - - @Override - public boolean speculating() { - return false; - } - - @Override - public MetadataProvider getMetadataProvider() { - return fakeFileCache; - } - - @Override - public ArtifactExpander getArtifactExpander() { - throw new UnsupportedOperationException(); - } - - @Override - public Duration getTimeout() { - return Duration.ZERO; - } - - @Override - public FileOutErr getFileOutErr() { - return outErr; - } - - @Override - public SortedMap getInputMapping( - boolean expandTreeArtifactsInRunfiles) throws IOException { - return new SpawnInputExpander(execRoot, /*strict*/ false) - .getInputMapping( - spawn, artifactExpander, ArtifactPathResolver.IDENTITY, fakeFileCache, true); - } - - @Override - public void report(ProgressStatus state, String name) { - assertThat(state).isEqualTo(ProgressStatus.EXECUTING); - } - - @Override - public MetadataInjector getMetadataInjector() { - return new MetadataInjector() { - @Override - public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) { - throw new UnsupportedOperationException(); - } - - @Override - public void injectRemoteDirectory( - Artifact.SpecialArtifact output, Map children) { - throw new UnsupportedOperationException(); - } - - @Override - public void markOmitted(ActionInput output) { - throw new UnsupportedOperationException(); - } - - @Override - public void addExpandedTreeOutput(TreeFileArtifact output) { - throw new UnsupportedOperationException(); - } - - @Override - public void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] digest) { - throw new UnsupportedOperationException(); - } - }; - } - } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java index 5b7b0deeb7228e..23ef7b9b9d3ad1 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java @@ -145,7 +145,7 @@ public void testDownloadAllResults() throws Exception { result.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); result.addOutputFilesBuilder().setPath("b/empty").setDigest(emptyDigest); result.addOutputFilesBuilder().setPath("a/bar").setDigest(barDigest).setIsExecutable(true); - client.download(result.build(), execRoot, null); + client.download(result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/foo"))).isEqualTo(fooDigest); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("b/empty"))).isEqualTo(emptyDigest); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/bar"))).isEqualTo(barDigest); @@ -177,7 +177,7 @@ public void testDownloadDirectory() throws Exception { ActionResult.Builder result = ActionResult.newBuilder(); result.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); result.addOutputDirectoriesBuilder().setPath("a/bar").setTreeDigest(barTreeDigest); - client.download(result.build(), execRoot, null); + client.download(result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/foo"))).isEqualTo(fooDigest); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/bar/qux"))).isEqualTo(quxDigest); @@ -195,7 +195,7 @@ public void testDownloadDirectoryEmpty() throws Exception { ActionResult.Builder result = ActionResult.newBuilder(); result.addOutputDirectoriesBuilder().setPath("a/bar").setTreeDigest(barTreeDigest); - client.download(result.build(), execRoot, null); + client.download(result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); assertThat(execRoot.getRelative("a/bar").isDirectory()).isTrue(); } @@ -233,7 +233,7 @@ public void testDownloadDirectoryNested() throws Exception { ActionResult.Builder result = ActionResult.newBuilder(); result.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); result.addOutputDirectoriesBuilder().setPath("a/bar").setTreeDigest(barTreeDigest); - client.download(result.build(), execRoot, null); + client.download(result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/foo"))).isEqualTo(fooDigest); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/bar/wobble/qux"))).isEqualTo(quxDigest); @@ -277,7 +277,7 @@ public void testDownloadDirectoriesWithSameHash() throws Exception { SimpleBlobStoreActionCache client = newClient(map); ActionResult.Builder result = ActionResult.newBuilder(); result.addOutputDirectoriesBuilder().setPath("a/").setTreeDigest(treeDigest); - client.download(result.build(), execRoot, null); + client.download(result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/bar/foo/file"))).isEqualTo(fileDigest); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/foo/file"))).isEqualTo(fileDigest); diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/BUILD b/src/test/java/com/google/devtools/build/lib/remote/util/BUILD index f4764ec3b1f307..c3781362c9c9ef 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/util/BUILD @@ -14,6 +14,8 @@ java_library( name = "util", srcs = glob(["*.java"]), deps = [ + "//src/main/java/com/google/devtools/build/lib:build-base", + "//src/main/java/com/google/devtools/build/lib:io", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/remote", "//src/main/java/com/google/devtools/build/lib/vfs", diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java b/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java new file mode 100644 index 00000000000000..17ede6b813d15a --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java @@ -0,0 +1,143 @@ +// Copyright 2019 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.remote.util; + +import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; +import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.actions.ArtifactPathResolver; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; +import com.google.devtools.build.lib.actions.MetadataProvider; +import com.google.devtools.build.lib.actions.Spawn; +import com.google.devtools.build.lib.actions.cache.MetadataInjector; +import com.google.devtools.build.lib.exec.SpawnInputExpander; +import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus; +import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; +import com.google.devtools.build.lib.util.io.FileOutErr; +import com.google.devtools.build.lib.vfs.FileStatus; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import java.io.IOException; +import java.time.Duration; +import java.util.Map; +import java.util.SortedMap; + +public class FakeSpawnExecutionContext implements SpawnExecutionContext { + + private boolean lockOutputFilesCalled; + + private final ArtifactExpander artifactExpander = (artifact, output) -> output.add(artifact); + + private final Spawn spawn; + private final MetadataProvider metadataProvider; + private final Path execRoot; + private final FileOutErr outErr; + + public FakeSpawnExecutionContext(Spawn spawn, MetadataProvider metadataProvider, + Path execRoot, FileOutErr outErr) { + this.spawn = spawn; + this.metadataProvider = metadataProvider; + this.execRoot = execRoot; + this.outErr = outErr; + } + + public boolean isLockOutputFilesCalled() { + return lockOutputFilesCalled; + } + + @Override + public int getId() { + return 0; + } + + @Override + public void prefetchInputs() { + throw new UnsupportedOperationException(); + } + + @Override + public void lockOutputFiles() { + lockOutputFilesCalled = true; + } + + @Override + public boolean speculating() { + return false; + } + + @Override + public MetadataProvider getMetadataProvider() { + return metadataProvider; + } + + @Override + public ArtifactExpander getArtifactExpander() { + throw new UnsupportedOperationException(); + } + + @Override + public Duration getTimeout() { + return Duration.ZERO; + } + + @Override + public FileOutErr getFileOutErr() { + return outErr; + } + + @Override + public SortedMap getInputMapping( + boolean expandTreeArtifactsInRunfiles) throws IOException { + return new SpawnInputExpander(execRoot, /*strict*/ false) + .getInputMapping( + spawn, artifactExpander, ArtifactPathResolver.IDENTITY, metadataProvider, true); + } + + @Override + public void report(ProgressStatus state, String name) { + // Intentionally left empty. + } + + @Override + public MetadataInjector getMetadataInjector() { + return new MetadataInjector() { + @Override + public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) { + throw new UnsupportedOperationException(); + } + + @Override + public void injectRemoteDirectory( + Artifact.SpecialArtifact output, Map children) { + throw new UnsupportedOperationException(); + } + + @Override + public void markOmitted(ActionInput output) { + throw new UnsupportedOperationException(); + } + + @Override + public void addExpandedTreeOutput(TreeFileArtifact output) { + throw new UnsupportedOperationException(); + } + + @Override + public void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] digest) { + throw new UnsupportedOperationException(); + } + }; + } +} \ No newline at end of file