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/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
index 7ac5cfa..65c7d88 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
@@ -29,6 +29,7 @@
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
+import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.SimpleSpawn;
import com.google.devtools.build.lib.actions.SpawnResult;
@@ -264,6 +265,45 @@
}
@Test
+ public void noCacheSpawns() throws Exception {
+ // Checks that spawns that have mayBeCached false are not looked up in the remote cache,
+ // and also that their result is not uploaded to the remote cache. The artifacts, however,
+ // are uploaded.
+ SimpleSpawn uncacheableSpawn = new SimpleSpawn(
+ new FakeOwner("foo", "bar"),
+ /*arguments=*/ ImmutableList.of(),
+ /*environment=*/ ImmutableMap.of(),
+ ImmutableMap.of(ExecutionRequirements.NO_CACHE, ""),
+ /*inputs=*/ ImmutableList.of(),
+ /*outputs=*/ ImmutableList.<ActionInput>of(),
+ ResourceSet.ZERO);
+ CacheHandle entry = cache.lookup(uncacheableSpawn, simplePolicy);
+ verify(remoteCache, never())
+ .getCachedActionResult(any(ActionKey.class));
+ assertThat(entry.hasResult()).isFalse();
+ SpawnResult result = new SpawnResult.Builder().setExitCode(0).setStatus(Status.SUCCESS).build();
+ ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));
+ entry.store(result, outputFiles);
+ verify(remoteCache)
+ .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false));
+ }
+
+ @Test
+ public void noCacheSpawnsNoResultStore() throws Exception {
+ // Only successful action results are uploaded to the remote cache. The artifacts, however,
+ // are uploaded regardless.
+ CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy);
+ verify(remoteCache).getCachedActionResult(any(ActionKey.class));
+ assertThat(entry.hasResult()).isFalse();
+ SpawnResult result =
+ new SpawnResult.Builder().setExitCode(1).setStatus(Status.NON_ZERO_EXIT).build();
+ ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));
+ entry.store(result, outputFiles);
+ verify(remoteCache)
+ .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false));
+ }
+
+ @Test
public void printWarningIfUploadFails() throws Exception {
CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy);
assertThat(entry.hasResult()).isFalse();