From c577870063428e79274650a71276ab722fade6e8 Mon Sep 17 00:00:00 2001 From: Son Luong Ngoc Date: Wed, 24 Jul 2024 15:05:38 +0200 Subject: [PATCH] RemoteCacheClient: add getServerCapabilities --- .../devtools/build/lib/remote/ApiVersion.java | 17 ++- .../build/lib/remote/GrpcCacheClient.java | 6 + .../build/lib/remote/RemoteCache.java | 8 ++ .../lib/remote/RemoteExecutionService.java | 118 +++++++++++++--- .../lib/remote/common/RemoteCacheClient.java | 3 + .../lib/remote/disk/DiskCacheClient.java | 6 + .../lib/remote/http/HttpCacheClient.java | 6 + .../lib/remote/options/RemoteOptions.java | 10 ++ .../remote/RemoteExecutionServiceTest.java | 126 +++++++++++++++++- .../lib/remote/RemoteSpawnCacheTest.java | 12 +- .../lib/remote/RemoteSpawnRunnerTest.java | 14 +- ...SpawnRunnerWithGrpcRemoteExecutorTest.java | 4 +- .../lib/remote/util/InMemoryCacheClient.java | 6 + .../bazel/remote/remote_execution_test.sh | 50 ++++++- .../remote/worker/CapabilitiesServer.java | 35 +++-- .../build/remote/worker/ExecutionServer.java | 57 +++++--- .../build/remote/worker/RemoteWorker.java | 2 +- .../remote/worker/RemoteWorkerOptions.java | 107 +++++++-------- 18 files changed, 465 insertions(+), 122 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 ee4ed593a81827..d49980f2feb8c5 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 @@ -23,12 +23,21 @@ public class ApiVersion implements Comparable { public final int patch; public final String prerelease; + // The version of the Remote Execution API that Bazel supports initially. + public static final ApiVersion twoPointZero = + new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(0).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()); + // The latest version of the Remote Execution API that Bazel is compatible with. + public static final ApiVersion twoPointTwo = + new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(2).build()); + // The current lowest/highest versions (inclusive) of the Remote Execution API that Bazel // supports. These fields will need to be updated together with all version changes. - 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()); + public static final ApiVersion low = twoPointZero; + public static final ApiVersion high = twoPointTwo; 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/GrpcCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java index bee979da2f557b..71b3f2ccf8c0a3 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java @@ -29,6 +29,7 @@ import build.bazel.remote.execution.v2.FindMissingBlobsResponse; import build.bazel.remote.execution.v2.GetActionResultRequest; import build.bazel.remote.execution.v2.RequestMetadata; +import build.bazel.remote.execution.v2.ServerCapabilities; import build.bazel.remote.execution.v2.UpdateActionResultRequest; import com.google.bytestream.ByteStreamGrpc; import com.google.bytestream.ByteStreamGrpc.ByteStreamStub; @@ -270,6 +271,11 @@ public CacheCapabilities getCacheCapabilities() throws IOException { return channel.getServerCapabilities().getCacheCapabilities(); } + @Override + public ServerCapabilities getServerCapabilities() throws IOException { + return channel.getServerCapabilities(); + } + @Override public ListenableFuture getAuthority() { return channel.withChannelFuture(ch -> Futures.immediateFuture(ch.authority())); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index 9906990dcfa578..0262fe1a5fb9d5 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -26,6 +26,7 @@ import build.bazel.remote.execution.v2.ActionResult; import build.bazel.remote.execution.v2.CacheCapabilities; import build.bazel.remote.execution.v2.Digest; +import build.bazel.remote.execution.v2.ServerCapabilities; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.flogger.GoogleLogger; @@ -120,6 +121,13 @@ public ListenableFuture getRemoteAuthority() { return remoteCacheClient.getAuthority(); } + public ServerCapabilities getServerCapabilities() throws IOException { + if (remoteCacheClient == null) { + return ServerCapabilities.getDefaultInstance(); + } + return remoteCacheClient.getServerCapabilities(); + } + public CachedActionResult downloadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) throws IOException, InterruptedException { 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 fbd223196c00a2..90bb0ab8533315 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 @@ -156,6 +156,7 @@ import java.util.concurrent.Semaphore; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Stream; import javax.annotation.Nullable; /** @@ -196,6 +197,8 @@ public class RemoteExecutionService { @Nullable private final Scrubber scrubber; private final Set knownMissingCasDigests; + private Boolean useOutputPaths; + public RemoteExecutionService( Executor executor, Reporter reporter, @@ -242,6 +245,7 @@ public RemoteExecutionService( } private Command buildCommand( + boolean useOutputPaths, Collection outputs, List arguments, ImmutableMap env, @@ -249,25 +253,32 @@ private Command buildCommand( RemotePathResolver remotePathResolver, @Nullable SpawnScrubber spawnScrubber) { Command.Builder command = Command.newBuilder(); - ArrayList outputFiles = new ArrayList<>(); - ArrayList outputDirectories = new ArrayList<>(); - ArrayList outputPaths = new ArrayList<>(); - for (ActionInput output : outputs) { - String pathString = - reencodeInternalToExternal(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 = + reencodeInternalToExternal(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 = + reencodeInternalToExternal(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,6 +553,63 @@ private void maybeReleaseRemoteActionBuildingSemaphore() { remoteActionBuildingSemaphore.release(); } + private synchronized void initUseOutputPaths() { + // If this has already been initialized, return + if (this.useOutputPaths != null) { + return; + } + this.useOutputPaths = remoteOptions.useOutputPaths; + try { + // If both Remote Executor and Remote Cache are configured, + // use the highest version that is supported by both server to compare. + if (remoteExecutor != null && remoteCache != null) { + var executorSupportStatus = + ClientApiVersion.current.checkServerSupportedVersions( + remoteExecutor.getServerCapabilities()); + var cacheSupportStatus = + ClientApiVersion.current.checkServerSupportedVersions( + remoteCache.getServerCapabilities()); + + ApiVersion highestSupportedApiVersion = ApiVersion.low; + if (executorSupportStatus.isSupported() && cacheSupportStatus.isSupported()) { + var executorHighestVersion = executorSupportStatus.getHighestSupportedVersion(); + var cacheHighestVersion = cacheSupportStatus.getHighestSupportedVersion(); + + var commonServerVersion = + executorHighestVersion.compareTo(cacheHighestVersion) <= 0 + ? executorHighestVersion + : cacheHighestVersion; + this.useOutputPaths = commonServerVersion.compareTo(ApiVersion.twoPointOne) >= 0; + return; + } + } + + if (remoteExecutor != null) { + var capabilities = remoteExecutor.getServerCapabilities(); + if (capabilities != null) { + var supportStatus = ClientApiVersion.current.checkServerSupportedVersions(capabilities); + if (supportStatus.isSupported()) { + this.useOutputPaths = + supportStatus.getHighestSupportedVersion().compareTo(ApiVersion.twoPointOne) >= 0; + return; + } + } + } + if (remoteCache != null) { + var capabilities = remoteCache.getServerCapabilities(); + if (capabilities != null) { + var supportStatus = ClientApiVersion.current.checkServerSupportedVersions(capabilities); + if (supportStatus.isSupported()) { + this.useOutputPaths = + supportStatus.getHighestSupportedVersion().compareTo(ApiVersion.twoPointOne) >= 0; + } + } + } + } catch (IOException e) { + // If we can't determine the server capabilities, use the default value. + } + } + /** Creates a new {@link RemoteAction} instance from spawn. */ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context) throws IOException, ExecException, ForbiddenActionInputException, InterruptedException { @@ -568,8 +636,12 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context platform = PlatformUtils.getPlatformProto(spawn, remoteOptions); } + if (this.useOutputPaths == null) { + initUseOutputPaths(); + } Command command = buildCommand( + this.useOutputPaths, spawn.getOutputFiles(), spawn.getArguments(), spawn.getEnvironment(), @@ -1509,8 +1581,18 @@ public SpawnResult waitForAndReuseOutputs(RemoteAction action, LocalExecution pr Map realToTmpPath = new HashMap<>(); ByteString inMemoryOutputContent = null; String inMemoryOutputPath = null; + if (this.useOutputPaths == null) { + initUseOutputPaths(); + } + var outputPathsList = + this.useOutputPaths + ? action.getCommand().getOutputPathsList() + : Stream.concat( + action.getCommand().getOutputFilesList().stream(), + action.getCommand().getOutputDirectoriesList().stream()) + .toList(); try { - for (String output : action.getCommand().getOutputPathsList()) { + for (String output : outputPathsList) { String reencodedOutput = reencodeExternalToInternal(output); Path sourcePath = previousExecution.action.getRemotePathResolver().outputPathToLocalPath(reencodedOutput); diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java index 7ea79ac648674e..d0fc235e8ffd48 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java @@ -18,6 +18,7 @@ import build.bazel.remote.execution.v2.ActionResult; import build.bazel.remote.execution.v2.CacheCapabilities; import build.bazel.remote.execution.v2.Digest; +import build.bazel.remote.execution.v2.ServerCapabilities; import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.ListenableFuture; @@ -35,6 +36,8 @@ public interface RemoteCacheClient extends MissingDigestsFinder { CacheCapabilities getCacheCapabilities() throws IOException; + ServerCapabilities getServerCapabilities() throws IOException; + ListenableFuture getAuthority(); /** diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java index 5f6264607d4f2a..5afed543d5203d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java @@ -22,6 +22,7 @@ import build.bazel.remote.execution.v2.CacheCapabilities; import build.bazel.remote.execution.v2.Digest; import build.bazel.remote.execution.v2.Directory; +import build.bazel.remote.execution.v2.ServerCapabilities; import build.bazel.remote.execution.v2.SymlinkAbsolutePathStrategy; import build.bazel.remote.execution.v2.Tree; import com.google.common.base.Ascii; @@ -253,6 +254,11 @@ public CacheCapabilities getCacheCapabilities() { .build(); } + @Override + public ServerCapabilities getServerCapabilities() { + return ServerCapabilities.newBuilder().setCacheCapabilities(getCacheCapabilities()).build(); + } + @Override public ListenableFuture getAuthority() { return immediateFuture(""); diff --git a/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java index 3d036a49923f45..b83f2afb8fb792 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java @@ -17,6 +17,7 @@ import build.bazel.remote.execution.v2.ActionResult; import build.bazel.remote.execution.v2.CacheCapabilities; import build.bazel.remote.execution.v2.Digest; +import build.bazel.remote.execution.v2.ServerCapabilities; import build.bazel.remote.execution.v2.SymlinkAbsolutePathStrategy; import com.google.auth.Credentials; import com.google.common.collect.ImmutableList; @@ -614,6 +615,11 @@ public CacheCapabilities getCacheCapabilities() { .build(); } + @Override + public ServerCapabilities getServerCapabilities() { + return ServerCapabilities.newBuilder().setCacheCapabilities(getCacheCapabilities()).build(); + } + @Override public ListenableFuture getAuthority() { return Futures.immediateFuture(""); diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index d88aea890df295..a8cf8350e5b6af 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -413,6 +413,16 @@ public RemoteBuildEventUploadModeConverter() { + " --experimental_remote_cache_compression_threshold.") public boolean cacheCompression; + @Option( + name = "incompatible_remote_use_output_paths", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "If enabled, check if the remote server(s) support remote execution API v2.1 or newer." + + "If yes, use the newer Command.output_paths field. Default is false.") + public boolean useOutputPaths; + @Option( name = "experimental_remote_cache_compression_threshold", // Based on discussions in #18997, `~100` is the break even point where the compression 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 3f60417fa2140f..689fe4c9207ba8 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 @@ -38,6 +38,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; @@ -46,6 +47,7 @@ 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.SymlinkNode; import build.bazel.remote.execution.v2.Tree; import com.google.common.base.Throwables; @@ -153,6 +155,22 @@ 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 ServerCapabilities legacyRemoteExecutorCapabilities = + ServerCapabilities.newBuilder() + .setLowApiVersion(ApiVersion.twoPointZero.toSemVer()) + .setHighApiVersion(ApiVersion.twoPointZero.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 FileSystem fs; private Path execRoot; @@ -171,6 +189,7 @@ public final void setUp() throws Exception { reporter.addHandler(eventHandler); remoteOptions = Options.getDefaults(RemoteOptions.class); + remoteOptions.useOutputPaths = true; fs = new InMemoryFileSystem(new JavaClock(), DigestHashFunction.SHA256); @@ -196,7 +215,9 @@ public final void setUp() throws Exception { outErr = new FileOutErr(stdout, stderr); cache = spy(new InMemoryRemoteCache(spy(new InMemoryCacheClient()), remoteOptions, digestUtil)); + doReturn(remoteExecutorCapabilities).when(cache).getServerCapabilities(); executor = mock(RemoteExecutionClient.class); + when(executor.getServerCapabilities()).thenReturn(remoteExecutorCapabilities); RequestMetadata metadata = TracingMetadataUtils.buildMetadata("none", "none", "action-id", null); @@ -215,9 +236,28 @@ 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 { + doReturn(legacyRemoteExecutorCapabilities).when(cache).getServerCapabilities(); + 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 @@ -233,8 +273,29 @@ 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 { + doReturn(legacyRemoteExecutorCapabilities).when(cache).getServerCapabilities(); + 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 @@ -250,11 +311,31 @@ 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 { + doReturn(legacyRemoteExecutorCapabilities).when(cache).getServerCapabilities(); + 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 = @@ -266,8 +347,43 @@ 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 { + doReturn(legacyRemoteExecutorCapabilities).when(cache).getServerCapabilities(); + 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 @@ -2367,10 +2483,8 @@ public void buildRemoteActionWithPathMapping(@TestParameter boolean remoteMerkle .containsExactly( PathFragment.create("outputs/bin/input1"), mappedInput, PathFragment.create("outputs/bin/input2"), unmappedInput); - assertThat(remoteAction.getCommand().getOutputFilesList()) - .containsExactly("outputs/bin/dir/output1", "outputs/bin/other_dir/output2"); - assertThat(remoteAction.getCommand().getOutputDirectoriesList()) - .containsExactly("outputs/bin/output_dir"); + assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty(); + assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty(); assertThat(remoteAction.getCommand().getOutputPathsList()) .containsExactly( "outputs/bin/dir/output1", "outputs/bin/other_dir/output2", "outputs/bin/output_dir"); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index 2ef60d6a0aec53..928af7df70ebfe 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -758,7 +758,9 @@ public void testDownloadMinimalIoError() throws Exception { @Test public void pathMappedActionIsDeduplicated() throws Exception { // arrange - RemoteSpawnCache cache = createRemoteSpawnCache(); + RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); + remoteOptions.useOutputPaths = true; + RemoteSpawnCache cache = remoteSpawnCacheWithOptions(remoteOptions); SimpleSpawn firstSpawn = simplePathMappedSpawn("k8-fastbuild"); FakeActionInputFileCache firstFakeFileCache = new FakeActionInputFileCache(execRoot); @@ -967,7 +969,9 @@ public void pathMappedActionWithInMemoryOutputIsDeduplicated() throws Exception @Test public void deduplicatedActionWithNonZeroExitCodeIsACacheMiss() throws Exception { // arrange - RemoteSpawnCache cache = createRemoteSpawnCache(); + RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); + remoteOptions.useOutputPaths = true; + RemoteSpawnCache cache = remoteSpawnCacheWithOptions(remoteOptions); SimpleSpawn firstSpawn = simplePathMappedSpawn("k8-fastbuild"); FakeActionInputFileCache firstFakeFileCache = new FakeActionInputFileCache(execRoot); @@ -1016,7 +1020,9 @@ public void deduplicatedActionWithNonZeroExitCodeIsACacheMiss() throws Exception @Test public void deduplicatedActionWithMissingOutputIsACacheMiss() throws Exception { // arrange - RemoteSpawnCache cache = createRemoteSpawnCache(); + RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); + remoteOptions.useOutputPaths = true; + RemoteSpawnCache cache = remoteSpawnCacheWithOptions(remoteOptions); SimpleSpawn firstSpawn = simplePathMappedSpawn("k8-fastbuild"); FakeActionInputFileCache firstFakeFileCache = new FakeActionInputFileCache(execRoot); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index 7ea21d5f566263..3394d19cad85b8 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -40,8 +40,10 @@ import build.bazel.remote.execution.v2.ExecuteRequest; import build.bazel.remote.execution.v2.ExecuteResponse; import build.bazel.remote.execution.v2.ExecutedActionMetadata; +import build.bazel.remote.execution.v2.ExecutionCapabilities; import build.bazel.remote.execution.v2.ExecutionStage.Value; import build.bazel.remote.execution.v2.LogFile; +import build.bazel.remote.execution.v2.ServerCapabilities; import com.google.common.collect.ClassToInstanceMap; import com.google.common.collect.ImmutableClassToInstanceMap; import com.google.common.collect.ImmutableList; @@ -172,6 +174,13 @@ public class RemoteSpawnRunnerTest { private static final String SIMPLE_ACTION_ID = "31aea267dc597b047a9b6993100415b6406f82822318dc8988e4164a535b51ee"; + private final ServerCapabilities remoteExecutorCapabilities = + ServerCapabilities.newBuilder() + .setLowApiVersion(ApiVersion.low.toSemVer()) + .setHighApiVersion(ApiVersion.high.toSemVer()) + .setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build()) + .build(); + @Before public final void setUp() throws Exception { MockitoAnnotations.initMocks(this); @@ -194,8 +203,9 @@ public final void setUp() throws Exception { remoteOptions = Options.getDefaults(RemoteOptions.class); retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); - when(cache.hasRemoteCache()).thenReturn(true); + doReturn(remoteExecutorCapabilities).when(cache).getServerCapabilities(); + when(executor.getServerCapabilities()).thenReturn(remoteExecutorCapabilities); when(cache.remoteActionCacheSupportsUpdate()).thenReturn(true); } @@ -288,6 +298,7 @@ public void nonCachableSpawnsShouldNotBeCached_localFallback() throws Exception // Test that if a non-cachable spawn is executed locally due to the local fallback, // that its result is not uploaded to the remote cache. + remoteOptions.useOutputPaths = true; remoteOptions.remoteAcceptCached = true; remoteOptions.remoteLocalFallback = true; remoteOptions.remoteUploadLocalResults = true; @@ -307,6 +318,7 @@ public void nonCachableSpawnsShouldNotBeCached_localFallback() throws Exception runner.exec(spawn, policy); verify(localRunner).exec(spawn, policy); + verify(cache).getServerCapabilities(); verify(cache).ensureInputsPresent(any(), any(), any(), anyBoolean(), any()); verify(cache, atLeastOnce()).hasRemoteCache(); verify(cache, atLeastOnce()).hasDiskCache(); 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 902b1df64803da..d82a9d0b7b92fe 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 @@ -293,6 +293,7 @@ public PathFragment getExecPath() { ImmutableList.of( Maps.immutableEntry("CacheKey1", "CacheValue1"), Maps.immutableEntry("CacheKey2", "CacheValue2")); + remoteOptions.useOutputPaths = true; retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); RemoteRetrier retrier = @@ -312,6 +313,8 @@ public Single create() { .build(); ServerCapabilities caps = ServerCapabilities.newBuilder() + .setLowApiVersion(ApiVersion.low.toSemVer()) + .setHighApiVersion(ApiVersion.high.toSemVer()) .setExecutionCapabilities( ExecutionCapabilities.newBuilder().setExecEnabled(true).build()) .build(); @@ -375,7 +378,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/test/java/com/google/devtools/build/lib/remote/util/InMemoryCacheClient.java b/src/test/java/com/google/devtools/build/lib/remote/util/InMemoryCacheClient.java index d6198a429e1834..fb8261a09f7c67 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/util/InMemoryCacheClient.java +++ b/src/test/java/com/google/devtools/build/lib/remote/util/InMemoryCacheClient.java @@ -17,6 +17,7 @@ import build.bazel.remote.execution.v2.ActionResult; import build.bazel.remote.execution.v2.CacheCapabilities; import build.bazel.remote.execution.v2.Digest; +import build.bazel.remote.execution.v2.ServerCapabilities; import build.bazel.remote.execution.v2.SymlinkAbsolutePathStrategy; import com.google.common.collect.ImmutableSet; import com.google.common.io.ByteStreams; @@ -117,6 +118,11 @@ public CacheCapabilities getCacheCapabilities() { .build(); } + @Override + public ServerCapabilities getServerCapabilities() { + return ServerCapabilities.getDefaultInstance(); + } + @Override public ListenableFuture getAuthority() { return Futures.immediateFuture(""); diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index c1ee0d73c2283f..bd8264b48dfe50 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -47,7 +47,7 @@ source "$(rlocation "io_bazel/src/test/shell/bazel/remote/remote_utils.sh")" \ || { echo "remote_utils.sh not found!" >&2; exit 1; } function set_up() { - start_worker + start_worker --legacy_api } function tear_down() { @@ -91,7 +91,7 @@ function setup_credential_helper_test() { EOF stop_worker - start_worker --expected_authorization_token=TOKEN + start_worker --legacy_api --expected_authorization_token=TOKEN } function test_credential_helper_remote_cache() { @@ -178,6 +178,48 @@ function test_credential_helper_clear_cache() { expect_credential_helper_calls 10 } +function test_remote_grpc_cache_with_legacy_api() { + # TODO(sluongng): Add this when we flip the default to use Remote API version 2.1. + # Test if Bazel works with Remote Cache using Remote Api version 2.0. + # stop_worker + # start_worker --legacy_api + + mkdir -p a + cat > a/BUILD < a/BUILD < responseObserver) { DigestFunction.Value df = digestUtil.getDigestFunction(); + + var builder = ServerCapabilities.newBuilder(); + if (workerOptions.legacyApi) { + builder + .setLowApiVersion(ApiVersion.twoPointZero.toSemVer()) + .setHighApiVersion(ApiVersion.twoPointZero.toSemVer()); + } else { + builder + .setLowApiVersion(ApiVersion.low.toSemVer()) + .setHighApiVersion(ApiVersion.high.toSemVer()); + } ServerCapabilities.Builder response = - ServerCapabilities.newBuilder() - .setLowApiVersion(ApiVersion.low.toSemVer()) - .setHighApiVersion(ApiVersion.high.toSemVer()) - .setCacheCapabilities( - CacheCapabilities.newBuilder() - .addDigestFunctions(df) - .setSymlinkAbsolutePathStrategy(SymlinkAbsolutePathStrategy.Value.DISALLOWED) - .setActionCacheUpdateCapabilities( - ActionCacheUpdateCapabilities.newBuilder().setUpdateEnabled(true).build()) - .setMaxBatchTotalSizeBytes(CasServer.MAX_BATCH_SIZE_BYTES) - .build()); + builder.setCacheCapabilities( + CacheCapabilities.newBuilder() + .addDigestFunctions(df) + .setSymlinkAbsolutePathStrategy(SymlinkAbsolutePathStrategy.Value.DISALLOWED) + .setActionCacheUpdateCapabilities( + ActionCacheUpdateCapabilities.newBuilder().setUpdateEnabled(true).build()) + .setMaxBatchTotalSizeBytes(CasServer.MAX_BATCH_SIZE_BYTES) + .build()); if (execEnabled) { response.setExecutionCapabilities( ExecutionCapabilities.newBuilder().setDigestFunction(df).setExecEnabled(true).build()); 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 7e677f135d2301..b34c7d989de79d 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 @@ -305,27 +305,44 @@ private ActionResult execute( Path workingDirectory = execRoot.getRelative(command.getWorkingDirectory()); workingDirectory.createDirectoryAndParents(); - 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); + List outputs; + if (command.getOutputPathsCount() == 0) { + outputs = + new ArrayList<>(command.getOutputDirectoriesCount() + command.getOutputFilesCount()); + for (String output : command.getOutputFilesList()) { + var file = workingDirectory.getRelative(output); + if (file.exists()) { + throw new FileAlreadyExistsException("Output file already exists: " + file); + } + file.getParentDirectory().createDirectoryAndParents(); + outputs.add(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); + 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); + } + } else { + outputs = new ArrayList<>(command.getOutputPathsCount()); + for (String output : command.getOutputPathsList()) { + var file = workingDirectory.getRelative(output); + // 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); } - file.getParentDirectory().createDirectoryAndParents(); - outputs.add(file); } // TODO(ulfjack): This is basically a copy of LocalSpawnRunner. Ideally, we'd use that @@ -454,8 +471,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(); diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java index 714abb6a99a6e9..c90d4d261246de 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java @@ -188,7 +188,7 @@ public RemoteWorker( } else { execServer = null; } - this.capabilitiesServer = new CapabilitiesServer(digestUtil, execServer != null); + this.capabilitiesServer = new CapabilitiesServer(digestUtil, execServer != null, workerOptions); } public Server startServer() throws IOException { diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorkerOptions.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorkerOptions.java index 493caea3a670bd..55f0a50ecc3a0a 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorkerOptions.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorkerOptions.java @@ -28,66 +28,70 @@ public class RemoteWorkerOptions extends OptionsBase { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); @Option( - name = "listen_port", - defaultValue = "8080", - category = "build_worker", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.UNKNOWN}, - help = "Listening port for the netty server." - ) + name = "listen_port", + defaultValue = "8080", + category = "build_worker", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "Listening port for the netty server.") public int listenPort; @Option( - name = "work_path", - defaultValue = "null", - category = "build_worker", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.UNKNOWN}, - help = "A directory for the build worker to do work." - ) + name = "work_path", + defaultValue = "null", + category = "build_worker", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "A directory for the build worker to do work.") public String workPath; @Option( - name = "cas_path", - defaultValue = "null", - category = "build_worker", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.UNKNOWN}, - help = "A directory for the build worker to store it's files in. If left unset, and if no " - + "other store is set, the worker falls back to an in-memory store." - ) + name = "cas_path", + defaultValue = "null", + category = "build_worker", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "A directory for the build worker to store it's files in. If left unset, and if no " + + "other store is set, the worker falls back to an in-memory store.") public String casPath; @Option( - name = "debug", - defaultValue = "false", - category = "build_worker", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.UNKNOWN}, - help = - "Turn this on for debugging remote job failures. There will be extra messages and the " - + "work directory will be preserved in the case of failure." - ) + name = "debug", + defaultValue = "false", + category = "build_worker", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "Turn this on for debugging remote job failures. There will be extra messages and the " + + "work directory will be preserved in the case of failure.") public boolean debug; @Option( - name = "pid_file", - defaultValue = "null", - category = "build_worker", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.UNKNOWN}, - help = "File for writing the process id for this worker when it is fully started." - ) + name = "legacy_api", + defaultValue = "false", + category = "build_worker", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "Restrict worker to RemoteApi version 2.0 capabilities") + public boolean legacyApi; + + @Option( + name = "pid_file", + defaultValue = "null", + category = "build_worker", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "File for writing the process id for this worker when it is fully started.") public String pidFile; @Option( - name = "sandboxing", - defaultValue = "false", - category = "build_worker", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.UNKNOWN}, - help = "If supported on this platform, use sandboxing for increased hermeticity." - ) + name = "sandboxing", + defaultValue = "false", + category = "build_worker", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "If supported on this platform, use sandboxing for increased hermeticity.") public boolean sandboxing; @Option( @@ -111,13 +115,12 @@ public class RemoteWorkerOptions extends OptionsBase { public List sandboxingTmpfsDirs; @Option( - name = "sandboxing_block_network", - defaultValue = "false", - category = "build_worker", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.UNKNOWN}, - help = "When using sandboxing, block network access for running actions." - ) + name = "sandboxing_block_network", + defaultValue = "false", + category = "build_worker", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "When using sandboxing, block network access for running actions.") public boolean sandboxingBlockNetwork; @Option(