From 2d4ce65192b237815958cda7703d7e1463ee0f09 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 | 8 +- .../lib/remote/RemoteExecutionService.java | 48 ++++++---- .../remote/RemoteExecutionServiceTest.java | 96 ++++++++++++++++++- ...SpawnRunnerWithGrpcRemoteExecutorTest.java | 3 +- .../build/remote/worker/ExecutionServer.java | 14 +-- 5 files changed, 134 insertions(+), 35 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..5eb5c8e5eac40c 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,8 @@ 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()); public ApiVersion(int major, int minor, int patch, String prerelease) { this.major = major; @@ -100,6 +99,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 60fd8411e392bb..b818d75cb878be 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 @@ -217,30 +217,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) { + ArrayList 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 { + ArrayList outputFiles = new ArrayList<>(); + ArrayList 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); @@ -538,8 +545,17 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context platform = PlatformUtils.getPlatformProto(spawn, remoteOptions); } + var useOutputPaths = false; + if (mayBeExecutedRemotely(spawn)) { + var capabilities = remoteExecutor.getServerCapabilities(); + if (capabilities != null) { + useOutputPaths = + ApiVersion.current.checkServerSupportedVersions(capabilities).isSupported(); + } + } 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 8b8c0e10dd8fe3..05ca335e1361ea 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; @@ -135,6 +138,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; @@ -174,6 +191,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); @@ -192,8 +210,25 @@ 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(); } @@ -210,6 +245,25 @@ 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"); } @@ -227,11 +281,29 @@ 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(); + } + @Test public void buildRemoteAction_withActionInputFileAsOutput() throws Exception { Spawn spawn = @@ -243,6 +315,23 @@ 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(); } @@ -259,7 +348,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 872669fa3fe6b1..687b8bf4155fbc 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 @@ -299,6 +299,8 @@ public int maxConcurrency() { }); ServerCapabilities caps = ServerCapabilities.newBuilder() + .setLowApiVersion(ApiVersion.current.toSemVer()) + .setHighApiVersion(ApiVersion.current.toSemVer()) .setExecutionCapabilities( ExecutionCapabilities.newBuilder().setExecEnabled(true).build()) .build(); @@ -349,7 +351,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..07ed28ce6ba3f9 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 @@ -274,15 +274,7 @@ private ActionResult execute( List outputs = new ArrayList<>(command.getOutputDirectoriesCount() + command.getOutputFilesCount()); - for (String output : command.getOutputFilesList()) { - Path file = workingDirectory.getRelative(output); - if (file.exists()) { - throw new FileAlreadyExistsException("Output file already exists: " + file); - } - file.getParentDirectory().createDirectoryAndParents(); - outputs.add(file); - } - for (String output : command.getOutputDirectoriesList()) { + for (String output : command.getOutputPathsList()) { Path file = workingDirectory.getRelative(output); if (file.exists()) { if (!file.isDirectory()) { @@ -419,8 +411,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();