Skip to content

Commit

Permalink
Remote: Check declared outputs when downloading outputs.
Browse files Browse the repository at this point in the history
An AC entry that misses declared outputs of an action is invalid because, if Bazel accepts this from remote cache, it will detect the missing declared outputs error at a later stage and fail the build.

This PR adds a check for mandatory outputs when downloading outputs from remote cache. If a mandatory output is missing from AC entry, Bazel will ignore the cache entry so build can continue.

Also fixes an issue introduced by bazelbuild#15016 that tests result are not uploaded to remote cache. Test spawns declare all the possible outputs but they usually don't generate them all. This change fixes that by only check for mandatory outputs via `spawn.isMandatoryOutput()`.

Fixes bazelbuild#14543.

Closes bazelbuild#15051.

PiperOrigin-RevId: 435307260
  • Loading branch information
coeuvre authored and copybara-github committed Mar 17, 2022
1 parent 9515e9b commit a151116
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1011,6 +1011,23 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
metadata = parseActionResultMetadata(action, result);
}

// Check that all mandatory outputs are created.
for (ActionInput output : action.spawn.getOutputFiles()) {
if (action.spawn.isMandatoryOutput(output)) {
Path localPath = execRoot.getRelative(output.getExecPath());
if (!metadata.files.containsKey(localPath)
&& !metadata.directories.containsKey(localPath)
&& !metadata.symlinks.containsKey(localPath)) {
throw new IOException(
"Invalid action cache entry "
+ action.actionKey.getDigest().getHash()
+ ": expected output "
+ prettyPrint(output)
+ " does not exist.");
}
}
}

FileOutErr outErr = action.spawnExecutionContext.getFileOutErr();

ImmutableList.Builder<ListenableFuture<FileMetadata>> downloadsBuilder =
Expand Down Expand Up @@ -1124,23 +1141,27 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
return null;
}

private static String prettyPrint(ActionInput actionInput) {
if (actionInput instanceof Artifact) {
return ((Artifact) actionInput).prettyPrint();
} else {
return actionInput.getExecPathString();
}
}

private Single<UploadManifest> buildUploadManifestAsync(
RemoteAction action, SpawnResult spawnResult) {
return Single.fromCallable(
() -> {
ImmutableList.Builder<Path> outputFiles = ImmutableList.builder();
// Check that all mandatory outputs are created.
for (ActionInput outputFile : action.spawn.getOutputFiles()) {
Path outputPath = execRoot.getRelative(outputFile.getExecPath());
if (!outputPath.exists()) {
String output;
if (outputFile instanceof Artifact) {
output = ((Artifact) outputFile).prettyPrint();
} else {
output = outputFile.getExecPathString();
}
throw new IOException("Expected output " + output + " was not created locally.");
Path localPath = execRoot.getRelative(outputFile.getExecPath());
if (action.spawn.isMandatoryOutput(outputFile) && !localPath.exists()) {
throw new IOException(
"Expected output " + prettyPrint(outputFile) + " was not created locally.");
}
outputFiles.add(outputPath);
outputFiles.add(localPath);
}

return UploadManifest.create(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1101,9 +1101,21 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw
ActionResult r = ActionResult.newBuilder().setExitCode(0).build();
RemoteActionResult result = RemoteActionResult.createFromCache(CachedActionResult.remote(r));
Artifact a1 = ActionsTestUtil.createArtifact(artifactRoot, "file1");
// set file1 as declared output but not mandatory output
Spawn spawn =
newSpawn(
ImmutableMap.of(REMOTE_EXECUTION_INLINE_OUTPUTS, "outputs/file1"), ImmutableSet.of(a1));
new SimpleSpawn(
new FakeOwner("foo", "bar", "//dummy:label"),
/*arguments=*/ ImmutableList.of(),
/*environment=*/ ImmutableMap.of(),
/*executionInfo=*/ ImmutableMap.of(REMOTE_EXECUTION_INLINE_OUTPUTS, "outputs/file1"),
/*runfilesSupplier=*/ null,
/*filesetMappings=*/ ImmutableMap.of(),
/*inputs=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/*outputs=*/ ImmutableSet.of(a1),
/*mandatoryOutputs=*/ ImmutableSet.of(),
ResourceSet.ZERO);

MetadataInjector injector = mock(MetadataInjector.class);
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn, injector);
RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class);
Expand All @@ -1120,6 +1132,32 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw
verify(injector, never()).injectFile(eq(a1), remoteFileMatchingDigest(d1));
}

@Test
public void downloadOutputs_missingMandatoryOutputs_reportError() throws Exception {
// Test that an AC which misses mandatory outputs is correctly ignored.
Digest fooDigest = cache.addContents(remoteActionExecutionContext, "foo-contents");
ActionResult.Builder builder = ActionResult.newBuilder();
builder.addOutputFilesBuilder().setPath("outputs/foo").setDigest(fooDigest);
RemoteActionResult result =
RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build()));
ImmutableSet.Builder<Artifact> outputs = ImmutableSet.builder();
ImmutableList<String> expectedOutputFiles = ImmutableList.of("outputs/foo", "outputs/bar");
for (String outputFile : expectedOutputFiles) {
Path path = remotePathResolver.outputPathToLocalPath(outputFile);
Artifact output = ActionsTestUtil.createArtifact(artifactRoot, path);
outputs.add(output);
}
Spawn spawn = newSpawn(ImmutableMap.of(), outputs.build());
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
RemoteExecutionService service = newRemoteExecutionService();
RemoteAction action = service.buildRemoteAction(spawn, context);

IOException error =
assertThrows(IOException.class, () -> service.downloadOutputs(action, result));

assertThat(error).hasMessageThat().containsMatch("expected output .+ does not exist.");
}

@Test
public void uploadOutputs_uploadDirectory_works() throws Exception {
// Test that uploading a directory works.
Expand Down Expand Up @@ -1368,7 +1406,7 @@ public void uploadOutputs_firesUploadEvents() throws Exception {
}

@Test
public void uploadOutputs_missingDeclaredOutputs_dontUpload() throws Exception {
public void uploadOutputs_missingMandatoryOutputs_dontUpload() throws Exception {
Path file = execRoot.getRelative("outputs/file");
Artifact outputFile = ActionsTestUtil.createArtifact(artifactRoot, file);
RemoteExecutionService service = newRemoteExecutionService();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
Expand Down Expand Up @@ -602,6 +603,7 @@ public void testDownloadMinimal() throws Exception {
when(remoteCache.downloadActionResult(
any(RemoteActionExecutionContext.class), any(), /* inlineOutErr= */ eq(false)))
.thenReturn(CachedActionResult.remote(success));
doReturn(null).when(cache.getRemoteExecutionService()).downloadOutputs(any(), any());

// act
CacheHandle cacheHandle = cache.lookup(simpleSpawn, simplePolicy);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1197,6 +1197,7 @@ public void testDownloadTopLevel() throws Exception {
RemoteSpawnRunner runner = newSpawnRunner(ImmutableSet.of(topLevelOutput));
RemoteExecutionService service = runner.getRemoteExecutionService();
doReturn(cachedActionResult).when(service).lookupCache(any());
doReturn(null).when(service).downloadOutputs(any(), any());

Spawn spawn = newSimpleSpawn(topLevelOutput);
FakeSpawnExecutionContext policy = getSpawnContext(spawn);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2017 The Bazel Authors. All rights reserved.
/// Copyright 2017 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -196,9 +196,12 @@ public final void setUp() throws Exception {
ImmutableList.of("/bin/echo", "Hi!"),
ImmutableMap.of("VARIABLE", "value"),
/*executionInfo=*/ ImmutableMap.<String, String>of(),
/*runfilesSupplier=*/ null,
/*filesetMappings=*/ ImmutableMap.of(),
/*inputs=*/ NestedSetBuilder.create(
Order.STABLE_ORDER, ActionInputHelper.fromPath("input")),
/*outputs=*/ ImmutableSet.<ActionInput>of(
/*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/*outputs=*/ ImmutableSet.of(
new ActionInput() {
@Override
public String getExecPathString() {
Expand Down Expand Up @@ -231,6 +234,7 @@ public PathFragment getExecPath() {
return PathFragment.create("bar");
}
}),
/*mandatoryOutputs=*/ ImmutableSet.of(),
ResourceSet.ZERO);

Path stdout = fs.getPath("/tmp/stdout");
Expand Down

0 comments on commit a151116

Please sign in to comment.