From db797286b4b1aaca66a1fb8fea197d65b439c70d Mon Sep 17 00:00:00 2001 From: Son Luong Ngoc Date: Sat, 29 Apr 2023 13:32:05 +0200 Subject: [PATCH] Conditionally set output_paths based on Remote Executor capabilities This is a follow up to #18269, toward the discussion in #18202. Bump the Remote API supported version to v2.1. Based on the Capability of the Remote Executor, either use output_paths field or the legacy fields output_files and output_directories. --- .../devtools/build/lib/remote/ApiVersion.java | 12 ++- .../lib/remote/RemoteExecutionService.java | 48 ++++++--- .../BuildWithoutTheBytesIntegrationTest.java | 8 +- .../remote/RemoteExecutionServiceTest.java | 100 +++++++++++++++++- ...SpawnRunnerWithGrpcRemoteExecutorTest.java | 3 +- .../build/remote/worker/ExecutionServer.java | 26 ++--- 6 files changed, 156 insertions(+), 41 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/ApiVersion.java b/src/main/java/com/google/devtools/build/lib/remote/ApiVersion.java index e56c4fb04d2f2c..f75865df9be6dd 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ApiVersion.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ApiVersion.java @@ -17,9 +17,7 @@ import build.bazel.remote.execution.v2.ServerCapabilities; import build.bazel.semver.SemVer; -/** - * Represents a version of the Remote Execution API. - */ +/** Represents a version of the Remote Execution API. */ public class ApiVersion implements Comparable { public final int major; public final int minor; @@ -28,7 +26,12 @@ public class ApiVersion implements Comparable { // The current version of the Remote Execution API. This field will need to be updated // together with all version changes. - public static final ApiVersion current = new ApiVersion(SemVer.newBuilder().setMajor(2).build()); + public static final ApiVersion current = + new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(1).build()); + // The version of the Remote Execution API that starts supporting the + // Command.output_paths and ActionResult.output_symlinks fields. + public static final ApiVersion twoPointOne = + new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(1).build()); public ApiVersion(int major, int minor, int patch, String prerelease) { this.major = major; @@ -100,6 +103,7 @@ private enum State { UNSUPPORTED, DEPRECATED, } + private final String message; private final State state; 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 0923429a757d3b..ace1c0528c374f 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 @@ -221,30 +221,37 @@ public RemoteExecutionService( } static Command buildCommand( + boolean useOutputPaths, Collection outputs, List arguments, ImmutableMap env, @Nullable Platform platform, RemotePathResolver remotePathResolver) { Command.Builder command = Command.newBuilder(); - ArrayList outputFiles = new ArrayList<>(); - ArrayList outputDirectories = new ArrayList<>(); - ArrayList outputPaths = new ArrayList<>(); - for (ActionInput output : outputs) { - String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output)); - if (output.isDirectory()) { - outputDirectories.add(pathString); - } else { - outputFiles.add(pathString); + if (useOutputPaths) { + var outputPaths = new ArrayList(); + for (ActionInput output : outputs) { + String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output)); + outputPaths.add(pathString); } - outputPaths.add(pathString); + Collections.sort(outputPaths); + command.addAllOutputPaths(outputPaths); + } else { + var outputFiles = new ArrayList(); + var outputDirectories = new ArrayList(); + for (ActionInput output : outputs) { + String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output)); + if (output.isDirectory()) { + outputDirectories.add(pathString); + } else { + outputFiles.add(pathString); + } + } + Collections.sort(outputFiles); + Collections.sort(outputDirectories); + command.addAllOutputFiles(outputFiles); + command.addAllOutputDirectories(outputDirectories); } - Collections.sort(outputFiles); - Collections.sort(outputDirectories); - Collections.sort(outputPaths); - command.addAllOutputFiles(outputFiles); - command.addAllOutputDirectories(outputDirectories); - command.addAllOutputPaths(outputPaths); if (platform != null) { command.setPlatform(platform); @@ -542,8 +549,17 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context platform = PlatformUtils.getPlatformProto(spawn, remoteOptions); } + var useOutputPaths = true; + if (mayBeExecutedRemotely(spawn)) { + var capabilities = remoteExecutor.getServerCapabilities(); + if (capabilities != null) { + var highApiVersion = new ApiVersion(capabilities.getHighApiVersion()); + useOutputPaths = highApiVersion.compareTo(ApiVersion.twoPointOne) >= 0; + } + } Command command = buildCommand( + useOutputPaths, spawn.getOutputFiles(), spawn.getArguments(), spawn.getEnvironment(), diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java index 72aaba62b77ad4..0213979a495e6c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java @@ -381,7 +381,13 @@ public void symlinkToNestedFile() throws Exception { "my_rule(name = 'one_remote', local = False, chain_length = 1)", "my_rule(name = 'two_remote', local = False, chain_length = 2)"); - buildTarget("//a:one_local", "//a:two_local", "//a:one_remote", "//a:two_remote"); + try { + buildTarget("//a:one_local", "//a:two_local", "//a:one_remote", "//a:two_remote"); + } catch (Exception e) { + System.out.println("SLUONG DEBUG:" + worker.getStderr()); + System.out.println("SLUONG DEBUG:" + worker.getStdout()); + throw e; + } } @Test 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 56c919284e529f..726187759c24be 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 @@ -39,6 +39,7 @@ import build.bazel.remote.execution.v2.Digest; import build.bazel.remote.execution.v2.Directory; import build.bazel.remote.execution.v2.DirectoryNode; +import build.bazel.remote.execution.v2.ExecutionCapabilities; import build.bazel.remote.execution.v2.FileNode; import build.bazel.remote.execution.v2.NodeProperties; import build.bazel.remote.execution.v2.NodeProperty; @@ -47,9 +48,11 @@ import build.bazel.remote.execution.v2.OutputSymlink; import build.bazel.remote.execution.v2.Platform; import build.bazel.remote.execution.v2.RequestMetadata; +import build.bazel.remote.execution.v2.ServerCapabilities; import build.bazel.remote.execution.v2.SymlinkAbsolutePathStrategy; import build.bazel.remote.execution.v2.SymlinkNode; import build.bazel.remote.execution.v2.Tree; +import build.bazel.semver.SemVer; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableClassToInstanceMap; import com.google.common.collect.ImmutableList; @@ -138,6 +141,20 @@ public class RemoteExecutionServiceTest { private final Reporter reporter = new Reporter(new EventBus()); private final StoredEventHandler eventHandler = new StoredEventHandler(); + private final ServerCapabilities removeExecutorCapabilities = + ServerCapabilities.newBuilder() + .setLowApiVersion(ApiVersion.current.toSemVer()) + .setHighApiVersion(ApiVersion.current.toSemVer()) + .setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build()) + .build(); + private final ApiVersion oldApiVersion = new ApiVersion(SemVer.newBuilder().setMajor(2).build()); + private final ServerCapabilities legacyRemoveExecutorCapabilities = + ServerCapabilities.newBuilder() + .setLowApiVersion(oldApiVersion.toSemVer()) + .setHighApiVersion(oldApiVersion.toSemVer()) + .setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build()) + .build(); + RemoteOptions remoteOptions; private Path execRoot; private ArtifactRoot artifactRoot; @@ -177,6 +194,7 @@ public final void setUp() throws Exception { cache = spy(new InMemoryRemoteCache(spy(new InMemoryCacheClient()), remoteOptions, digestUtil)); executor = mock(RemoteExecutionClient.class); + when(executor.getServerCapabilities()).thenReturn(removeExecutorCapabilities); RequestMetadata metadata = TracingMetadataUtils.buildMetadata("none", "none", "action-id", null); @@ -195,9 +213,27 @@ public void buildRemoteAction_withRegularFileAsOutput() throws Exception { RemoteAction remoteAction = service.buildRemoteAction(spawn, context); - assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly(execPath.toString()); + assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty(); + assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty(); assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly(execPath.toString()); + } + + @Test + public void legacy_buildRemoteAction_withRegularFileAsOutput() throws Exception { + when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities); + PathFragment execPath = execRoot.getRelative("path/to/tree").asFragment(); + Spawn spawn = + new SpawnBuilder("dummy") + .withOutput(ActionsTestUtil.createArtifactWithExecPath(artifactRoot, execPath)) + .build(); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteExecutionService service = newRemoteExecutionService(); + + RemoteAction remoteAction = service.buildRemoteAction(spawn, context); + + assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly(execPath.toString()); assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty(); + assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty(); } @Test @@ -213,8 +249,28 @@ public void buildRemoteAction_withTreeArtifactAsOutput() throws Exception { RemoteAction remoteAction = service.buildRemoteAction(spawn, context); + assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty(); + assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty(); + assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/dir"); + } + + @Test + public void legacy_buildRemoteAction_withTreeArtifactAsOutput() throws Exception { + when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities); + Spawn spawn = + new SpawnBuilder("dummy") + .withOutput( + ActionsTestUtil.createTreeArtifactWithGeneratingAction( + artifactRoot, PathFragment.create("path/to/dir"))) + .build(); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteExecutionService service = newRemoteExecutionService(); + + RemoteAction remoteAction = service.buildRemoteAction(spawn, context); + assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty(); assertThat(remoteAction.getCommand().getOutputDirectoriesList()).containsExactly("path/to/dir"); + assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty(); } @Test @@ -230,11 +286,30 @@ public void buildRemoteAction_withUnresolvedSymlinkAsOutput() throws Exception { RemoteAction remoteAction = service.buildRemoteAction(spawn, context); - assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/link"); + assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty(); assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty(); assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/link"); } + @Test + public void legacy_buildRemoteAction_withUnresolvedSymlinkAsOutput() throws Exception { + when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities); + Spawn spawn = + new SpawnBuilder("dummy") + .withOutput( + ActionsTestUtil.createUnresolvedSymlinkArtifactWithExecPath( + artifactRoot, PathFragment.create("path/to/link"))) + .build(); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteExecutionService service = newRemoteExecutionService(); + + RemoteAction remoteAction = service.buildRemoteAction(spawn, context); + + assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/link"); + assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty(); + assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty(); + } + @Test public void buildRemoteAction_withActionInputFileAsOutput() throws Exception { Spawn spawn = @@ -246,8 +321,26 @@ public void buildRemoteAction_withActionInputFileAsOutput() throws Exception { RemoteAction remoteAction = service.buildRemoteAction(spawn, context); + assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty(); + assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty(); + assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/file"); + } + + @Test + public void legacy_buildRemoteAction_withActionInputFileAsOutput() throws Exception { + when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities); + Spawn spawn = + new SpawnBuilder("dummy") + .withOutput(ActionInputHelper.fromPath(PathFragment.create("path/to/file"))) + .build(); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteExecutionService service = newRemoteExecutionService(); + + RemoteAction remoteAction = service.buildRemoteAction(spawn, context); + assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/file"); assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty(); + assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty(); } @Test @@ -262,7 +355,8 @@ public void buildRemoteAction_withActionInputDirectoryAsOutput() throws Exceptio RemoteAction remoteAction = service.buildRemoteAction(spawn, context); assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty(); - assertThat(remoteAction.getCommand().getOutputDirectoriesList()).containsExactly("path/to/dir"); + assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty(); + assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/dir"); } @Test 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 38eeace4a50f9e..e0b0f832511260 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 @@ -302,6 +302,8 @@ public int maxConcurrency() { }); ServerCapabilities caps = ServerCapabilities.newBuilder() + .setLowApiVersion(ApiVersion.current.toSemVer()) + .setHighApiVersion(ApiVersion.current.toSemVer()) .setExecutionCapabilities( ExecutionCapabilities.newBuilder().setExecEnabled(true).build()) .build(); @@ -353,7 +355,6 @@ public int maxConcurrency() { .setName("VARIABLE") .setValue("value") .build()) - .addAllOutputFiles(ImmutableList.of("bar", "foo")) .addAllOutputPaths(ImmutableList.of("bar", "foo")) .build(); cmdDigest = DIGEST_UTIL.compute(command); diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index 7baded7192b37e..ba84d143f874da 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -272,27 +272,21 @@ private ActionResult execute( workingDirectory.createDirectoryAndParents(); List outputs = - new ArrayList<>(command.getOutputDirectoriesCount() + command.getOutputFilesCount()); + new ArrayList<>(command.getOutputPathsCount()); - for (String output : command.getOutputFilesList()) { + for (String output : command.getOutputPathsList()) { Path file = workingDirectory.getRelative(output); - if (file.exists()) { + // Since https://github.com/bazelbuild/bazel/pull/15818, + // Bazel includes all expected output directories as part of Action's inputs. + // + // Ensure no output file exists before execution happen. + // Ignore if output directories pre-exist. + if (file.exists() && !file.isDirectory()) { throw new FileAlreadyExistsException("Output file already exists: " + file); } file.getParentDirectory().createDirectoryAndParents(); outputs.add(file); } - for (String output : command.getOutputDirectoriesList()) { - Path file = workingDirectory.getRelative(output); - if (file.exists()) { - if (!file.isDirectory()) { - throw new FileAlreadyExistsException( - "Non-directory exists at output directory path: " + file); - } - } - file.getParentDirectory().createDirectoryAndParents(); - outputs.add(file); - } // TODO(ulfjack): This is basically a copy of LocalSpawnRunner. Ideally, we'd use that // implementation instead of copying it. @@ -419,8 +413,8 @@ private static long getUid() throws InterruptedException { com.google.devtools.build.lib.shell.Command cmd = new com.google.devtools.build.lib.shell.Command( new String[] {"id", "-u"}, - /*environmentVariables=*/ null, - /*workingDirectory=*/ null, + /* environmentVariables= */ null, + /* workingDirectory= */ null, uidTimeout); try { ByteArrayOutputStream stdout = new ByteArrayOutputStream();