diff options
author | 2017-12-11 07:53:15 -0800 | |
---|---|---|
committer | 2017-12-11 07:57:24 -0800 | |
commit | a22d0e9c14e58b29d81f5a83bdcc6e5fce52eafe (patch) | |
tree | 305828b0cc64d6b9564b23ca311704abde8a9335 /src/main/java/com | |
parent | 180d25660cc5b1272497e30a1cdc494ac5eadf77 (diff) |
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
Diffstat (limited to 'src/main/java/com')
6 files changed, 74 insertions, 34 deletions
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 51d8d3b645..b6a26954d4 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 @@ public interface SpawnResult { /** 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 @@ public interface SpawnResult { 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 @@ public interface SpawnResult { this.numBlockInputOperations = builder.numBlockInputOperations; this.numInvoluntaryContextSwitches = builder.numInvoluntaryContextSwitches; this.cacheHit = builder.cacheHit; + this.failureMessage = builder.failureMessage; } @Override @@ -274,6 +281,11 @@ public interface SpawnResult { } @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 @@ public interface SpawnResult { 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 @@ public interface SpawnResult { 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 1512a0139c..bf3ef4e59c 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 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte 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 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte 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 016dfe80bd..1374b47fd8 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 @@ public interface SpawnCache extends ActionContext { * 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 57f19fa258..91e27cd49c 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.ExecutionStrategy; 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 @@ final class RemoteSpawnCache implements SpawnCache { 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 @@ final class RemoteSpawnCache implements SpawnCache { 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 @@ final class RemoteSpawnCache implements SpawnCache { @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 c67ef489a6..c63975738b 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 @@ class RemoteSpawnRunner implements SpawnRunner { 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 @@ class RemoteSpawnRunner implements SpawnRunner { 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 @@ class RemoteSpawnRunner implements SpawnRunner { 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 @@ class RemoteSpawnRunner implements SpawnRunner { @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 @@ class RemoteSpawnRunner implements SpawnRunner { } 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 5875dad58d..957449eb2b 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; 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 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { 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 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { } 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 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { } 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 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { return new SpawnResult.Builder() .setStatus(Status.EXECUTION_FAILED) .setExitCode(LOCAL_EXEC_ERROR) + .setFailureMessage(failureMessage) .build(); } @@ -182,6 +183,7 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { .setWallTime(wallTime) .setUserTime(commandResult.getUserExecutionTime()) .setSystemTime(commandResult.getSystemExecutionTime()) + .setFailureMessage(status != Status.SUCCESS || exitCode != 0 ? failureMessage : "") .build(); } |