Fix: uploading artifacts of failed actions to remote cache stopped working.
To reproduce: run a failing test with --experimental_remote_spawn_cache or with --spawn_strategy=remote and no executor. Expected: test log is uploaded.
Desired behavior:
- regardless of whether a spawn is cacheable or not, its artifacts should be uploaded to the remote cache.
- the spawn result should only be set if the spawn is cacheable *and* the action succeeded.
- when executing remotely, the do_not_cache field should be set for non-cacheable spawns, and the remote execution engine should respect it.
This CL contains multiple fixes to ensure the above behaviors, and adds a few tests, both end to end and unit tests. Important behavior change: it is no longer assumed that non-cacheable spawns should use a NO_CACHE SpawnCache! The appropriate test case was removed. Instead, an assumption was added that all implementations of SpawnCache should respect the Spawns.mayBeCached(spawn) property. Currently, only NO_CACHE and RemoteSpawnCache exist, and they (now) support it.
TESTED=remote build execution backend.
WANT_LGTM: philwo,buchgr
RELNOTES: None
PiperOrigin-RevId: 178617937
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 51d8d3b..b6a2695 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
@@ -178,6 +178,11 @@
/** Whether the spawn result was a cache hit. */
boolean isCacheHit();
+ /** Returns an optional custom failure message for the result. */
+ default String getFailureMessage() {
+ return "";
+ }
+
String getDetailMessage(
String messagePrefix, String message, boolean catastrophe, boolean forciblyRunRemotely);
@@ -196,6 +201,7 @@
private final Optional<Long> numBlockInputOperations;
private final Optional<Long> numInvoluntaryContextSwitches;
private final boolean cacheHit;
+ private final String failureMessage;
SimpleSpawnResult(Builder builder) {
this.exitCode = builder.exitCode;
@@ -208,6 +214,7 @@
this.numBlockInputOperations = builder.numBlockInputOperations;
this.numInvoluntaryContextSwitches = builder.numInvoluntaryContextSwitches;
this.cacheHit = builder.cacheHit;
+ this.failureMessage = builder.failureMessage;
}
@Override
@@ -274,6 +281,11 @@
}
@Override
+ public String getFailureMessage() {
+ return failureMessage;
+ }
+
+ @Override
public String getDetailMessage(
String messagePrefix, String message, boolean catastrophe, boolean forciblyRunRemotely) {
TerminationStatus status = new TerminationStatus(
@@ -318,6 +330,7 @@
private Optional<Long> numBlockInputOperations = Optional.empty();
private Optional<Long> numInvoluntaryContextSwitches = Optional.empty();
private boolean cacheHit;
+ private String failureMessage = "";
public SpawnResult build() {
if (status == Status.SUCCESS) {
@@ -395,5 +408,10 @@
this.cacheHit = cacheHit;
return this;
}
+
+ public Builder setFailureMessage(String failureMessage) {
+ this.failureMessage = failureMessage;
+ return this;
+ }
}
}
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 1512a01..bf3ef4e 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
@@ -84,7 +84,7 @@
SpawnCache cache = actionExecutionContext.getContext(SpawnCache.class);
// In production, the getContext method guarantees that we never get null back. However, our
// integration tests don't set it up correctly, so cache may be null in testing.
- if (cache == null || !Spawns.mayBeCached(spawn)) {
+ if (cache == null) {
cache = SpawnCache.NO_CACHE;
}
SpawnResult spawnResult;
@@ -107,12 +107,15 @@
if (spawnResult.status() != Status.SUCCESS) {
String cwd = actionExecutionContext.getExecRoot().getPathString();
+ String resultMessage = spawnResult.getFailureMessage();
String message =
- CommandFailureUtils.describeCommandFailure(
- actionExecutionContext.getVerboseFailures(),
- spawn.getArguments(),
- spawn.getEnvironment(),
- cwd);
+ resultMessage != ""
+ ? resultMessage
+ : CommandFailureUtils.describeCommandFailure(
+ actionExecutionContext.getVerboseFailures(),
+ spawn.getArguments(),
+ spawn.getEnvironment(),
+ cwd);
throw new SpawnExecException(message, spawnResult, /*forciblyRunRemotely=*/false);
}
return ImmutableList.of(spawnResult);
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 016dfe8..1374b47 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
@@ -168,6 +168,8 @@
* object is for the cache to store expensive intermediate values (such as the cache key) that are
* needed both for the lookup and the subsequent store operation.
*
+ * <p>The lookup must not succeed for non-cachable spawns. See {@link Spawns#mayBeCached()}.
+ *
* <p>Note that cache stores may be disabled, in which case the returned {@link CacheHandle}
* instance's {@link CacheHandle#store} is a no-op.
*/
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 57f19fa..91e27cd 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
@@ -19,6 +19,7 @@
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.actions.Spawns;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.Reporter;
@@ -102,7 +103,8 @@
digestUtil.compute(command),
repository.getMerkleDigest(inputRoot),
platform,
- policy.getTimeout());
+ policy.getTimeout(),
+ Spawns.mayBeCached(spawn));
// Look up action cache, and reuse the action output if it is found.
final ActionKey actionKey = digestUtil.computeActionKey(action);
@@ -113,7 +115,9 @@
Context previous = withMetadata.attach();
try {
ActionResult result =
- this.options.remoteAcceptCached ? remoteCache.getCachedActionResult(actionKey) : null;
+ this.options.remoteAcceptCached && Spawns.mayBeCached(spawn)
+ ? remoteCache.getCachedActionResult(actionKey)
+ : null;
if (result != null) {
// We don't cache failed actions, so we know the outputs exist.
// For now, download all outputs locally; in the future, we can reuse the digests to
@@ -156,7 +160,10 @@
@Override
public void store(SpawnResult result, Collection<Path> files)
throws InterruptedException, IOException {
- boolean uploadAction = Status.SUCCESS.equals(result.status()) && result.exitCode() == 0;
+ boolean uploadAction =
+ Spawns.mayBeCached(spawn)
+ && Status.SUCCESS.equals(result.status())
+ && result.exitCode() == 0;
Context previous = withMetadata.attach();
try {
remoteCache.upload(actionKey, execRoot, files, policy.getFileOutErr(), uploadAction);
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 c67ef48..c639757 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
@@ -130,7 +130,8 @@
digestUtil.compute(command),
repository.getMerkleDigest(inputRoot),
platform,
- policy.getTimeout());
+ policy.getTimeout(),
+ Spawns.mayBeCached(spawn));
// Look up action cache, and reuse the action output if it is found.
ActionKey actionKey = digestUtil.computeActionKey(action);
@@ -262,7 +263,8 @@
Digest command,
Digest inputRoot,
Platform platform,
- Duration timeout) {
+ Duration timeout,
+ boolean cacheable) {
Action.Builder action = Action.newBuilder();
action.setCommandDigest(command);
action.setInputRootDigest(inputRoot);
@@ -279,6 +281,9 @@
if (!timeout.isZero()) {
action.setTimeout(com.google.protobuf.Duration.newBuilder().setSeconds(timeout.getSeconds()));
}
+ if (!cacheable) {
+ action.setDoNotCache(true);
+ }
return action.build();
}
@@ -326,7 +331,7 @@
@Nullable RemoteActionCache remoteCache,
@Nullable ActionKey actionKey)
throws ExecException, IOException, InterruptedException {
- if (uploadToCache && Spawns.mayBeCached(spawn) && remoteCache != null && actionKey != null) {
+ if (uploadToCache && remoteCache != null && actionKey != null) {
return execLocallyAndUpload(spawn, policy, inputMap, remoteCache, actionKey);
}
return fallbackRunner.exec(spawn, policy);
@@ -351,7 +356,10 @@
}
List<Path> outputFiles = listExistingOutputFiles(execRoot, spawn);
try {
- boolean uploadAction = Status.SUCCESS.equals(result.status()) && result.exitCode() == 0;
+ boolean uploadAction =
+ Spawns.mayBeCached(spawn)
+ && Status.SUCCESS.equals(result.status())
+ && result.exitCode() == 0;
remoteCache.upload(actionKey, execRoot, outputFiles, policy.getFileOutErr(), uploadAction);
} catch (IOException e) {
if (verboseFailures) {
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
index 5875dad..957449e 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
@@ -26,7 +26,6 @@
import com.google.devtools.build.lib.actions.SpawnResult.Status;
import com.google.devtools.build.lib.actions.UserExecException;
import com.google.devtools.build.lib.exec.ExecutionOptions;
-import com.google.devtools.build.lib.exec.SpawnExecException;
import com.google.devtools.build.lib.exec.SpawnRunner;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.shell.AbnormalTerminationException;
@@ -90,13 +89,13 @@
Path execRoot,
Path tmpDir,
Duration timeout)
- throws ExecException, IOException, InterruptedException {
+ throws IOException, InterruptedException {
try {
sandbox.createFileSystem();
OutErr outErr = policy.getFileOutErr();
policy.prefetchInputs();
- SpawnResult result = run(sandbox, outErr, timeout, tmpDir);
+ SpawnResult result = run(originalSpawn, sandbox, outErr, timeout, execRoot, tmpDir);
policy.lockOutputFiles();
try {
@@ -105,23 +104,6 @@
} catch (IOException e) {
throw new IOException("Could not move output artifacts from sandboxed execution", e);
}
-
- if (result.status() != Status.SUCCESS || result.exitCode() != 0) {
- String message;
- if (sandboxOptions.sandboxDebug) {
- message =
- CommandFailureUtils.describeCommandFailure(
- true, sandbox.getArguments(), sandbox.getEnvironment(), execRoot.getPathString());
- } else {
- message =
- CommandFailureUtils.describeCommandFailure(
- verboseFailures,
- originalSpawn.getArguments(),
- originalSpawn.getEnvironment(),
- execRoot.getPathString()) + SANDBOX_DEBUG_SUGGESTION;
- }
- throw new SpawnExecException(message, result, /*forciblyRunRemotely=*/false);
- }
return result;
} finally {
if (!sandboxOptions.sandboxDebug) {
@@ -131,12 +113,30 @@
}
private final SpawnResult run(
- SandboxedSpawn sandbox, OutErr outErr, Duration timeout, Path tmpDir)
+ Spawn originalSpawn,
+ SandboxedSpawn sandbox,
+ OutErr outErr,
+ Duration timeout,
+ Path execRoot,
+ Path tmpDir)
throws IOException, InterruptedException {
Command cmd = new Command(
sandbox.getArguments().toArray(new String[0]),
sandbox.getEnvironment(),
sandbox.getSandboxExecRoot().getPathFile());
+ String failureMessage;
+ if (sandboxOptions.sandboxDebug) {
+ failureMessage =
+ CommandFailureUtils.describeCommandFailure(
+ true, sandbox.getArguments(), sandbox.getEnvironment(), execRoot.getPathString());
+ } else {
+ failureMessage =
+ CommandFailureUtils.describeCommandFailure(
+ verboseFailures,
+ originalSpawn.getArguments(),
+ originalSpawn.getEnvironment(),
+ execRoot.getPathString()) + SANDBOX_DEBUG_SUGGESTION;
+ }
long startTime = System.currentTimeMillis();
CommandResult commandResult;
@@ -162,6 +162,7 @@
return new SpawnResult.Builder()
.setStatus(Status.EXECUTION_FAILED)
.setExitCode(LOCAL_EXEC_ERROR)
+ .setFailureMessage(failureMessage)
.build();
}
@@ -182,6 +183,7 @@
.setWallTime(wallTime)
.setUserTime(commandResult.getUserExecutionTime())
.setSystemTime(commandResult.getSystemExecutionTime())
+ .setFailureMessage(status != Status.SUCCESS || exitCode != 0 ? failureMessage : "")
.build();
}
