Skip to content

Commit

Permalink
RemoteExecutionService: support output_symlinks in ActionResult (#18441)
Browse files Browse the repository at this point in the history 
Since Remote API v2.1, both
  - output_file_symlinks
  - output_directory_symlinks
were marked as deprecated in favor of output_symlinks.

However, Bazel downloadOutputs has only support the 2 deprecated fields
and not output_symlinks.

Add support for output_symlinks.

Closes #18198.

PiperOrigin-RevId: 527511382
Change-Id: I350ab77a5d30ab06dab4118cf76f0b0e7650a739

Co-authored-by: Son Luong Ngoc <[email protected]>
  • Loading branch information
@iancha1992 @sluongng
iancha1992 and sluongng authored Jun 6, 2023
1 parent 828ae6f commit 46c7dcf
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 11 deletions.
41 changes: 30 additions & 11 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 @@ -210,7 +210,7 @@ public RemoteExecutionService(
this.tempPathGenerator = tempPathGenerator;
this.captureCorruptedOutputsDir = captureCorruptedOutputsDir;

this.scheduler = Schedulers.from(executor, /*interruptibleWorker=*/ true);
this.scheduler = Schedulers.from(executor, /* interruptibleWorker= */ true);
}

static Command buildCommand(
Expand Down Expand Up @@ -639,6 +639,10 @@ public List<OutputSymlink> getOutputDirectorySymlinks() {
return actionResult.getOutputDirectorySymlinksList();
}

public List<OutputSymlink> getOutputSymlinks() {
return actionResult.getOutputSymlinksList();
}

/**
* Returns the freeform informational message with details on the execution of the action that
* may be displayed to the user upon failure or when requested explicitly.
Expand Down Expand Up @@ -1005,7 +1009,7 @@ ActionResultMetadata parseActionResultMetadata(
directExecutor()));
}

waitForBulkTransfer(dirMetadataDownloads.values(), /* cancelRemainingOnInterrupt=*/ true);
waitForBulkTransfer(dirMetadataDownloads.values(), /* cancelRemainingOnInterrupt= */ true);

ImmutableMap.Builder<Path, DirectoryMetadata> directories = ImmutableMap.builder();
for (Map.Entry<Path, ListenableFuture<Tree>> metadataDownload :
Expand All @@ -1029,18 +1033,33 @@ ActionResultMetadata parseActionResultMetadata(
new FileMetadata(localPath, outputFile.getDigest(), outputFile.getIsExecutable()));
}

ImmutableMap.Builder<Path, SymlinkMetadata> symlinks = ImmutableMap.builder();
Iterable<OutputSymlink> outputSymlinks =
Iterables.concat(result.getOutputFileSymlinks(), result.getOutputDirectorySymlinks());
for (OutputSymlink symlink : outputSymlinks) {
Path localPath =
var symlinkMap = new HashMap<Path, SymlinkMetadata>();
var outputSymlinks =
Iterables.concat(
result.getOutputFileSymlinks(),
result.getOutputDirectorySymlinks(),
result.getOutputSymlinks());
for (var symlink : outputSymlinks) {
var localPath =
remotePathResolver.outputPathToLocalPath(encodeBytestringUtf8(symlink.getPath()));
symlinks.put(
localPath, new SymlinkMetadata(localPath, PathFragment.create(symlink.getTarget())));
var target = PathFragment.create(symlink.getTarget());
var existingMetadata = symlinkMap.get(localPath);
if (existingMetadata != null) {
if (!target.equals(existingMetadata.target())) {
throw new IOException(
String.format(
"Symlink path collision: '%s' is mapped to both '%s' and '%s'. Action Result"
+ " should not contain multiple targets for the same symlink.",
localPath, existingMetadata.target(), target));
}
continue;
}

symlinkMap.put(localPath, new SymlinkMetadata(localPath, target));
}

return new ActionResultMetadata(
files.buildOrThrow(), symlinks.buildOrThrow(), directories.buildOrThrow());
files.buildOrThrow(), ImmutableMap.copyOf(symlinkMap), directories.buildOrThrow());
}

/**
Expand Down Expand Up @@ -1156,7 +1175,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
ListenableFuture<byte[]> inMemoryOutputDownload =
remoteCache.downloadBlob(context, inMemoryOutputDigest);
waitForBulkTransfer(
ImmutableList.of(inMemoryOutputDownload), /* cancelRemainingOnInterrupt=*/ true);
ImmutableList.of(inMemoryOutputDownload), /* cancelRemainingOnInterrupt= */ true);
byte[] data = getFromFuture(inMemoryOutputDownload);
return new InMemoryOutput(inMemoryOutput, ByteString.copyFrom(data));
}
Expand Down
76 changes: 76 additions & 0 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 @@ -575,6 +575,54 @@ public void downloadOutputs_relativeDirectorySymlink_success() throws Exception
assertThat(context.isLockOutputFilesCalled()).isTrue();
}

@Test
public void downloadOutputs_relativeOutputSymlinks_success() throws Exception {
// Test that download outputs works when the action result only contains output_symlinks
// and not output_file_symlinks or output_directory_symlinks, which are deprecated.
ActionResult.Builder builder = ActionResult.newBuilder();
builder.addOutputSymlinksBuilder().setPath("outputs/a/b/link").setTarget("../../foo");
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);

// Doesn't check for dangling links, hence download succeeds.
service.downloadOutputs(action, result);

Path path = execRoot.getRelative("outputs/a/b/link");
assertThat(path.isSymbolicLink()).isTrue();
assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("../../foo"));
assertThat(context.isLockOutputFilesCalled()).isTrue();
}

@Test
public void downloadOutputs_outputSymlinksCompatibility_success() throws Exception {
// Test that download outputs works when the action result contains both output_symlinks
// and output_file_symlinks (or output_directory_symlinks).
//
// Remote Execution Server may set both fields to ensure backward compatibility with
// clients that don't support output_symlinks.
ActionResult.Builder builder = ActionResult.newBuilder();
builder.addOutputFileSymlinksBuilder().setPath("outputs/a/b/link").setTarget("foo");
builder.addOutputSymlinksBuilder().setPath("outputs/a/b/link").setTarget("foo");
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);

// Doesn't check for dangling links, hence download succeeds.
service.downloadOutputs(action, result);

Path path = execRoot.getRelative("outputs/a/b/link");
assertThat(path.isSymbolicLink()).isTrue();
assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("foo"));
assertThat(context.isLockOutputFilesCalled()).isTrue();
}

@Test
public void downloadOutputs_relativeSymlinkInDirectory_success() throws Exception {
Tree tree =
Expand Down Expand Up @@ -666,6 +714,28 @@ public void downloadOutputs_absoluteSymlinkInDirectory_error() throws Exception
assertThat(context.isLockOutputFilesCalled()).isTrue();
}

@Test
public void downloadOutputs_symlinkCollision_error() throws Exception {
ActionResult.Builder builder = ActionResult.newBuilder();
builder.addOutputDirectorySymlinksBuilder().setPath("outputs/foo").setTarget("foo1");
builder.addOutputSymlinksBuilder().setPath("outputs/foo").setTarget("foo2");
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);

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

assertThat(expected.getSuppressed()).isEmpty();
assertThat(expected).hasMessageThat().contains("Symlink path collision");
assertThat(expected).hasMessageThat().contains("outputs/foo");
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
Expand Down Expand Up @@ -1948,6 +2018,12 @@ private Spawn newSpawnFromResult(
outputs.add(output);
}

for (OutputSymlink symlink : result.getOutputSymlinks()) {
Path path = remotePathResolver.outputPathToLocalPath(symlink.getPath());
Artifact output = ActionsTestUtil.createArtifact(artifactRoot, path);
outputs.add(output);
}

return newSpawn(executionInfo, outputs.build());
}

Expand Down

0 comments on commit 46c7dcf

Please sign in to comment.