Skip to content

Commit

Permalink
Remove superfluous code in RemoteExecutionService.
Browse files Browse the repository at this point in the history 
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 bazelbuild#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
  • Loading branch information
@tjgq @copybara-github
tjgq authored and copybara-github committed Jun 19, 2023
1 parent beb4659 commit 41ce54d
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 23 deletions.
12 changes: 6 additions & 6 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -849,6 +849,12 @@ private void createSymlinks(Iterable<SymlinkMetadata> 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());
}
}
Expand Down Expand Up @@ -1166,12 +1172,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
Map<Path, Path> 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<Path, DirectoryMetadata> entry : metadata.directories()) {
entry.getKey().createDirectoryAndParents();
}
downloadsBuilder.addAll(
buildFilesToDownload(context, progressStatusListener, metadata, realToTmpPath));
} else {
Expand Down
79 changes: 62 additions & 17 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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));
Expand Down Expand Up @@ -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();
}

Expand All @@ -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));
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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));

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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();
}

Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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();
}
}
}

0 comments on commit 41ce54d

Please sign in to comment.