From bdca95f0fecf4491505450fecdebd92496a31994 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 | 7 +- .../lib/remote/RemoteExecutionService.java | 51 +++++--- .../remote/RemoteExecutionServiceTest.java | 116 +++++++++++++++++- ...SpawnRunnerWithGrpcRemoteExecutorTest.java | 3 +- .../build/remote/worker/ExecutionServer.java | 27 ++-- 5 files changed, 167 insertions(+), 37 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 cf6ed356efa185..7ec67e4952579a 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 @@ -28,7 +28,12 @@ public class ApiVersion implements Comparable { public static final ApiVersion low = new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(0).build()); public static final ApiVersion high = - new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(0).build()); + 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; 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 b9032d0653dfc9..b6d7e2ded00a79 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); + } + 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); + } } - outputPaths.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,20 @@ 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 supportStatus = ClientApiVersion.current.checkServerSupportedVersions(capabilities); + if (supportStatus.isSupported()) { + useOutputPaths = + supportStatus.getHighestSupportedVersion().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/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 208b1c7a8a013a..37fee120e0d057 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,23 @@ public class RemoteExecutionServiceTest { private final Reporter reporter = new Reporter(new EventBus()); private final StoredEventHandler eventHandler = new StoredEventHandler(); + // In the past, Bazel only supports RemoteApi version 2.0. + // Use this to ensure we are backward compatible with Servers that only support 2.0. + private final ApiVersion oldApiVersion = new ApiVersion(SemVer.newBuilder().setMajor(2).build()); + private final ServerCapabilities legacyRemoteExecutorCapabilities = + ServerCapabilities.newBuilder() + .setLowApiVersion(oldApiVersion.toSemVer()) + .setHighApiVersion(oldApiVersion.toSemVer()) + .setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build()) + .build(); + + private final ServerCapabilities remoteExecutorCapabilities = + ServerCapabilities.newBuilder() + .setLowApiVersion(ApiVersion.low.toSemVer()) + .setHighApiVersion(ApiVersion.high.toSemVer()) + .setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build()) + .build(); + RemoteOptions remoteOptions; private Path execRoot; private ArtifactRoot artifactRoot; @@ -177,6 +197,7 @@ public final void setUp() throws Exception { cache = spy(new InMemoryRemoteCache(spy(new InMemoryCacheClient()), remoteOptions, digestUtil)); executor = mock(RemoteExecutionClient.class); + when(executor.getServerCapabilities()).thenReturn(remoteExecutorCapabilities); RequestMetadata metadata = TracingMetadataUtils.buildMetadata("none", "none", "action-id", null); @@ -195,9 +216,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(legacyRemoteExecutorCapabilities); + 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 +252,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(legacyRemoteExecutorCapabilities); + 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 +289,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(legacyRemoteExecutorCapabilities); + 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_withActionInputAsOutput() throws Exception { Spawn spawn = @@ -246,8 +324,42 @@ public void buildRemoteAction_withActionInputAsOutput() 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(legacyRemoteExecutorCapabilities); + 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 + public void buildRemoteAction_withActionInputDirectoryAsOutput() throws Exception { + Spawn spawn = + new SpawnBuilder("dummy") + .withOutput(ActionInputHelper.fromPath(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()).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..0ca5ace6300558 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.low.toSemVer()) + .setHighApiVersion(ApiVersion.high.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..d9c4f3ca3b594a 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 @@ -271,28 +271,21 @@ private ActionResult execute( Path workingDirectory = execRoot.getRelative(command.getWorkingDirectory()); workingDirectory.createDirectoryAndParents(); - List outputs = - new ArrayList<>(command.getOutputDirectoriesCount() + command.getOutputFilesCount()); + List outputs = 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 +412,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();