diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java index 95c02ee92b8421..5eab4dbd702aba 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java @@ -119,4 +119,14 @@ default String getProgressMessage(RepositoryMapping mainRepositoryMapping) { default boolean mayInsensitivelyPropagateInputs() { return false; } + + /** + * Returns true if the action may modify spawn outputs after the spawn has executed. + * + *

If this returns true, any kind of spawn output caching or reuse needs to happen + * synchronously directly after the spawn execution. + */ + default boolean mayModifySpawnOutputsAfterExecution() { + return false; + } } 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 60c14b8627550a..c38ed59c551221 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 @@ -29,7 +29,6 @@ import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.protobuf.ByteString; import java.io.IOException; -import java.io.InputStream; import java.time.Instant; import java.util.Locale; import javax.annotation.Nullable; @@ -38,7 +37,7 @@ @SuppressWarnings("GoodTime") // Use ints instead of Durations to improve build time (cl/505728570) public interface SpawnResult { - int POSIX_TIMEOUT_EXIT_CODE = /*SIGNAL_BASE=*/ 128 + /*SIGALRM=*/ 14; + int POSIX_TIMEOUT_EXIT_CODE = /* SIGNAL_BASE= */ 128 + /* SIGALRM= */ 14; /** The status of the attempted Spawn execution. */ enum Status { @@ -262,14 +261,11 @@ default String getFailureMessage() { * ExecutionRequirements#REMOTE_EXECUTION_INLINE_OUTPUTS}. */ @Nullable - default InputStream getInMemoryOutput(ActionInput output) { + default ByteString getInMemoryOutput(ActionInput output) { return null; } - String getDetailMessage( - String message, - boolean catastrophe, - boolean forciblyRunRemotely); + String getDetailMessage(String message, boolean catastrophe, boolean forciblyRunRemotely); /** Returns a file path to the action metadata log. */ @Nullable @@ -434,11 +430,8 @@ public String getFailureMessage() { @Override public String getDetailMessage( - String message, - boolean catastrophe, - boolean forciblyRunRemotely) { - TerminationStatus status = new TerminationStatus( - exitCode(), status() == Status.TIMEOUT); + String message, boolean catastrophe, boolean forciblyRunRemotely) { + TerminationStatus status = new TerminationStatus(exitCode(), status() == Status.TIMEOUT); String reason = "(" + status.toShortString() + ")"; // e.g. "(Exit 1)" String explanation = Strings.isNullOrEmpty(message) ? "" : ": " + message; @@ -457,17 +450,18 @@ public String getDetailMessage( explanation += " (Remote action was terminated due to Out of Memory.)"; } if (status() != Status.TIMEOUT && forciblyRunRemotely) { - explanation += " Action tagged as local was forcibly run remotely and failed - it's " - + "possible that the action simply doesn't work remotely"; + explanation += + " Action tagged as local was forcibly run remotely and failed - it's " + + "possible that the action simply doesn't work remotely"; } return reason + explanation; } @Nullable @Override - public InputStream getInMemoryOutput(ActionInput output) { + public ByteString getInMemoryOutput(ActionInput output) { if (inMemoryOutputFile != null && inMemoryOutputFile.equals(output)) { - return inMemoryContents.newInput(); + return inMemoryContents; } return null; } @@ -488,6 +482,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 ByteString 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/analysis/actions/StarlarkAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java index 2aa701010771dc..1f32ec35e46d6f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java @@ -47,6 +47,7 @@ import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.StarlarkAction.Code; import com.google.errorprone.annotations.CanIgnoreReturnValue; +import com.google.protobuf.ByteString; import java.io.BufferedReader; import java.io.FileNotFoundException; import java.io.IOException; @@ -315,9 +316,9 @@ private InputStream getUnusedInputListInputStream( // Note: SpawnActionContext guarantees that the first list entry exists and corresponds to the // executed spawn. Artifact unusedInputsListArtifact = unusedInputsList.get(); - InputStream inputStream = spawnResults.get(0).getInMemoryOutput(unusedInputsListArtifact); - if (inputStream != null) { - return inputStream; + ByteString content = spawnResults.get(0).getInMemoryOutput(unusedInputsListArtifact); + if (content != null) { + return content.newInput(); } // Fallback to reading from disk. try { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index 52c4ab456d1afe..01cf8da909fabe 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java @@ -309,6 +309,16 @@ private static ImmutableSet nonNullAsSet(Artifact... artifacts) { this.isExecutedOnWindows = isExecutedOnWindows; } + @Override + public boolean mayModifySpawnOutputsAfterExecution() { + // Test actions modify test spawn outputs after execution: + // - if there are multiple attempts (unavoidable); + // - in all cases due to appending any stray stderr output to the test log in + // StandaloneTestStrategy. + // TODO: Get rid of the second case and only return true if there are multiple attempts. + return true; + } + public boolean isExecutedOnWindows() { return isExecutedOnWindows; } 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 10e4258f4b482e..e31c9604f96af8 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 @@ -221,7 +221,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 166ba6963ec07e..cfd94913148ad0 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD @@ -225,6 +225,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 9b4680294bfb82..694e0794616da0 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,6 +18,7 @@ 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 java.io.Closeable; import java.io.IOException; diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java index 5011d3758f9a5d..2fb63d7272e517 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java @@ -52,6 +52,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.lib.vfs.SyscallCache; +import com.google.protobuf.ByteString; import java.io.IOException; import java.io.InputStream; import java.util.Collection; @@ -351,6 +352,7 @@ public Collection extractInclusions( * Otherwise "null" * @throws ExecException if scanning fails */ + @Nullable private static InputStream spawnGrep( Artifact input, PathFragment outputExecPath, @@ -412,7 +414,11 @@ private static InputStream spawnGrep( } SpawnResult result = Iterables.getLast(results); - return result.getInMemoryOutput(output); + ByteString includesContent = result.getInMemoryOutput(output); + if (includesContent != null) { + return includesContent.newInput(); + } + return null; } private static void dump(ActionExecutionContext fromContext, ActionExecutionContext toContext) { 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 ccefcc55a40d21..aa5c4ae1c5ab36 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; @@ -130,6 +132,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; @@ -145,9 +148,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; @@ -927,15 +932,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); @@ -1336,7 +1338,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<>(); @@ -1399,6 +1402,224 @@ 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 final Phaser spawnResultConsumers = + new Phaser(1) { + @Override + protected boolean onAdvance(int phase, int registeredParties) { + // We only use a single phase. + return true; + } + }; + + 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); + } + + /** + * Attempts to register a thread waiting for the {@link #spawnResultFuture} to become available + * and returns true if successful. + * + *

Every call to this method must be matched by a call to {@link #unregister()} via + * try-finally. + * + *

This always returns true for actions that do not modify their spawns' outputs after + * execution. + */ + public boolean registerForOutputReuse() { + // We only use a single phase. + return spawnResultConsumers.register() == 0; + } + + /** + * Unregisters a thread waiting for the {@link #spawnResultFuture}, either after successful + * reuse of the outputs or upon failure. + */ + public void unregister() { + spawnResultConsumers.arriveAndDeregister(); + } + + /** + * Waits for all potential consumers of the {@link #spawnResultFuture} to be done with their + * output reuse. + */ + public void awaitAllOutputReuse() { + spawnResultConsumers.arriveAndAwaitAdvance(); + } + + /** + * 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<>(); + ByteString inMemoryOutputContent = null; + String inMemoryOutputPath = null; + try { + for (String output : action.getCommand().getOutputPathsList()) { + String reencodedOutput = encodeBytestringUtf8(output); + Path sourcePath = + previousExecution + .action + .getRemotePathResolver() + .outputPathToLocalPath(reencodedOutput); + ActionInput outputArtifact = previousOutputs.get(sourcePath); + Path targetPath = action.getRemotePathResolver().outputPathToLocalPath(reencodedOutput); + inMemoryOutputContent = previousSpawnResult.getInMemoryOutput(outputArtifact); + if (inMemoryOutputContent != null) { + inMemoryOutputPath = targetPath.relativeTo(execRoot).getPathString(); + continue; + } + 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); + } + realToTmpPath.put(targetPath, 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; + } + } + } + + // 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); + } catch (InterruptedException | IOException e) { + // Delete any copied output files. + try { + for (Path tmpPath : realToTmpPath.values()) { + tmpPath.delete(); + } + } catch (IOException ignored) { + // Best effort, will be cleaned up at server restart. + } + throw e; + } + + if (inMemoryOutputPath != null) { + String finalInMemoryOutputPath = inMemoryOutputPath; + ByteString finalInMemoryOutputContent = inMemoryOutputContent; + return new SpawnResult.DelegateSpawnResult(previousSpawnResult) { + @Override + @Nullable + public ByteString getInMemoryOutput(ActionInput output) { + if (output.getExecPathString().equals(finalInMemoryOutputPath)) { + return finalInMemoryOutputContent; + } + return null; + } + }; + } + + return previousSpawnResult; + } + private boolean shouldDownload(RemoteActionResult result, PathFragment execPath) { if (outputService instanceof BazelOutputService) { return false; @@ -1473,7 +1694,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( @@ -1483,7 +1704,8 @@ public void uploadOutputs(RemoteAction action, SpawnResult spawnResult) SpawnResult.Status.SUCCESS.equals(spawnResult.status()) && spawnResult.exitCode() == 0, "shouldn't upload outputs of failed local action"); - if (remoteOptions.remoteCacheAsync) { + if (remoteOptions.remoteCacheAsync + && !action.getSpawn().getResourceOwner().mayModifySpawnOutputsAfterExecution()) { Single.using( remoteCache::retain, remoteCache -> @@ -1509,6 +1731,7 @@ public void onSuccess(@NonNull ActionResult actionResult) { Profiler.instance() .completeTask(startTime, ProfilerTask.UPLOAD_TIME, "upload outputs"); backgroundTaskPhaser.arriveAndDeregister(); + onUploadComplete.run(); } @Override @@ -1517,6 +1740,7 @@ public void onError(@NonNull Throwable e) { .completeTask(startTime, ProfilerTask.UPLOAD_TIME, "upload outputs"); backgroundTaskPhaser.arriveAndDeregister(); reportUploadError(e); + onUploadComplete.run(); } }); } else { @@ -1526,6 +1750,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 ad0c51c65e37c9..ae802b93203935 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,59 +100,124 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) context.setDigest(digestUtil.asSpawnLogProto(action.getActionKey())); Profiler prof = Profiler.instance(); + LocalExecution thisExecution = null; if (shouldAcceptCachedResult) { - // Metadata will be available in context.current() until we detach. - // This is done via a thread-local variable. + // 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; try { - RemoteActionResult result; - try (SilentCloseable c = prof.profile(ProfilerTask.REMOTE_CACHE_CHECK, "check cache hit")) { - result = remoteExecutionService.lookupCache(action); - } - // In case the remote cache returned a failed action (exit code != 0) we treat it as a - // cache miss - if (result != null && result.getExitCode() == 0) { - Stopwatch fetchTime = Stopwatch.createStarted(); - InMemoryOutput inMemoryOutput; - try (SilentCloseable c = prof.profile(REMOTE_DOWNLOAD, "download outputs")) { - inMemoryOutput = remoteExecutionService.downloadOutputs(action, result); - } - fetchTime.stop(); - totalTime.stop(); - spawnMetrics - .setFetchTimeInMs((int) fetchTime.elapsed().toMillis()) - .setTotalTimeInMs((int) totalTime.elapsed().toMillis()) - .setNetworkTimeInMs((int) action.getNetworkTime().getDuration().toMillis()); - SpawnResult spawnResult = - createSpawnResult( - digestUtil, + thisExecution = LocalExecution.createIfDeduplicatable(action); + if (shouldUploadLocalResults && thisExecution != null) { + LocalExecution previousOrThisExecution = + inFlightExecutions.merge( action.getActionKey(), - result.getExitCode(), - /* cacheHit= */ true, - result.cacheName(), - inMemoryOutput, - result.getExecutionMetadata().getExecutionStartTimestamp(), - result.getExecutionMetadata().getExecutionCompletedTimestamp(), - spawnMetrics.build(), - spawn.getMnemonic()); - return SpawnCache.success(spawnResult); + thisExecution, + (existingExecution, thisExecutionArg) -> { + if (existingExecution.registerForOutputReuse()) { + return existingExecution; + } else { + // The existing execution has completed and its results may have already + // been modified by its action, so we can't deduplicate against it. Instead, + // start a new in-flight execution. + return thisExecutionArg; + } + }); + previousExecution = + previousOrThisExecution == thisExecution ? null : previousOrThisExecution; } - } catch (CacheNotFoundException e) { - // Intentionally left blank - } catch (IOException e) { - if (BulkTransferException.allCausedByCacheNotFoundException(e)) { + try { + RemoteActionResult result; + try (SilentCloseable c = + prof.profile(ProfilerTask.REMOTE_CACHE_CHECK, "check cache hit")) { + result = remoteExecutionService.lookupCache(action); + } + // In case the remote cache returned a failed action (exit code != 0) we treat it as a + // cache miss + if (result != null && result.getExitCode() == 0) { + Stopwatch fetchTime = Stopwatch.createStarted(); + InMemoryOutput inMemoryOutput; + try (SilentCloseable c = prof.profile(REMOTE_DOWNLOAD, "download outputs")) { + inMemoryOutput = remoteExecutionService.downloadOutputs(action, result); + } + fetchTime.stop(); + totalTime.stop(); + spawnMetrics + .setFetchTimeInMs((int) fetchTime.elapsed().toMillis()) + .setTotalTimeInMs((int) totalTime.elapsed().toMillis()) + .setNetworkTimeInMs((int) action.getNetworkTime().getDuration().toMillis()); + SpawnResult spawnResult = + createSpawnResult( + digestUtil, + action.getActionKey(), + result.getExitCode(), + /* cacheHit= */ true, + result.cacheName(), + inMemoryOutput, + result.getExecutionMetadata().getExecutionStartTimestamp(), + result.getExecutionMetadata().getExecutionCompletedTimestamp(), + spawnMetrics.build(), + spawn.getMnemonic()); + return SpawnCache.success(spawnResult); + } + } catch (CacheNotFoundException e) { // Intentionally left blank - } else { - String errorMessage = Utils.grpcAwareErrorMessage(e, verboseFailures); - if (isNullOrEmpty(errorMessage)) { - errorMessage = e.getClass().getSimpleName(); + } catch (IOException e) { + if (BulkTransferException.allCausedByCacheNotFoundException(e)) { + // Intentionally left blank + } else { + String errorMessage = Utils.grpcAwareErrorMessage(e, verboseFailures); + if (isNullOrEmpty(errorMessage)) { + errorMessage = e.getClass().getSimpleName(); + } + errorMessage = "Remote Cache: " + errorMessage; + remoteExecutionService.report(Event.warn(errorMessage)); } - errorMessage = "Remote Cache: " + errorMessage; - 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. + } + } finally { + if (previousExecution != null) { + previousExecution.unregister(); } } } if (shouldUploadLocalResults) { + final LocalExecution thisExecutionFinal = thisExecution; return new CacheHandle() { @Override public boolean hasResult() { @@ -167,8 +236,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,12 +254,25 @@ 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())); + if (thisExecutionFinal != null + && action.getSpawn().getResourceOwner().mayModifySpawnOutputsAfterExecution()) { + // In this case outputs have been uploaded synchronously and the callback above has run, + // so no new executions will be deduplicated against this one. We can safely await all + // existing executions finish the reuse. + // Note that while this call itself isn't interruptible, all operations it awaits are + // interruptible. + try (SilentCloseable c = prof.profile(REMOTE_DOWNLOAD, "await output reuse")) { + thisExecutionFinal.awaitAllOutputReuse(); + } + } } - @Override - public void close() {} - private void checkForConcurrentModifications() throws IOException, ForbiddenActionInputException { for (ActionInput input : action.getInputMap(true).values()) { @@ -204,6 +286,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 0d7d6c7110e8d3..9a342fae5e742b 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 @@ -695,7 +695,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 3a2c708477c316..5819cc3a169dd6 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/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 51156c12777192..7e72f9ed0b23fd 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -96,6 +96,7 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.SkyframeLookupResult; +import com.google.protobuf.ByteString; import java.io.IOException; import java.io.InputStream; import java.nio.charset.StandardCharsets; @@ -1538,16 +1539,11 @@ e, createFailureDetail("OutErr copy failure", Code.COPY_OUT_ERR_FAILURE)), } @Nullable - private byte[] getDotDContents(SpawnResult spawnResult) throws EnvironmentalExecException { + private byte[] getDotDContents(SpawnResult spawnResult) { if (getDotdFile() != null) { - InputStream in = spawnResult.getInMemoryOutput(getDotdFile()); - if (in != null) { - try { - return ByteStreams.toByteArray(in); - } catch (IOException e) { - throw new EnvironmentalExecException( - e, createFailureDetail("Reading in-memory .d file failed", Code.D_FILE_READ_FAILURE)); - } + ByteString content = spawnResult.getInMemoryOutput(getDotdFile()); + if (content != null) { + return content.toByteArray(); } } return null; diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java index 9d2784c2d2eaee..d47872ba66d851 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java @@ -82,6 +82,7 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.view.proto.Deps; +import com.google.protobuf.ByteString; import com.google.protobuf.ExtensionRegistry; import java.io.IOException; import java.io.InputStream; @@ -714,6 +715,14 @@ public NestedSet getPossibleInputsForTesting() { return null; } + @Override + public boolean mayModifySpawnOutputsAfterExecution() { + // Causes of spawn output modification after execution: + // - Fallback to the full classpath with --experimental_java_classpath=bazel. + // - In-place rewriting of .jdeps files with --experimental_output_paths=strip. + return true; + } + /** * Locally rewrites a .jdeps file to replace missing config prefixes. * @@ -839,11 +848,11 @@ private static Deps.Dependencies readExecutorJdeps( Artifact outputDepsProto, ActionExecutionContext actionExecutionContext) throws IOException { - InputStream inMemoryOutput = spawnResult.getInMemoryOutput(outputDepsProto); + ByteString inMemoryOutput = spawnResult.getInMemoryOutput(outputDepsProto); try (InputStream inputStream = inMemoryOutput == null ? actionExecutionContext.getInputPath(outputDepsProto).getInputStream() - : inMemoryOutput) { + : inMemoryOutput.newInput()) { return Deps.Dependencies.parseFrom(inputStream, ExtensionRegistry.getEmptyRegistry()); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java index 0866cfba49fa88..728aedf7edb9b3 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java @@ -161,6 +161,14 @@ protected void afterExecute( } } + @Override + public boolean mayModifySpawnOutputsAfterExecution() { + // Causes of spawn output modification after execution: + // - In-place rewriting of .jdeps files with --experimental_output_paths=strip. + // TODO: Use separate files as action and spawn output to avoid in-place modification. + return true; + } + public static Builder newBuilder(RuleContext ruleContext) { return new Builder(ruleContext); } diff --git a/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java b/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java index df9e2b868831e3..04568054a4a23c 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java @@ -66,7 +66,7 @@ public void getTimeoutMessageNoTime() { } @Test - public void inMemoryContents() throws Exception { + public void inMemoryContents() { ActionInput output = ActionInputHelper.fromPath("/foo/bar"); ByteString contents = ByteString.copyFromUtf8("hello world"); @@ -78,7 +78,7 @@ public void inMemoryContents() throws Exception { .setInMemoryOutput(output, contents) .build(); - assertThat(ByteString.readFrom(r.getInMemoryOutput(output))).isEqualTo(contents); + assertThat(r.getInMemoryOutput(output)).isEqualTo(contents); assertThat(r.getInMemoryOutput(null)).isEqualTo(null); assertThat(r.getInMemoryOutput(ActionInputHelper.fromPath("/does/not/exist"))).isEqualTo(null); } 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 6c7a2bb7b68c2d..1f9d3208718b5b 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 @@ -1647,7 +1647,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(); @@ -1694,7 +1694,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(); @@ -1768,7 +1768,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(); @@ -1804,7 +1804,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(); @@ -1856,7 +1856,7 @@ public void uploadOutputs_emptyOutputs_doNotPerformUpload() throws Exception { .build(); // act - service.uploadOutputs(action, spawnResult); + service.uploadOutputs(action, spawnResult, () -> {}); // assert assertThat( @@ -1882,7 +1882,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); @@ -1907,7 +1907,7 @@ public void uploadOutputs_firesUploadEvents() throws Exception { .setRunnerName("test") .build(); - service.uploadOutputs(action, spawnResult); + service.uploadOutputs(action, spawnResult, () -> {}); assertThat(eventHandler.getPosts()) .containsAtLeast( @@ -1934,7 +1934,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 26a1621525d177..8b774677b16b20 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; @@ -43,15 +44,18 @@ import com.google.common.eventbus.EventBus; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.ActionContext; +import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.ArtifactPathResolver; +import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.ForbiddenActionInputException; 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,15 +86,21 @@ 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; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.devtools.common.options.Options; +import com.google.protobuf.ByteString; import java.io.IOException; import java.time.Duration; +import java.util.Set; import java.util.SortedMap; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicReference; import javax.annotation.Nullable; import org.junit.Before; import org.junit.Test; @@ -98,6 +108,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; @@ -119,7 +130,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; @@ -128,101 +139,102 @@ 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, /*strict*/ false); - } - - @Override - public Duration getTimeout() { - return Duration.ZERO; - } - - @Override - public FileOutErr getFileOutErr() { - return outErr; - } - - @Override - public SortedMap getInputMapping( - PathFragment baseDirectory, boolean willAccessRepeatedly) - throws IOException, ForbiddenActionInputException { - return getSpawnInputExpander() - .getInputMapping(simpleSpawn, SIMPLE_ARTIFACT_EXPANDER, baseDirectory, fakeFileCache); - } - - @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, /*strict*/ false); + } + + @Override + public Duration getTimeout() { + return Duration.ZERO; + } + + @Override + public FileOutErr getFileOutErr() { + return outErr; + } + + @Override + public SortedMap getInputMapping( + PathFragment baseDirectory, boolean willAccessRepeatedly) + throws IOException, ForbiddenActionInputException { + return getSpawnInputExpander() + .getInputMapping(spawn, SIMPLE_ARTIFACT_EXPANDER, baseDirectory, fakeFileCache); + } + + @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) { @@ -237,6 +249,33 @@ private static SimpleSpawn simpleSpawnWithExecutionInfo( ResourceSet.ZERO); } + private static SimpleSpawn simplePathMappedSpawn(String configSegment) { + return simplePathMappedSpawn( + configSegment, new FakeOwner("Mnemonic", "Progress Message", "//dummy:label")); + } + + private static SimpleSpawn simplePathMappedSpawn( + String configSegment, ActionExecutionMetadata owner) { + String inputPath = "bazel-bin/%s/bin/input"; + String outputPath = "bazel-bin/%s/bin/output"; + return new SimpleSpawn( + owner, + ImmutableList.of("cp", inputPath.formatted("cfg"), outputPath.formatted("cfg")), + ImmutableMap.of("VARIABLE", "value"), + ImmutableMap.of(ExecutionRequirements.SUPPORTS_PATH_MAPPING, ""), + /* runfilesSupplier= */ null, + /* 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)); } @@ -273,7 +312,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"); @@ -286,6 +325,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"); } @@ -338,7 +378,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(); @@ -369,9 +409,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 @@ -535,7 +575,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 @@ -558,9 +598,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); @@ -606,9 +646,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. } @@ -682,4 +722,303 @@ 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 pathMappedActionIsDeduplicatedWithSpawnOutputModification() throws Exception { + // arrange + RemoteSpawnCache cache = createRemoteSpawnCache(); + + ActionExecutionMetadata firstExecutionOwner = + new FakeOwner("Mnemonic", "Progress Message", "//dummy:label") { + @Override + public boolean mayModifySpawnOutputsAfterExecution() { + return true; + } + }; + SimpleSpawn firstSpawn = simplePathMappedSpawn("k8-fastbuild", firstExecutionOwner); + 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(); + CountDownLatch enteredWaitForAndReuseOutputs = new CountDownLatch(1); + CountDownLatch completeWaitForAndReuseOutputs = new CountDownLatch(1); + CountDownLatch enteredUploadOutputs = new CountDownLatch(1); + Set spawnsThatWaitedForOutputReuse = ConcurrentHashMap.newKeySet(); + Mockito.doAnswer( + (Answer) + invocation -> { + spawnsThatWaitedForOutputReuse.add( + ((RemoteAction) invocation.getArgument(0)).getSpawn()); + enteredWaitForAndReuseOutputs.countDown(); + completeWaitForAndReuseOutputs.await(); + return (SpawnResult) invocation.callRealMethod(); + }) + .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 avoids introducing + // more concurrency to this test. + Mockito.doAnswer( + (Answer) + invocation -> { + enteredUploadOutputs.countDown(); + return null; + }) + .when(remoteExecutionService) + .uploadOutputs(any(), any(), any()); + + // act + // Simulate the first spawn writing to the output, but delay its completion. + CacheHandle firstCacheHandle = cache.lookup(firstSpawn, firstPolicy); + FileSystemUtils.writeContent( + fs.getPath("/exec/root/bazel-bin/k8-fastbuild/bin/output"), UTF_8, "hello"); + + // Start the second spawn and wait for it to deduplicate against the first one. + AtomicReference secondCacheHandleRef = new AtomicReference<>(); + Thread lookupSecondSpawn = + new Thread( + () -> { + try { + secondCacheHandleRef.set(cache.lookup(secondSpawn, secondPolicy)); + } catch (InterruptedException + | IOException + | ExecException + | ForbiddenActionInputException e) { + throw new IllegalStateException(e); + } + }); + lookupSecondSpawn.start(); + enteredWaitForAndReuseOutputs.await(); + + // Complete the first spawn and immediately corrupt its outputs. + Thread completeFirstSpawn = + new Thread( + () -> { + try { + firstCacheHandle.store( + new SpawnResult.Builder() + .setExitCode(0) + .setStatus(Status.SUCCESS) + .setRunnerName("test") + .build()); + FileSystemUtils.writeContent( + fs.getPath("/exec/root/bazel-bin/k8-fastbuild/bin/output"), UTF_8, "corrupted"); + } catch (IOException | ExecException | InterruptedException e) { + throw new IllegalStateException(e); + } + }); + completeFirstSpawn.start(); + // Make it more likely to detect races by waiting for the first spawn to (fake) upload its + // outputs. + enteredUploadOutputs.await(); + + // Let the second spawn complete its output reuse. + completeWaitForAndReuseOutputs.countDown(); + lookupSecondSpawn.join(); + CacheHandle secondCacheHandle = secondCacheHandleRef.get(); + + completeFirstSpawn.join(); + + // assert + assertThat(spawnsThatWaitedForOutputReuse).containsExactly(secondSpawn); + 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 pathMappedActionWithInMemoryOutputIsDeduplicated() 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)) { + firstCacheHandle.store( + new SpawnResult.Builder() + .setExitCode(0) + .setStatus(Status.SUCCESS) + .setRunnerName("test") + .setInMemoryOutput( + firstSpawn.getOutputFiles().getFirst(), ByteString.copyFromUtf8("in-memory")) + .build()); + } + CacheHandle secondCacheHandle = cache.lookup(secondSpawn, secondPolicy); + + // assert + ActionInput inMemoryOutput = secondSpawn.getOutputFiles().getFirst(); + assertThat(secondCacheHandle.hasResult()).isTrue(); + assertThat(secondCacheHandle.getResult().getRunnerName()).isEqualTo("deduplicated"); + assertThat(secondCacheHandle.getResult().getInMemoryOutput(inMemoryOutput).toStringUtf8()) + .isEqualTo("in-memory"); + assertThat(execRoot.getRelative(inMemoryOutput.getExecPath()).exists()).isFalse(); + 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 e7994fc4803cb8..854ba501a9d74a 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 @@ -243,7 +243,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); } @@ -318,7 +318,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( @@ -344,7 +344,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 @@ -377,7 +377,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 @@ -404,7 +404,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); @@ -422,7 +422,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 c619ffcc3abf69..d16a38c5e42c8d 100755 --- a/src/test/shell/bazel/path_mapping_test.sh +++ b/src/test/shell/bazel/path_mapping_test.sh @@ -792,4 +792,269 @@ Hello, BazelCon Munich! Hello, BazelCon New York!" "$(cat "$(bazel cquery --output=files //pkg:all_greetings)")" } +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"