From a151116f7f671efea9a95f3e251561e53e535cca Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Thu, 17 Mar 2022 04:14:41 -0700 Subject: [PATCH] Remote: Check declared outputs when downloading outputs. 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 #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 #14543. Closes #15051. PiperOrigin-RevId: 435307260 --- .../lib/remote/RemoteExecutionService.java | 41 ++++++++++++----- .../remote/RemoteExecutionServiceTest.java | 44 +++++++++++++++++-- .../lib/remote/RemoteSpawnCacheTest.java | 2 + .../lib/remote/RemoteSpawnRunnerTest.java | 1 + ...SpawnRunnerWithGrpcRemoteExecutorTest.java | 8 +++- 5 files changed, 81 insertions(+), 15 deletions(-) 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 ebe3ec05f57943..b6ccc3adc071d4 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 @@ -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> downloadsBuilder = @@ -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 buildUploadManifestAsync( RemoteAction action, SpawnResult spawnResult) { return Single.fromCallable( () -> { ImmutableList.Builder 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( 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 9011e20c98149b..26615963cd5756 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 @@ -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); @@ -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 outputs = ImmutableSet.builder(); + ImmutableList 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. @@ -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(); 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 b359091f6ce5bc..c46e05d31733ab 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 @@ -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; @@ -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); 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 d7ca873267073e..22afbf5d2e8334 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 @@ -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); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java index a59627a6b19349..57066fa8a9ae52 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java @@ -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. @@ -196,9 +196,12 @@ public final void setUp() throws Exception { ImmutableList.of("/bin/echo", "Hi!"), ImmutableMap.of("VARIABLE", "value"), /*executionInfo=*/ ImmutableMap.of(), + /*runfilesSupplier=*/ null, + /*filesetMappings=*/ ImmutableMap.of(), /*inputs=*/ NestedSetBuilder.create( Order.STABLE_ORDER, ActionInputHelper.fromPath("input")), - /*outputs=*/ ImmutableSet.of( + /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /*outputs=*/ ImmutableSet.of( new ActionInput() { @Override public String getExecPathString() { @@ -231,6 +234,7 @@ public PathFragment getExecPath() { return PathFragment.create("bar"); } }), + /*mandatoryOutputs=*/ ImmutableSet.of(), ResourceSet.ZERO); Path stdout = fs.getPath("/tmp/stdout");