From 41ce54d304c2c8203f9612636412c16533f0484b Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 19 Jun 2023 06:13:27 -0700 Subject: [PATCH] Remove superfluous code in RemoteExecutionService. The directories that are to contain spawn outputs have already been created before the spawn executes, so it's unnecessary for RemoteExecutionService to create them (however, tests must be amended to establish that precondition). The comment incorrectly suggests this is a fix for #6260. The actual fix was 4392ba4. In addition, fix a bug whereby materializing an output directory as a symlink would fail because the directory already exists. PiperOrigin-RevId: 541623490 Change-Id: I25abd3b7b5fc068a84f41c86b0050c110443218f --- .../lib/remote/RemoteExecutionService.java | 12 +-- .../remote/RemoteExecutionServiceTest.java | 79 +++++++++++++++---- 2 files changed, 68 insertions(+), 23 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 00001e90186828..eb55f4f40a2a6d 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 @@ -849,6 +849,12 @@ private void createSymlinks(Iterable symlinks) throws IOExcepti "Failed creating directory and parents for %s", symlink.path()) .createDirectoryAndParents(); + // If a directory output is being materialized as a symlink, we must first delete the + // preexisting empty directory. + if (symlink.path().exists(Symlinks.NOFOLLOW) + && symlink.path().isDirectory(Symlinks.NOFOLLOW)) { + symlink.path().delete(); + } symlink.path().createSymbolicLink(symlink.target()); } } @@ -1166,12 +1172,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re Map realToTmpPath = new HashMap<>(); if (downloadOutputs) { - // Create output directories first. - // This ensures that the directories are present even if downloading fails. - // See https://github.com/bazelbuild/bazel/issues/6260. - for (Entry entry : metadata.directories()) { - entry.getKey().createDirectoryAndParents(); - } downloadsBuilder.addAll( buildFilesToDownload(context, progressStatusListener, metadata, realToTmpPath)); } else { 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 ad8899c76b7066..b2845ab99fa867 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 @@ -303,6 +303,7 @@ public void downloadOutputs_outputFiles_executableBitIgnored() throws Exception FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act service.downloadOutputs(action, result); @@ -331,6 +332,7 @@ public void downloadOutputs_siblingLayoutAndRelativeToInputRoot_works() throws E FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act service.downloadOutputs(action, result); @@ -396,6 +398,7 @@ public void downloadOutputs_emptyOutputDirectories_works() throws Exception { FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act service.downloadOutputs(action, result); @@ -442,6 +445,7 @@ public void downloadOutputs_nestedOutputDirectories_works() throws Exception { FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act service.downloadOutputs(action, result); @@ -479,6 +483,7 @@ public void downloadOutputs_outputDirectoriesWithNestedFile_works() throws Excep FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act service.downloadOutputs(action, result); @@ -521,13 +526,14 @@ public void downloadOutputs_outputDirectoriesWithSameHash_works() throws Excepti .build(); Digest treeDigest = cache.addContents(remoteActionExecutionContext, tree.toByteArray()); ActionResult.Builder builder = ActionResult.newBuilder(); - builder.addOutputDirectoriesBuilder().setPath("outputs/a/").setTreeDigest(treeDigest); + builder.addOutputDirectoriesBuilder().setPath("outputs/a").setTreeDigest(treeDigest); RemoteActionResult result = RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build())); Spawn spawn = newSpawnFromResult(result); FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act service.downloadOutputs(action, result); @@ -570,6 +576,7 @@ public void downloadOutputs_relativeDirectorySymlink_success() throws Exception FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // Doesn't check for dangling links, hence download succeeds. service.downloadOutputs(action, result); @@ -592,6 +599,7 @@ public void downloadOutputs_relativeOutputSymlinks_success() throws Exception { FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // Doesn't check for dangling links, hence download succeeds. service.downloadOutputs(action, result); @@ -618,6 +626,7 @@ public void downloadOutputs_outputSymlinksCompatibility_success() throws Excepti FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // Doesn't check for dangling links, hence download succeeds. service.downloadOutputs(action, result); @@ -647,6 +656,7 @@ public void downloadOutputs_relativeSymlinkInDirectory_success() throws Exceptio remoteOptions.incompatibleRemoteDisallowSymlinkInTreeArtifact = false; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // Doesn't check for dangling links, hence download succeeds. service.downloadOutputs(action, result); @@ -667,6 +677,7 @@ public void downloadOutputs_absoluteFileSymlink_success() throws Exception { FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); service.downloadOutputs(action, result); @@ -686,6 +697,7 @@ public void downloadOutputs_absoluteDirectorySymlink_success() throws Exception FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); service.downloadOutputs(action, result); @@ -711,6 +723,7 @@ public void downloadOutputs_absoluteSymlinkInDirectory_error() throws Exception FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); IOException expected = assertThrows(IOException.class, () -> service.downloadOutputs(action, result)); @@ -743,37 +756,44 @@ public void downloadOutputs_symlinkCollision_error() throws Exception { assertThat(expected).hasMessageThat().contains("foo1"); assertThat(expected).hasMessageThat().contains("foo2"); } - + @Test public void downloadOutputs_onFailure_maintainDirectories() throws Exception { - // Test that output directories are not deleted on download failure. See - // https://github.com/bazelbuild/bazel/issues/6260. - Tree tree = Tree.newBuilder().setRoot(Directory.getDefaultInstance()).build(); + // Test that output directories created prior to spawn execution are not deleted on failure. + Digest treeFileDigest = + cache.addException("outputs/outputdir/outputfile", new IOException("download failed")); + Tree tree = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addFiles( + FileNode.newBuilder() + .setName("outputfile") + .setDigest(treeFileDigest) + .setIsExecutable(true))) + .build(); Digest treeDigest = cache.addContents(remoteActionExecutionContext, tree.toByteArray()); - Digest outputFileDigest = - cache.addException("outputdir/outputfile", new IOException("download failed")); - Digest otherFileDigest = cache.addContents(remoteActionExecutionContext, "otherfile"); + Digest otherFileDigest = + cache.addException("outputs/otherdir/otherfile", new IOException("download failed")); ActionResult.Builder builder = ActionResult.newBuilder(); builder.addOutputDirectoriesBuilder().setPath("outputs/outputdir").setTreeDigest(treeDigest); builder.addOutputFiles( - OutputFile.newBuilder() - .setPath("outputs/outputdir/outputfile") - .setDigest(outputFileDigest)); - builder.addOutputFiles( - OutputFile.newBuilder().setPath("outputs/otherfile").setDigest(otherFileDigest)); + OutputFile.newBuilder().setPath("outputs/otherdir/otherfile").setDigest(otherFileDigest)); RemoteActionResult result = RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build())); Spawn spawn = newSpawnFromResult(result); FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); assertThrows(BulkTransferException.class, () -> service.downloadOutputs(action, result)); - assertThat(cache.getNumFailedDownloads()).isEqualTo(1); + assertThat(cache.getNumFailedDownloads()).isEqualTo(2); assertThat(execRoot.getRelative("outputs/outputdir").exists()).isTrue(); assertThat(execRoot.getRelative("outputs/outputdir/outputfile").exists()).isFalse(); - assertThat(execRoot.getRelative("outputs/otherfile").exists()).isFalse(); + assertThat(execRoot.getRelative("outputs/otherdir").exists()).isTrue(); + assertThat(execRoot.getRelative("outputs/otherdir/otherfile").exists()).isFalse(); assertThat(context.isLockOutputFilesCalled()).isFalse(); } @@ -798,6 +818,7 @@ public void downloadOutputs_onError_waitForRemainingDownloadsToComplete() throws FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); BulkTransferException downloadException = assertThrows(BulkTransferException.class, () -> service.downloadOutputs(action, result)); @@ -829,6 +850,7 @@ public void downloadOutputs_withMultipleErrors_addsThemAsSuppressed() throws Exc FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); BulkTransferException e = assertThrows(BulkTransferException.class, () -> service.downloadOutputs(action, result)); @@ -859,6 +881,7 @@ public void downloadOutputs_withDuplicateIOErrors_doesNotSuppress() throws Excep FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); BulkTransferException downloadException = assertThrows(BulkTransferException.class, () -> service.downloadOutputs(action, result)); @@ -889,6 +912,7 @@ public void downloadOutputs_withDuplicateInterruptions_doesNotSuppress() throws FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); InterruptedException e = assertThrows(InterruptedException.class, () -> service.downloadOutputs(action, result)); @@ -919,6 +943,7 @@ public void downloadOutputs_withStdoutStderrOnSuccess_writable() throws Exceptio FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn, spyOutErr); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); service.downloadOutputs(action, result); @@ -959,6 +984,7 @@ public void downloadOutputs_withStdoutStderrOnFailure_writableAndEmpty() throws FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn, spyOutErr); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); assertThrows(BulkTransferException.class, () -> service.downloadOutputs(action, result)); @@ -991,6 +1017,7 @@ public void downloadOutputs_outputNameClashesWithTempName_success() throws Excep FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); service.downloadOutputs(action, result); @@ -1019,6 +1046,7 @@ public void downloadOutputs_outputFilesWithoutTopLevel_inject() throws Exception remoteOptions.remoteOutputsMode = RemoteOutputsMode.TOPLEVEL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1059,6 +1087,7 @@ public void downloadOutputs_outputFilesWithMinimal_injectingMetadata() throws Ex remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1121,6 +1150,7 @@ public void downloadOutputs_outputDirectoriesWithMinimal_injectingMetadata() thr remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1139,8 +1169,7 @@ public void downloadOutputs_outputDirectoriesWithMinimal_injectingMetadata() thr eq(toBinaryDigest(d2)), eq(d2.getSizeBytes()), anyLong()); - Path outputBase = checkNotNull(artifactRoot.getRoot().asPath()); - assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); + assertThat(execRoot.getRelative("outputs/dir").readdir(Symlinks.NOFOLLOW)).isEmpty(); assertThat(context.isLockOutputFilesCalled()).isTrue(); } @@ -1185,6 +1214,7 @@ public void downloadOutputs_outputDirectoriesWithMinimalOnFailure_failProperly() remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act BulkTransferException e = @@ -1216,6 +1246,7 @@ public void downloadOutputs_nonInlinedStdoutAndStderrWithMinimal_works() throws remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1255,6 +1286,7 @@ public void downloadOutputs_inlinedStdoutAndStderrWithMinimal_works() throws Exc remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1298,6 +1330,7 @@ public void downloadOutputs_inMemoryOutputWithMinimal_downloadIt() throws Except remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1358,6 +1391,7 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1392,6 +1426,7 @@ public void downloadOutputs_missingMandatoryOutputs_reportError() throws Excepti FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); IOException error = assertThrows(IOException.class, () -> service.downloadOutputs(action, result)); @@ -2150,4 +2185,14 @@ private RemoteExecutionService newRemoteExecutionService(RemoteOptions remoteOpt null, DUMMY_REMOTE_OUTPUT_CHECKER); } + + private void createOutputDirectories(Spawn spawn) throws IOException { + for (ActionInput input : spawn.getOutputFiles()) { + Path dir = execRoot.getRelative(input.getExecPath()); + if (!input.isDirectory()) { + dir = dir.getParentDirectory(); + } + dir.createDirectoryAndParents(); + } + } }