diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java index 3f4e52265a803a..4ffb4d233f6f9b 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java +++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java @@ -488,6 +488,149 @@ public Digest getDigest() { } } + /** + * A helper class for wrapping an existing {@link SpawnResult} and modifying a subset of its + * methods. + */ + class DelegateSpawnResult implements SpawnResult { + private final SpawnResult delegate; + + public DelegateSpawnResult(SpawnResult delegate) { + this.delegate = delegate; + } + + @Override + public boolean setupSuccess() { + return delegate.setupSuccess(); + } + + @Override + public boolean isCatastrophe() { + return delegate.isCatastrophe(); + } + + @Override + public Status status() { + return delegate.status(); + } + + @Override + public int exitCode() { + return delegate.exitCode(); + } + + @Override + @Nullable + public FailureDetail failureDetail() { + return delegate.failureDetail(); + } + + @Override + @Nullable + public String getExecutorHostName() { + return delegate.getExecutorHostName(); + } + + @Override + public String getRunnerName() { + return delegate.getRunnerName(); + } + + @Override + public String getRunnerSubtype() { + return delegate.getRunnerSubtype(); + } + + @Override + @Nullable + public Instant getStartTime() { + return delegate.getStartTime(); + } + + @Override + public int getWallTimeInMs() { + return delegate.getWallTimeInMs(); + } + + @Override + public int getUserTimeInMs() { + return delegate.getUserTimeInMs(); + } + + @Override + public int getSystemTimeInMs() { + return delegate.getSystemTimeInMs(); + } + + @Override + @Nullable + public Long getNumBlockOutputOperations() { + return delegate.getNumBlockOutputOperations(); + } + + @Override + @Nullable + public Long getNumBlockInputOperations() { + return delegate.getNumBlockInputOperations(); + } + + @Override + @Nullable + public Long getNumInvoluntaryContextSwitches() { + return delegate.getNumInvoluntaryContextSwitches(); + } + + @Override + @Nullable + public Long getMemoryInKb() { + return delegate.getMemoryInKb(); + } + + @Override + public SpawnMetrics getMetrics() { + return delegate.getMetrics(); + } + + @Override + public boolean isCacheHit() { + return delegate.isCacheHit(); + } + + @Override + public String getFailureMessage() { + return delegate.getFailureMessage(); + } + + @Override + @Nullable + public InputStream getInMemoryOutput(ActionInput output) { + return delegate.getInMemoryOutput(output); + } + + @Override + public String getDetailMessage( + String message, boolean catastrophe, boolean forciblyRunRemotely) { + return delegate.getDetailMessage(message, catastrophe, forciblyRunRemotely); + } + + @Override + @Nullable + public MetadataLog getActionMetadataLog() { + return delegate.getActionMetadataLog(); + } + + @Override + public boolean wasRemote() { + return delegate.wasRemote(); + } + + @Override + @Nullable + public Digest getDigest() { + return delegate.getDigest(); + } + } + /** Builder class for {@link SpawnResult}. */ final class Builder { private int exitCode; 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 de0f339ffbf5ed..eefedb7cfa799b 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 @@ -148,8 +148,7 @@ public ImmutableList exec( } SpawnResult spawnResult; ExecException ex = null; - try { - CacheHandle cacheHandle = cache.lookup(spawn, context); + try (CacheHandle cacheHandle = cache.lookup(spawn, context)) { if (cacheHandle.hasResult()) { spawnResult = Preconditions.checkNotNull(cacheHandle.getResult()); } else { @@ -227,7 +226,7 @@ public ImmutableList exec( ? resultMessage : CommandFailureUtils.describeCommandFailure( executionOptions.verboseFailures, cwd, spawn); - throw new SpawnExecException(message, spawnResult, /*forciblyRunRemotely=*/ false); + throw new SpawnExecException(message, spawnResult, /* forciblyRunRemotely= */ false); } return ImmutableList.of(spawnResult); } 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 fbbec5ebcd32f3..63878b884600a8 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD @@ -231,6 +231,7 @@ java_library( deps = [ ":spawn_runner", "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/profiler", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java index 9193114cb8d9e1..02ff41ca4fa56a 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java @@ -18,7 +18,9 @@ import com.google.devtools.build.lib.actions.ForbiddenActionInputException; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnResult; +import com.google.devtools.build.lib.actions.Spawns; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; +import com.google.devtools.build.lib.profiler.SilentCloseable; import java.io.IOException; import java.util.NoSuchElementException; @@ -51,6 +53,9 @@ public boolean willStore() { public void store(SpawnResult result) throws InterruptedException, IOException { // Do nothing. } + + @Override + public void close() {} }; /** @@ -77,6 +82,9 @@ public boolean willStore() { public void store(SpawnResult result) throws InterruptedException, IOException { throw new IllegalStateException(); } + + @Override + public void close() {} }; } @@ -104,7 +112,7 @@ private NoSpawnCache() {} * to the cache after successful execution. Otherwise, if {@link #willStore} returns false, then * {@link #store} throws an {@link IllegalStateException}. */ - interface CacheHandle { + interface CacheHandle extends SilentCloseable { /** Returns whether the cache lookup was successful. */ boolean hasResult(); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index fd11cf0c2c13ea..8f7eeac7790b9b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -17,6 +17,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Strings.isNullOrEmpty; +import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.util.concurrent.Futures.immediateFailedFuture; import static com.google.common.util.concurrent.Futures.immediateFuture; import static com.google.common.util.concurrent.Futures.transform; @@ -62,6 +63,7 @@ import com.google.common.eventbus.Subscribe; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.SettableFuture; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactPathResolver; @@ -128,6 +130,7 @@ import io.reactivex.rxjava3.core.SingleObserver; import io.reactivex.rxjava3.disposables.Disposable; import io.reactivex.rxjava3.schedulers.Schedulers; +import java.io.FileNotFoundException; import java.io.IOException; import java.time.Instant; import java.util.ArrayList; @@ -143,9 +146,11 @@ import java.util.SortedMap; import java.util.TreeMap; import java.util.TreeSet; +import java.util.concurrent.CancellationException; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; import java.util.concurrent.Phaser; import java.util.concurrent.Semaphore; @@ -862,15 +867,12 @@ private void deletePartialDownloadedOutputs( } } - /** - * Copies moves the downloaded outputs from their download location to their declared location. - */ + /** Moves the locally created outputs from their temporary location to their declared location. */ private void moveOutputsToFinalLocation( - List finishedDownloads, Map realToTmpPath) throws IOException { + Iterable localOutputs, Map realToTmpPath) throws IOException { // Move the output files from their temporary name to the actual output file name. Executable // bit is ignored since the file permission will be changed to 0555 after execution. - for (FileMetadata outputFile : finishedDownloads) { - Path realPath = outputFile.path(); + for (Path realPath : localOutputs) { Path tmpPath = Preconditions.checkNotNull(realToTmpPath.get(realPath)); realPath.getParentDirectory().createDirectoryAndParents(); FileSystemUtils.moveFile(tmpPath, realPath); @@ -1269,7 +1271,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re // TODO(chiwang): Stage directories directly ((BazelOutputService) outputService).stageArtifacts(finishedDownloads); } else { - moveOutputsToFinalLocation(finishedDownloads, realToTmpPath); + moveOutputsToFinalLocation( + Iterables.transform(finishedDownloads, FileMetadata::path), realToTmpPath); } List symlinksInDirectories = new ArrayList<>(); @@ -1326,6 +1329,152 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re return null; } + /** An ongoing local execution of a spawn. */ + public static final class LocalExecution { + private final RemoteAction action; + private final SettableFuture spawnResultFuture; + + private LocalExecution(RemoteAction action) { + this.action = action; + this.spawnResultFuture = SettableFuture.create(); + } + + /** + * Creates a new {@link LocalExecution} instance tracking the potential local execution of the + * given {@link RemoteAction} if there is a chance that the same action will be executed by a + * different Spawn. + * + *

This is only done for local (as in, non-remote) execution as remote executors are expected + * to already have deduplication mechanisms for actions in place, perhaps even across different + * builds and clients. + */ + @Nullable + public static LocalExecution createIfDeduplicatable(RemoteAction action) { + if (action.getSpawn().getPathMapper().isNoop()) { + return null; + } + return new LocalExecution(action); + } + + /** + * Signals to all potential consumers of the {@link #spawnResultFuture} that this execution has + * been cancelled and that the result will not be available. + */ + public void cancel() { + spawnResultFuture.cancel(true); + } + } + + /** + * Makes the {@link SpawnResult} available to all parallel {@link Spawn}s for the same {@link + * RemoteAction} waiting for it or notifies them that the spawn failed. + * + * @return Whether the spawn result should be uploaded to the cache. + */ + public boolean commitResultAndDecideWhetherToUpload( + SpawnResult result, @Nullable LocalExecution execution) { + if (result.status().equals(SpawnResult.Status.SUCCESS) && result.exitCode() == 0) { + if (execution != null) { + execution.spawnResultFuture.set(result); + } + return true; + } else { + if (execution != null) { + execution.spawnResultFuture.cancel(true); + } + return false; + } + } + + /** + * Reuses the outputs of a concurrent local execution of the same RemoteAction in a different + * spawn. + * + *

Since each output file is generated by a unique action and actions generally take care to + * run a unique spawn for each output file, this method is only useful with path mapping enabled, + * which allows different spawns in a single build to have the same RemoteAction.ActionKey. + * + * @return The {@link SpawnResult} of the previous execution if it was successful, otherwise null. + */ + @Nullable + public SpawnResult waitForAndReuseOutputs(RemoteAction action, LocalExecution previousExecution) + throws InterruptedException, IOException { + checkState(!shutdown.get(), "shutdown"); + + SpawnResult previousSpawnResult; + try { + previousSpawnResult = previousExecution.spawnResultFuture.get(); + } catch (CancellationException | ExecutionException e) { + if (e.getCause() != null) { + Throwables.throwIfInstanceOf(e.getCause(), InterruptedException.class); + Throwables.throwIfUnchecked(e.getCause()); + } + // The spawn this action was deduplicated against failed due to an exception or + // non-zero exit code. Since it isn't possible to transparently replay its failure for the + // current spawn, we rerun the action instead. + return null; + } + + Preconditions.checkArgument( + action.getActionKey().equals(previousExecution.action.getActionKey())); + + ImmutableMap previousOutputs = + previousExecution.action.getSpawn().getOutputFiles().stream() + .collect(toImmutableMap(output -> execRoot.getRelative(output.getExecPath()), o -> o)); + Map realToTmpPath = new HashMap<>(); + for (String output : action.getCommand().getOutputPathsList()) { + Path sourcePath = + previousExecution + .action + .getRemotePathResolver() + .outputPathToLocalPath(encodeBytestringUtf8(output)); + ActionInput outputArtifact = previousOutputs.get(sourcePath); + Path tmpPath = tempPathGenerator.generateTempPath(); + tmpPath.getParentDirectory().createDirectoryAndParents(); + try { + if (outputArtifact.isDirectory()) { + tmpPath.createDirectory(); + FileSystemUtils.copyTreesBelow(sourcePath, tmpPath, Symlinks.NOFOLLOW); + } else if (outputArtifact.isSymlink()) { + FileSystemUtils.ensureSymbolicLink(tmpPath, sourcePath.readSymbolicLink()); + } else { + FileSystemUtils.copyFile(sourcePath, tmpPath); + } + } catch (FileNotFoundException e) { + // The spawn this action was deduplicated against failed to create an output file. If the + // output is mandatory, we cannot reuse the previous execution. + if (action.getSpawn().isMandatoryOutput(outputArtifact)) { + return null; + } + } + + Path targetPath = + action.getRemotePathResolver().outputPathToLocalPath(encodeBytestringUtf8(output)); + realToTmpPath.put(targetPath, tmpPath); + } + + // TODO: FileOutErr is action-scoped, not spawn-scoped, but this is not a problem for the + // current use case of supporting deduplication of path mapped spawns: + // 1. Starlark and C++ compilation actions always create a single spawn. + // 2. Java compilation actions may run a fallback spawn, but reset the FileOutErr before + // running it. + // If this changes, we will need to introduce a spawn-scoped OutErr. + FileOutErr.dump( + previousExecution.action.getSpawnExecutionContext().getFileOutErr(), + action.getSpawnExecutionContext().getFileOutErr()); + + action + .getSpawnExecutionContext() + .lockOutputFiles( + previousSpawnResult.exitCode(), + previousSpawnResult.getFailureMessage(), + action.getSpawnExecutionContext().getFileOutErr()); + // All outputs are created locally. + moveOutputsToFinalLocation(realToTmpPath.keySet(), realToTmpPath); + + return previousSpawnResult; + } + private boolean shouldDownload(RemoteActionResult result, PathFragment execPath) { if (outputService instanceof BazelOutputService) { return false; @@ -1400,7 +1549,7 @@ UploadManifest buildUploadManifest(RemoteAction action, SpawnResult spawnResult) } /** Upload outputs of a remote action which was executed locally to remote cache. */ - public void uploadOutputs(RemoteAction action, SpawnResult spawnResult) + public void uploadOutputs(RemoteAction action, SpawnResult spawnResult, Runnable onUploadComplete) throws InterruptedException, ExecException { checkState(!shutdown.get(), "shutdown"); checkState( @@ -1431,12 +1580,14 @@ public void onSubscribe(@NonNull Disposable d) { @Override public void onSuccess(@NonNull ActionResult actionResult) { backgroundTaskPhaser.arriveAndDeregister(); + onUploadComplete.run(); } @Override public void onError(@NonNull Throwable e) { backgroundTaskPhaser.arriveAndDeregister(); reportUploadError(e); + onUploadComplete.run(); } }); } else { @@ -1446,6 +1597,8 @@ public void onError(@NonNull Throwable e) { manifest.upload(action.getRemoteActionExecutionContext(), remoteCache, reporter); } catch (IOException e) { reportUploadError(e); + } finally { + onUploadComplete.run(); } } } 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 1845388c3a9716..3224f56ac23684 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 @@ -26,7 +26,6 @@ import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnMetrics; import com.google.devtools.build.lib.actions.SpawnResult; -import com.google.devtools.build.lib.actions.SpawnResult.Status; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.events.Event; @@ -35,9 +34,11 @@ import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.profiler.SilentCloseable; +import com.google.devtools.build.lib.remote.RemoteExecutionService.LocalExecution; import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteActionResult; import com.google.devtools.build.lib.remote.common.BulkTransferException; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; +import com.google.devtools.build.lib.remote.common.RemoteCacheClient; 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.Utils; @@ -45,6 +46,7 @@ import com.google.devtools.build.lib.vfs.Path; import java.io.IOException; import java.util.NoSuchElementException; +import java.util.concurrent.ConcurrentHashMap; /** A remote {@link SpawnCache} implementation. */ @ThreadSafe // If the RemoteActionCache implementation is thread-safe. @@ -55,6 +57,8 @@ final class RemoteSpawnCache implements SpawnCache { private final RemoteExecutionService remoteExecutionService; private final DigestUtil digestUtil; private final boolean verboseFailures; + private final ConcurrentHashMap inFlightExecutions = + new ConcurrentHashMap<>(); RemoteSpawnCache( Path execRoot, @@ -96,7 +100,19 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) context.setDigest(digestUtil.asSpawnLogProto(action.getActionKey())); Profiler prof = Profiler.instance(); + LocalExecution thisExecution = null; if (shouldAcceptCachedResult) { + // With path mapping enabled, different Spawns in a single build can have the same ActionKey. + // When their result isn't in the cache and two of them are scheduled concurrently, neither + // will result in a cache hit before the other finishes and uploads its result, which results + // in unnecessary work. To avoid this, we keep track of in-flight executions as long as their + // results haven't been uploaded to the cache yet and deduplicate all of them against the + // first one. + LocalExecution previousExecution = null; + thisExecution = LocalExecution.createIfDeduplicatable(action); + if (shouldUploadLocalResults && thisExecution != null) { + previousExecution = inFlightExecutions.putIfAbsent(action.getActionKey(), thisExecution); + } // Metadata will be available in context.current() until we detach. // This is done via a thread-local variable. try { @@ -146,9 +162,41 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) remoteExecutionService.report(Event.warn(errorMessage)); } } + if (previousExecution != null) { + Stopwatch fetchTime = Stopwatch.createStarted(); + SpawnResult previousResult; + try (SilentCloseable c = prof.profile(REMOTE_DOWNLOAD, "reuse outputs")) { + previousResult = remoteExecutionService.waitForAndReuseOutputs(action, previousExecution); + } + if (previousResult != null) { + spawnMetrics + .setFetchTimeInMs((int) fetchTime.elapsed().toMillis()) + .setTotalTimeInMs((int) totalTime.elapsed().toMillis()) + .setNetworkTimeInMs((int) action.getNetworkTime().getDuration().toMillis()); + SpawnMetrics buildMetrics = spawnMetrics.build(); + return SpawnCache.success( + new SpawnResult.DelegateSpawnResult(previousResult) { + @Override + public String getRunnerName() { + return "deduplicated"; + } + + @Override + public SpawnMetrics getMetrics() { + return buildMetrics; + } + }); + } + // If we reach here, the previous execution was not successful (it encountered an exception + // or the spawn had an exit code != 0). Since it isn't possible to accurately recreate the + // failure without rerunning the action, we fall back to running the action locally. This + // means that we have introduced an unnecessary wait, but that can only happen in the case + // of a failing build with --keep_going. + } } if (shouldUploadLocalResults) { + final LocalExecution thisExecutionFinal = thisExecution; return new CacheHandle() { @Override public boolean hasResult() { @@ -167,8 +215,8 @@ public boolean willStore() { @Override public void store(SpawnResult result) throws ExecException, InterruptedException { - boolean uploadResults = Status.SUCCESS.equals(result.status()) && result.exitCode() == 0; - if (!uploadResults) { + if (!remoteExecutionService.commitResultAndDecideWhetherToUpload( + result, thisExecutionFinal)) { return; } @@ -185,7 +233,12 @@ public void store(SpawnResult result) throws ExecException, InterruptedException } } - remoteExecutionService.uploadOutputs(action, result); + // As soon as the result is in the cache, actions can get the result from it instead of + // from the first in-flight execution. Not keeping in-flight executions around + // indefinitely is important to avoid excessive memory pressure - Spawns can be very + // large. + remoteExecutionService.uploadOutputs( + action, result, () -> inFlightExecutions.remove(action.getActionKey())); } private void checkForConcurrentModifications() @@ -201,6 +254,13 @@ private void checkForConcurrentModifications() } } } + + @Override + public void close() { + if (thisExecutionFinal != null) { + thisExecutionFinal.cancel(); + } + } }; } else { return SpawnCache.NO_RESULT_NO_STORE; 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 6055d550cc6e4d..a99f18a7c275b9 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 @@ -709,7 +709,7 @@ SpawnResult execLocallyAndUpload( } } - remoteExecutionService.uploadOutputs(action, result); + remoteExecutionService.uploadOutputs(action, result, () -> {}); return result; } diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java index 0aa94084268662..341be193a4ee24 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java @@ -17,7 +17,6 @@ import com.google.common.base.Preconditions; import com.google.devtools.build.lib.actions.ActionInput; -import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ForbiddenActionInputException; import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.Spawn; @@ -76,12 +75,6 @@ default String localPathToOutputPath(ActionInput actionInput) { */ Path outputPathToLocalPath(String outputPath); - /** Resolves the local {@link Path} for the {@link ActionInput}. */ - default Path outputPathToLocalPath(ActionInput actionInput) { - String outputPath = localPathToOutputPath(actionInput.getExecPath()); - return outputPathToLocalPath(outputPath); - } - /** Creates the default {@link RemotePathResolver}. */ static RemotePathResolver createDefault(Path execRoot) { return new DefaultRemotePathResolver(execRoot); @@ -139,11 +132,6 @@ public String localPathToOutputPath(PathFragment execPath) { public Path outputPathToLocalPath(String outputPath) { return execRoot.getRelative(outputPath); } - - @Override - public Path outputPathToLocalPath(ActionInput actionInput) { - return ActionInputHelper.toInputPath(actionInput, execRoot); - } } /** @@ -224,10 +212,6 @@ public Path outputPathToLocalPath(String outputPath) { return getBase().getRelative(outputPath); } - @Override - public Path outputPathToLocalPath(ActionInput actionInput) { - return ActionInputHelper.toInputPath(actionInput, execRoot); - } } /** diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 95124d005b61cd..34ef1aa0010517 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -1649,7 +1649,7 @@ public void uploadOutputs_uploadDirectory_works() throws Exception { // act UploadManifest manifest = service.buildUploadManifest(action, spawnResult); - service.uploadOutputs(action, spawnResult); + service.uploadOutputs(action, spawnResult, () -> {}); // assert ActionResult.Builder expectedResult = ActionResult.newBuilder(); @@ -1696,7 +1696,7 @@ public void uploadOutputs_uploadEmptyDirectory_works() throws Exception { // act UploadManifest manifest = service.buildUploadManifest(action, spawnResult); - service.uploadOutputs(action, spawnResult); + service.uploadOutputs(action, spawnResult, () -> {}); // assert ActionResult.Builder expectedResult = ActionResult.newBuilder(); @@ -1770,7 +1770,7 @@ public void uploadOutputs_uploadNestedDirectory_works() throws Exception { // act UploadManifest manifest = service.buildUploadManifest(action, spawnResult); - service.uploadOutputs(action, spawnResult); + service.uploadOutputs(action, spawnResult, () -> {}); // assert ActionResult.Builder expectedResult = ActionResult.newBuilder(); @@ -1806,7 +1806,7 @@ private void doUploadDanglingSymlink(PathFragment targetPath) throws Exception { // act UploadManifest manifest = service.buildUploadManifest(action, spawnResult); - service.uploadOutputs(action, spawnResult); + service.uploadOutputs(action, spawnResult, () -> {}); // assert ActionResult.Builder expectedResult = ActionResult.newBuilder(); @@ -1858,7 +1858,7 @@ public void uploadOutputs_emptyOutputs_doNotPerformUpload() throws Exception { .build(); // act - service.uploadOutputs(action, spawnResult); + service.uploadOutputs(action, spawnResult, () -> {}); // assert assertThat( @@ -1884,7 +1884,7 @@ public void uploadOutputs_uploadFails_printWarning() throws Exception { .when(cache) .uploadActionResult(any(), any(), any()); - service.uploadOutputs(action, spawnResult); + service.uploadOutputs(action, spawnResult, () -> {}); assertThat(eventHandler.getEvents()).hasSize(1); Event evt = eventHandler.getEvents().get(0); @@ -1909,7 +1909,7 @@ public void uploadOutputs_firesUploadEvents() throws Exception { .setRunnerName("test") .build(); - service.uploadOutputs(action, spawnResult); + service.uploadOutputs(action, spawnResult, () -> {}); assertThat(eventHandler.getPosts()) .containsAtLeast( @@ -1936,7 +1936,7 @@ public void uploadOutputs_missingMandatoryOutputs_dontUpload() throws Exception .setRunnerName("test") .build(); - service.uploadOutputs(action, spawnResult); + service.uploadOutputs(action, spawnResult, () -> {}); // assert assertThat(cache.getNumFindMissingDigests()).isEmpty(); 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 242d6e3e0865d4..b4542501a3d035 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 @@ -18,6 +18,7 @@ import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat; import static com.google.common.util.concurrent.Futures.immediateVoidFuture; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.eq; @@ -52,6 +53,7 @@ import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.actions.SimpleSpawn; +import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.SpawnResult.Status; import com.google.devtools.build.lib.clock.JavaClock; @@ -82,6 +84,7 @@ import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.OutputService; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -98,6 +101,7 @@ import org.junit.runners.JUnit4; import org.mockito.ArgumentCaptor; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; @@ -118,7 +122,7 @@ public class RemoteSpawnCacheTest { private Path execRoot; private TempPathGenerator tempPathGenerator; private SimpleSpawn simpleSpawn; - private FakeActionInputFileCache fakeFileCache; + private SpawnExecutionContext simplePolicy; @Mock private RemoteCache remoteCache; private FileOutErr outErr; @@ -127,104 +131,103 @@ public class RemoteSpawnCacheTest { private Reporter reporter; private RemotePathResolver remotePathResolver; - private final SpawnExecutionContext simplePolicy = - new SpawnExecutionContext() { - @Nullable private com.google.devtools.build.lib.exec.Protos.Digest digest; - - @Override - public int getId() { - return 0; - } - - @Override - public void setDigest(com.google.devtools.build.lib.exec.Protos.Digest digest) { - checkState(this.digest == null); - this.digest = digest; - } - - @Override - @Nullable - public com.google.devtools.build.lib.exec.Protos.Digest getDigest() { - return digest; - } - - @Override - public ListenableFuture prefetchInputs() { - return immediateVoidFuture(); - } - - @Override - public void lockOutputFiles(int exitCode, String errorMessage, FileOutErr outErr) {} - - @Override - public boolean speculating() { - return false; - } - - @Override - public InputMetadataProvider getInputMetadataProvider() { - return fakeFileCache; - } - - @Override - public ArtifactPathResolver getPathResolver() { - return ArtifactPathResolver.forExecRoot(execRoot); - } - - @Override - public ArtifactExpander getArtifactExpander() { - throw new UnsupportedOperationException(); - } - - @Override - public SpawnInputExpander getSpawnInputExpander() { - return new SpawnInputExpander(execRoot); - } - - @Override - public Duration getTimeout() { - return Duration.ZERO; - } - - @Override - public FileOutErr getFileOutErr() { - return outErr; - } - - @Override - public SortedMap getInputMapping( - PathFragment baseDirectory, boolean willAccessRepeatedly) - throws ForbiddenActionInputException { - return getSpawnInputExpander() - .getInputMapping( - simpleSpawn, - treeArtifact -> ImmutableSortedSet.of(), - fakeFileCache, - baseDirectory); - } - - @Override - public void report(ProgressStatus progress) {} - - @Override - public boolean isRewindingEnabled() { - return false; - } - - @Override - public void checkForLostInputs() {} - - @Override - public T getContext(Class identifyingType) { - throw new UnsupportedOperationException(); - } - - @Nullable - @Override - public FileSystem getActionFileSystem() { - return null; - } - }; + private static SpawnExecutionContext createSpawnExecutionContext( + Spawn spawn, Path execRoot, FakeActionInputFileCache fakeFileCache, FileOutErr outErr) { + return new SpawnExecutionContext() { + @Nullable private com.google.devtools.build.lib.exec.Protos.Digest digest; + + @Override + public int getId() { + return 0; + } + + @Override + public void setDigest(com.google.devtools.build.lib.exec.Protos.Digest digest) { + checkState(this.digest == null); + this.digest = digest; + } + + @Override + @Nullable + public com.google.devtools.build.lib.exec.Protos.Digest getDigest() { + return digest; + } + + @Override + public ListenableFuture prefetchInputs() { + return immediateVoidFuture(); + } + + @Override + public void lockOutputFiles(int exitCode, String errorMessage, FileOutErr outErr) {} + + @Override + public boolean speculating() { + return false; + } + + @Override + public InputMetadataProvider getInputMetadataProvider() { + return fakeFileCache; + } + + @Override + public ArtifactPathResolver getPathResolver() { + return ArtifactPathResolver.forExecRoot(execRoot); + } + + @Override + public ArtifactExpander getArtifactExpander() { + throw new UnsupportedOperationException(); + } + + @Override + public SpawnInputExpander getSpawnInputExpander() { + return new SpawnInputExpander(execRoot); + } + + @Override + public Duration getTimeout() { + return Duration.ZERO; + } + + @Override + public FileOutErr getFileOutErr() { + return outErr; + } + + @Override + public SortedMap getInputMapping( + PathFragment baseDirectory, boolean willAccessRepeatedly) + throws ForbiddenActionInputException { + return getSpawnInputExpander() + .getInputMapping( + spawn, treeArtifact -> ImmutableSortedSet.of(), fakeFileCache, baseDirectory); + } + + @Override + public void report(ProgressStatus progress) {} + + @Override + public boolean isRewindingEnabled() { + return false; + } + + @Override + public void checkForLostInputs() {} + + @Override + public T getContext(Class identifyingType) { + throw new UnsupportedOperationException(); + } + + @Nullable + @Override + public FileSystem getActionFileSystem() { + return null; + } + }; + } private static SimpleSpawn simpleSpawnWithExecutionInfo( ImmutableMap executionInfo) { @@ -239,6 +242,26 @@ private static SimpleSpawn simpleSpawnWithExecutionInfo( ResourceSet.ZERO); } + private static SimpleSpawn simplePathMappedSpawn(String configSegment) { + String inputPath = "bazel-bin/%s/bin/input"; + String outputPath = "bazel-bin/%s/bin/output"; + return new SimpleSpawn( + new FakeOwner("Mnemonic", "Progress Message", "//dummy:label"), + ImmutableList.of("cp", inputPath.formatted("cfg"), outputPath.formatted("cfg")), + ImmutableMap.of("VARIABLE", "value"), + ImmutableMap.of(ExecutionRequirements.SUPPORTS_PATH_MAPPING, ""), + /* filesetMappings= */ ImmutableMap.of(), + /* inputs= */ NestedSetBuilder.create( + Order.STABLE_ORDER, ActionInputHelper.fromPath(inputPath.formatted(configSegment))), + /* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /* outputs= */ ImmutableSet.of( + ActionInputHelper.fromPath(outputPath.formatted(configSegment))), + /* mandatoryOutputs= */ null, + ResourceSet.ZERO, + execPath -> + execPath.subFragment(0, 1).getRelative("cfg").getRelative(execPath.subFragment(2))); + } + private RemoteSpawnCache createRemoteSpawnCache() { return remoteSpawnCacheWithOptions(Options.getDefaults(RemoteOptions.class)); } @@ -274,7 +297,7 @@ public final void setUp() throws Exception { execRoot = fs.getPath("/exec/root"); execRoot.createDirectoryAndParents(); tempPathGenerator = new TempPathGenerator(fs.getPath("/execroot/_tmp/actions/remote")); - fakeFileCache = new FakeActionInputFileCache(execRoot); + FakeActionInputFileCache fakeFileCache = new FakeActionInputFileCache(execRoot); simpleSpawn = simpleSpawnWithExecutionInfo(ImmutableMap.of()); Path stdout = fs.getPath("/tmp/stdout"); @@ -287,6 +310,7 @@ public final void setUp() throws Exception { reporter.addHandler(eventHandler); remotePathResolver = RemotePathResolver.createDefault(execRoot); + simplePolicy = createSpawnExecutionContext(simpleSpawn, execRoot, fakeFileCache, outErr); fakeFileCache.createScratchInput(simpleSpawn.getInputFiles().getSingleton(), "xyz"); } @@ -339,7 +363,7 @@ public CachedActionResult answer(InvocationOnMock invocation) { verify(service) .downloadOutputs( any(), eq(RemoteActionResult.createFromCache(CachedActionResult.remote(actionResult)))); - verify(service, never()).uploadOutputs(any(), any()); + verify(service, never()).uploadOutputs(any(), any(), any()); assertThat(result.getDigest()) .isEqualTo(digestUtil.asSpawnLogProto(actionKeyCaptor.getValue())); assertThat(result.setupSuccess()).isTrue(); @@ -370,9 +394,9 @@ public void cacheMiss() throws Exception { .setStatus(Status.SUCCESS) .setRunnerName("test") .build(); - doNothing().when(service).uploadOutputs(any(), any()); + doNothing().when(service).uploadOutputs(any(), any(), any()); entry.store(result); - verify(service).uploadOutputs(any(), any()); + verify(service).uploadOutputs(any(), any(), any()); } @Test @@ -536,7 +560,7 @@ public void failedActionsAreNotUploaded() throws Exception { .setRunnerName("test") .build(); entry.store(result); - verify(service, never()).uploadOutputs(any(), any()); + verify(service, never()).uploadOutputs(any(), any(), any()); } @Test @@ -559,9 +583,9 @@ public void printWarningIfDownloadFails() throws Exception { .setRunnerName("test") .build(); - doNothing().when(service).uploadOutputs(any(), any()); + doNothing().when(service).uploadOutputs(any(), any(), any()); entry.store(result); - verify(service).uploadOutputs(any(), eq(result)); + verify(service).uploadOutputs(any(), eq(result), any()); assertThat(eventHandler.getEvents()).hasSize(1); Event evt = eventHandler.getEvents().get(0); @@ -607,9 +631,9 @@ public CachedActionResult answer(InvocationOnMock invocation) { .setRunnerName("test") .build(); - doNothing().when(service).uploadOutputs(any(), any()); + doNothing().when(service).uploadOutputs(any(), any(), any()); entry.store(result); - verify(service).uploadOutputs(any(), eq(result)); + verify(service).uploadOutputs(any(), eq(result), any()); assertThat(eventHandler.getEvents()).isEmpty(); // no warning is printed. } @@ -683,4 +707,141 @@ public void testDownloadMinimalIoError() throws Exception { assertThat(evt.getKind()).isEqualTo(EventKind.WARNING); assertThat(evt.getMessage()).contains(downloadFailure.getMessage()); } + + @Test + public void pathMappedActionIsDeduplicated() throws Exception { + // arrange + RemoteSpawnCache cache = createRemoteSpawnCache(); + + SimpleSpawn firstSpawn = simplePathMappedSpawn("k8-fastbuild"); + FakeActionInputFileCache firstFakeFileCache = new FakeActionInputFileCache(execRoot); + firstFakeFileCache.createScratchInput(firstSpawn.getInputFiles().getSingleton(), "xyz"); + SpawnExecutionContext firstPolicy = + createSpawnExecutionContext(firstSpawn, execRoot, firstFakeFileCache, outErr); + + SimpleSpawn secondSpawn = simplePathMappedSpawn("k8-opt"); + FakeActionInputFileCache secondFakeFileCache = new FakeActionInputFileCache(execRoot); + secondFakeFileCache.createScratchInput(secondSpawn.getInputFiles().getSingleton(), "xyz"); + SpawnExecutionContext secondPolicy = + createSpawnExecutionContext(secondSpawn, execRoot, secondFakeFileCache, outErr); + + RemoteExecutionService remoteExecutionService = cache.getRemoteExecutionService(); + Mockito.doCallRealMethod().when(remoteExecutionService).waitForAndReuseOutputs(any(), any()); + // Simulate a very slow upload to the remote cache to ensure that the second spawn is + // deduplicated rather than a cache hit. This is a slight hack, but also avoid introducing + // concurrency to this test. + Mockito.doNothing().when(remoteExecutionService).uploadOutputs(any(), any(), any()); + + // act + try (CacheHandle firstCacheHandle = cache.lookup(firstSpawn, firstPolicy)) { + FileSystemUtils.writeContent( + fs.getPath("/exec/root/bazel-bin/k8-fastbuild/bin/output"), UTF_8, "hello"); + firstCacheHandle.store( + new SpawnResult.Builder() + .setExitCode(0) + .setStatus(Status.SUCCESS) + .setRunnerName("test") + .build()); + } + CacheHandle secondCacheHandle = cache.lookup(secondSpawn, secondPolicy); + + // assert + assertThat(secondCacheHandle.hasResult()).isTrue(); + assertThat(secondCacheHandle.getResult().getRunnerName()).isEqualTo("deduplicated"); + assertThat( + FileSystemUtils.readContent( + fs.getPath("/exec/root/bazel-bin/k8-opt/bin/output"), UTF_8)) + .isEqualTo("hello"); + assertThat(secondCacheHandle.willStore()).isFalse(); + } + + @Test + public void deduplicatedActionWithNonZeroExitCodeIsACacheMiss() throws Exception { + // arrange + RemoteSpawnCache cache = createRemoteSpawnCache(); + + SimpleSpawn firstSpawn = simplePathMappedSpawn("k8-fastbuild"); + FakeActionInputFileCache firstFakeFileCache = new FakeActionInputFileCache(execRoot); + firstFakeFileCache.createScratchInput(firstSpawn.getInputFiles().getSingleton(), "xyz"); + SpawnExecutionContext firstPolicy = + createSpawnExecutionContext(firstSpawn, execRoot, firstFakeFileCache, outErr); + + SimpleSpawn secondSpawn = simplePathMappedSpawn("k8-opt"); + FakeActionInputFileCache secondFakeFileCache = new FakeActionInputFileCache(execRoot); + secondFakeFileCache.createScratchInput(secondSpawn.getInputFiles().getSingleton(), "xyz"); + SpawnExecutionContext secondPolicy = + createSpawnExecutionContext(secondSpawn, execRoot, secondFakeFileCache, outErr); + + RemoteExecutionService remoteExecutionService = cache.getRemoteExecutionService(); + Mockito.doCallRealMethod().when(remoteExecutionService).waitForAndReuseOutputs(any(), any()); + // Simulate a very slow upload to the remote cache to ensure that the second spawn is + // deduplicated rather than a cache hit. This is a slight hack, but also avoid introducing + // concurrency to this test. + Mockito.doNothing().when(remoteExecutionService).uploadOutputs(any(), any(), any()); + + // act + try (CacheHandle firstCacheHandle = cache.lookup(firstSpawn, firstPolicy)) { + FileSystemUtils.writeContent( + fs.getPath("/exec/root/bazel-bin/k8-fastbuild/bin/output"), UTF_8, "hello"); + firstCacheHandle.store( + new SpawnResult.Builder() + .setExitCode(1) + .setStatus(Status.NON_ZERO_EXIT) + .setFailureDetail( + FailureDetail.newBuilder() + .setMessage("test spawn failed") + .setSpawn( + FailureDetails.Spawn.newBuilder() + .setCode(FailureDetails.Spawn.Code.NON_ZERO_EXIT)) + .build()) + .setRunnerName("test") + .build()); + } + CacheHandle secondCacheHandle = cache.lookup(secondSpawn, secondPolicy); + + // assert + assertThat(secondCacheHandle.hasResult()).isFalse(); + assertThat(secondCacheHandle.willStore()).isTrue(); + } + + @Test + public void deduplicatedActionWithMissingOutputIsACacheMiss() throws Exception { + // arrange + RemoteSpawnCache cache = createRemoteSpawnCache(); + + SimpleSpawn firstSpawn = simplePathMappedSpawn("k8-fastbuild"); + FakeActionInputFileCache firstFakeFileCache = new FakeActionInputFileCache(execRoot); + firstFakeFileCache.createScratchInput(firstSpawn.getInputFiles().getSingleton(), "xyz"); + SpawnExecutionContext firstPolicy = + createSpawnExecutionContext(firstSpawn, execRoot, firstFakeFileCache, outErr); + + SimpleSpawn secondSpawn = simplePathMappedSpawn("k8-opt"); + FakeActionInputFileCache secondFakeFileCache = new FakeActionInputFileCache(execRoot); + secondFakeFileCache.createScratchInput(secondSpawn.getInputFiles().getSingleton(), "xyz"); + SpawnExecutionContext secondPolicy = + createSpawnExecutionContext(secondSpawn, execRoot, secondFakeFileCache, outErr); + + RemoteExecutionService remoteExecutionService = cache.getRemoteExecutionService(); + Mockito.doCallRealMethod().when(remoteExecutionService).waitForAndReuseOutputs(any(), any()); + // Simulate a very slow upload to the remote cache to ensure that the second spawn is + // deduplicated rather than a cache hit. This is a slight hack, but also avoid introducing + // concurrency to this test. + Mockito.doNothing().when(remoteExecutionService).uploadOutputs(any(), any(), any()); + + // act + try (CacheHandle firstCacheHandle = cache.lookup(firstSpawn, firstPolicy)) { + // Do not create the output. + firstCacheHandle.store( + new SpawnResult.Builder() + .setExitCode(0) + .setStatus(Status.SUCCESS) + .setRunnerName("test") + .build()); + } + CacheHandle secondCacheHandle = cache.lookup(secondSpawn, secondPolicy); + + // assert + assertThat(secondCacheHandle.hasResult()).isFalse(); + assertThat(secondCacheHandle.willStore()).isTrue(); + } } 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 48136b8c92ece2..b7a3d6a506d319 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 @@ -242,7 +242,7 @@ public void nonCachableSpawnsShouldNotBeCached_remote() throws Exception { // TODO(olaola): verify that the uploaded action has the doNotCache set. verify(service, never()).lookupCache(any()); - verify(service, never()).uploadOutputs(any(), any()); + verify(service, never()).uploadOutputs(any(), any(), any()); verifyNoMoreInteractions(localRunner); } @@ -317,7 +317,7 @@ public void cachableSpawnsShouldBeCached_localFallback() throws Exception { RemoteSpawnRunner runner = spy(newSpawnRunner()); RemoteExecutionService service = runner.getRemoteExecutionService(); - doNothing().when(service).uploadOutputs(any(), any()); + doNothing().when(service).uploadOutputs(any(), any(), any()); // Throw an IOException to trigger the local fallback. when(executor.executeRemotely( @@ -343,7 +343,7 @@ public void cachableSpawnsShouldBeCached_localFallback() throws Exception { verify(localRunner).exec(eq(spawn), eq(policy)); verify(runner) .execLocallyAndUpload(any(), eq(spawn), eq(policy), /* uploadLocalResults= */ eq(true)); - verify(service).uploadOutputs(any(), eq(res)); + verify(service).uploadOutputs(any(), eq(res), any()); } @Test @@ -376,7 +376,7 @@ public void failedLocalActionShouldNotBeUploaded() throws Exception { verify(localRunner).exec(eq(spawn), eq(policy)); verify(runner) .execLocallyAndUpload(any(), eq(spawn), eq(policy), /* uploadLocalResults= */ eq(true)); - verify(service, never()).uploadOutputs(any(), any()); + verify(service, never()).uploadOutputs(any(), any(), any()); } @Test @@ -403,7 +403,7 @@ public void treatFailedCachedActionAsCacheMiss_local() throws Exception { any(ExecuteRequest.class), any(OperationObserver.class))) .thenThrow(IOException.class); - doNothing().when(service).uploadOutputs(any(), any()); + doNothing().when(service).uploadOutputs(any(), any(), any()); Spawn spawn = newSimpleSpawn(); SpawnExecutionContext policy = getSpawnContext(spawn); @@ -421,7 +421,7 @@ public void treatFailedCachedActionAsCacheMiss_local() throws Exception { verify(localRunner).exec(eq(spawn), eq(policy)); verify(runner) .execLocallyAndUpload(any(), eq(spawn), eq(policy), /* uploadLocalResults= */ eq(true)); - verify(service).uploadOutputs(any(), eq(result)); + verify(service).uploadOutputs(any(), eq(result), any()); verify(service, never()).downloadOutputs(any(), any()); } diff --git a/src/test/shell/bazel/path_mapping_test.sh b/src/test/shell/bazel/path_mapping_test.sh index 560625b8993106..e2ed5dcb9a7203 100755 --- a/src/test/shell/bazel/path_mapping_test.sh +++ b/src/test/shell/bazel/path_mapping_test.sh @@ -46,6 +46,15 @@ source "$(rlocation "io_bazel/src/test/shell/bazel/remote_helpers.sh")" \ source "$(rlocation "io_bazel/src/test/shell/bazel/remote/remote_utils.sh")" \ || { echo "remote_utils.sh not found!" >&2; exit 1; } +case "$(uname -s | tr [:upper:] [:lower:])" in +msys*|mingw*|cygwin*) + function is_windows() { true; } + ;; +*) + function is_windows() { false; } + ;; +esac + function set_up() { start_worker @@ -546,4 +555,269 @@ EOF expect_log ' 3 remote cache hit' } +function test_path_stripping_deduplicated_action() { + if is_windows; then + echo "Skipping test_path_stripping_deduplicated_action on Windows as it requires sandboxing" + return + fi + + mkdir rules + touch rules/BUILD + cat > rules/defs.bzl <<'EOF' +def _slow_rule_impl(ctx): + out_file = ctx.actions.declare_file(ctx.attr.name + "_file") + out_dir = ctx.actions.declare_directory(ctx.attr.name + "_dir") + out_symlink = ctx.actions.declare_symlink(ctx.attr.name + "_symlink") + outs = [out_file, out_dir, out_symlink] + args = ctx.actions.args().add_all(outs, expand_directories = False) + ctx.actions.run_shell( + outputs = outs, + command = """ + # Sleep to ensure that two actions are scheduled in parallel. + sleep 3 + + echo "Hello, stdout!" + >&2 echo "Hello, stderr!" + + echo 'echo "Hello, file!"' > $1 + chmod +x $1 + echo 'Hello, dir!' > $2/file + ln -s $(basename $2)/file $3 + """, + arguments = [args], + execution_requirements = {"supports-path-mapping": ""}, + ) + return [ + DefaultInfo(files = depset(outs)), + ] + +slow_rule = rule(_slow_rule_impl) +EOF + + mkdir -p pkg + cat > pkg/BUILD <<'EOF' +load("//rules:defs.bzl", "slow_rule") + +slow_rule(name = "my_rule") + +COMMAND = """ +function validate() { + # Sorted by file name. + local -r dir=$$1 + local -r file=$$2 + local -r symlink=$$3 + + [[ $$($$file) == "Hello, file!" ]] || exit 1 + + [[ -d $$dir ]] || exit 1 + [[ $$(cat $$dir/file) == "Hello, dir!" ]] || exit 1 + + [[ -L $$symlink ]] || exit 1 + [[ $$(cat $$symlink) == "Hello, dir!" ]] || exit 1 +} +validate $(execpaths :my_rule) +touch $@ +""" + +genrule( + name = "gen_exec", + outs = ["out_exec"], + cmd = COMMAND, + tools = [":my_rule"], +) + +genrule( + name = "gen_target", + outs = ["out_target"], + cmd = COMMAND, + srcs = [":my_rule"], +) +EOF + + bazel build \ + --experimental_output_paths=strip \ + --remote_cache=grpc://localhost:${worker_port} \ + //pkg:all &> $TEST_log || fail "build failed unexpectedly" + # The first slow_write action plus two genrules. + expect_log '3 \(linux\|darwin\|processwrapper\)-sandbox' + expect_log '1 deduplicated' + + # Even though the spawn is only executed once, its stdout/stderr should be + # printed as if it wasn't deduplicated. + expect_log_once 'INFO: From Action pkg/my_rule_file:' + expect_log_once 'INFO: From Action pkg/my_rule_file \[for tool\]:' + expect_log_n 'Hello, stderr!' 2 + expect_log_n 'Hello, stdout!' 2 + + bazel clean || fail "clean failed unexpectedly" + bazel build \ + --experimental_output_paths=strip \ + --remote_cache=grpc://localhost:${worker_port} \ + //pkg:all &> $TEST_log || fail "build failed unexpectedly" + # The cache is checked before deduplication. + expect_log '4 remote cache hit' +} + +function test_path_stripping_deduplicated_action_output_not_created() { + if is_windows; then + echo "Skipping test_path_stripping_deduplicated_action_output_not_created on Windows as it requires sandboxing" + return + fi + + mkdir rules + touch rules/BUILD + cat > rules/defs.bzl <<'EOF' +def _slow_rule_impl(ctx): + out_file = ctx.actions.declare_file(ctx.attr.name + "_file") + outs = [out_file] + args = ctx.actions.args().add_all(outs) + ctx.actions.run_shell( + outputs = outs, + command = """ + # Sleep to ensure that two actions are scheduled in parallel. + sleep 3 + + echo "Hello, stdout!" + >&2 echo "Hello, stderr!" + + # Do not create the output file + """, + arguments = [args], + execution_requirements = {"supports-path-mapping": ""}, + ) + return [ + DefaultInfo(files = depset(outs)), + ] + +slow_rule = rule(_slow_rule_impl) +EOF + + mkdir -p pkg + cat > pkg/BUILD <<'EOF' +load("//rules:defs.bzl", "slow_rule") + +slow_rule(name = "my_rule") + +COMMAND = """ +[[ $$($(execpath :my_rule)) == "Hello, file!" ]] || exit 1 +touch $@ +""" + +genrule( + name = "gen_exec", + outs = ["out_exec"], + cmd = COMMAND, + tools = [":my_rule"], +) + +genrule( + name = "gen_target", + outs = ["out_target"], + cmd = COMMAND, + srcs = [":my_rule"], +) +EOF + + bazel build \ + --experimental_output_paths=strip \ + --remote_cache=grpc://localhost:${worker_port} \ + --keep_going \ + //pkg:all &> $TEST_log && fail "build succeeded unexpectedly" + # The second action runs normally after discovering that the first one failed + # to create the output file. + expect_log '2 \(linux\|darwin\|processwrapper\)-sandbox' + expect_not_log '[0-9] deduplicated' + + expect_log 'Action pkg/my_rule_file failed:' + expect_log 'Action pkg/my_rule_file \[for tool\] failed:' + # Remote cache warning. + expect_log 'Expected output pkg/my_rule_file was not created locally.' + + expect_log_once 'INFO: From Action pkg/my_rule_file:' + expect_log_once 'INFO: From Action pkg/my_rule_file \[for tool\]:' + expect_log_n 'Hello, stderr!' 2 + expect_log_n 'Hello, stdout!' 2 +} + +function test_path_stripping_deduplicated_action_non_zero_exit_code() { + if is_windows; then + echo "Skipping test_path_stripping_deduplicated_action_non_zero_exit_code on Windows as it requires sandboxing" + return + fi + + mkdir rules + touch rules/BUILD + cat > rules/defs.bzl <<'EOF' +def _slow_rule_impl(ctx): + out_file = ctx.actions.declare_file(ctx.attr.name + "_file") + outs = [out_file] + args = ctx.actions.args().add_all(outs) + ctx.actions.run_shell( + outputs = outs, + command = """ + # Sleep to ensure that two actions are scheduled in parallel. + sleep 3 + + echo "Hello, stdout!" + >&2 echo "Hello, stderr!" + + # Create the output file, but with a non-zero exit code. + echo 'echo "Hello, file!"' > $1 + exit 1 + """, + arguments = [args], + execution_requirements = {"supports-path-mapping": ""}, + ) + return [ + DefaultInfo(files = depset(outs)), + ] + +slow_rule = rule(_slow_rule_impl) +EOF + + mkdir -p pkg + cat > pkg/BUILD <<'EOF' +load("//rules:defs.bzl", "slow_rule") + +slow_rule(name = "my_rule") + +COMMAND = """ +[[ $$($(execpath :my_rule)) == "Hello, file!" ]] || exit 1 +touch $@ +""" + +genrule( + name = "gen_exec", + outs = ["out_exec"], + cmd = COMMAND, + tools = [":my_rule"], +) + +genrule( + name = "gen_target", + outs = ["out_target"], + cmd = COMMAND, + srcs = [":my_rule"], +) +EOF + + bazel build \ + --experimental_output_paths=strip \ + --remote_cache=grpc://localhost:${worker_port} \ + --keep_going \ + //pkg:all &> $TEST_log && fail "build succeeded unexpectedly" + # Failing actions are not deduplicated. + expect_not_log '[0-9] deduplicated' + + expect_log 'Action pkg/my_rule_file failed:' + expect_log 'Action pkg/my_rule_file \[for tool\] failed:' + + # The first execution emits stdout/stderr, the second doesn't. + # stdout/stderr are emitted as part of the failing action error, not as an + # info. + expect_not_log 'INFO: From Action pkg/my_rule_file' + expect_log_n 'Hello, stderr!' 2 + expect_log_n 'Hello, stdout!' 2 +} + run_suite "path mapping tests"