From 4b367aee33d2f62a3fe6a420d555dc42384967f9 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 9 Jun 2021 00:29:55 -0700 Subject: [PATCH] Remote: Register "remote" strategy even if remote execution is not available. So that you can set a `--spawn_strategy` that includes `remote` without setting `--remote_executor`. In addition, if `--remote_local_fallback` is set and `remote_executor` is unreachable when build starts, Bazel will fallback to next available strategy. Fixes #13340 and #13487. Closes #13490. PiperOrigin-RevId: 378339904 --- .../remote/RemoteActionContextProvider.java | 29 +++++++---- .../lib/remote/RemoteExecutionService.java | 31 ++++++++++-- .../build/lib/remote/RemoteModule.java | 8 +++- .../build/lib/remote/RemoteSpawnCache.java | 7 +-- .../build/lib/remote/RemoteSpawnRunner.java | 2 +- .../build/lib/remote/RemoteModuleTest.java | 43 ++++++++++++++++- .../bazel/remote/remote_execution_test.sh | 48 +++++++++++++++++++ 7 files changed, 146 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java index c1f8462f077f34..46a3bc535a17b7 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java @@ -42,7 +42,7 @@ final class RemoteActionContextProvider implements ExecutorLifecycleListener { private final CommandEnvironment env; - private final RemoteCache cache; + @Nullable private final RemoteCache cache; @Nullable private final RemoteExecutionClient executor; @Nullable private final ListeningScheduledExecutorService retryScheduler; private final DigestUtil digestUtil; @@ -52,19 +52,27 @@ final class RemoteActionContextProvider implements ExecutorLifecycleListener { private RemoteActionContextProvider( CommandEnvironment env, - RemoteCache cache, + @Nullable RemoteCache cache, @Nullable RemoteExecutionClient executor, @Nullable ListeningScheduledExecutorService retryScheduler, DigestUtil digestUtil, @Nullable Path logDir) { this.env = Preconditions.checkNotNull(env, "env"); - this.cache = Preconditions.checkNotNull(cache, "cache"); + this.cache = cache; this.executor = executor; this.retryScheduler = retryScheduler; this.digestUtil = digestUtil; this.logDir = logDir; } + public static RemoteActionContextProvider createForPlaceholder( + CommandEnvironment env, + ListeningScheduledExecutorService retryScheduler, + DigestUtil digestUtil) { + return new RemoteActionContextProvider( + env, /*cache=*/ null, /*executor=*/ null, retryScheduler, digestUtil, /*logDir=*/ null); + } + public static RemoteActionContextProvider createForRemoteCaching( CommandEnvironment env, RemoteCache cache, @@ -125,12 +133,7 @@ RemoteExecutionService getRemoteExecutionService() { * * @param registryBuilder builder with which to register the strategy */ - public void registerRemoteSpawnStrategyIfApplicable( - SpawnStrategyRegistry.Builder registryBuilder) { - if (executor == null) { - return; // Can't use a spawn strategy without executor. - } - + public void registerRemoteSpawnStrategy(SpawnStrategyRegistry.Builder registryBuilder) { boolean verboseFailures = checkNotNull(env.getOptions().getOptions(ExecutionOptions.class)).verboseFailures; RemoteSpawnRunner spawnRunner = @@ -168,6 +171,10 @@ RemoteCache getRemoteCache() { return cache; } + RemoteExecutionClient getRemoteExecutionClient() { + return executor; + } + void setFilesToDownload(ImmutableSet topLevelOutputs) { this.filesToDownload = Preconditions.checkNotNull(topLevelOutputs, "filesToDownload"); } @@ -181,7 +188,9 @@ public void executionPhaseStarting( @Override public void executionPhaseEnding() { - cache.close(); + if (cache != null) { + cache.close(); + } if (executor != null) { executor.close(); } 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 aecd293d7ca92e..8641397973726f 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 @@ -14,6 +14,8 @@ package com.google.devtools.build.lib.remote; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; import static com.google.devtools.build.lib.remote.util.Utils.getInMemoryOutputPath; import static com.google.devtools.build.lib.remote.util.Utils.hasFilesToDownload; @@ -29,7 +31,6 @@ import build.bazel.remote.execution.v2.LogFile; import build.bazel.remote.execution.v2.Platform; import build.bazel.remote.execution.v2.RequestMetadata; -import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -81,7 +82,7 @@ public class RemoteExecutionService { private final String commandId; private final DigestUtil digestUtil; private final RemoteOptions remoteOptions; - private final RemoteCache remoteCache; + @Nullable private final RemoteCache remoteCache; @Nullable private final RemoteExecutionClient remoteExecutor; private final ImmutableSet filesToDownload; @@ -92,7 +93,7 @@ public RemoteExecutionService( String commandId, DigestUtil digestUtil, RemoteOptions remoteOptions, - RemoteCache remoteCache, + @Nullable RemoteCache remoteCache, @Nullable RemoteExecutionClient remoteExecutor, ImmutableSet filesToDownload) { this.execRoot = execRoot; @@ -213,6 +214,21 @@ public NetworkTime getNetworkTime() { } } + /** Returns {@code true} if the result of spawn may be cached remotely. */ + public boolean mayBeCachedRemotely(Spawn spawn) { + return remoteCache != null && Spawns.mayBeCached(spawn) && Spawns.mayBeCachedRemotely(spawn); + } + + /** Returns {@code true} if the result of spawn may be cached. */ + public boolean mayBeCached(Spawn spawn) { + return remoteCache != null && Spawns.mayBeCached(spawn); + } + + /** Returns {@code true} if the spawn may be executed remotely. */ + public boolean mayBeExecutedRemotely(Spawn spawn) { + return remoteCache != null && remoteExecutor != null && Spawns.mayBeExecutedRemotely(spawn); + } + /** Creates a new {@link RemoteAction} instance from spawn. */ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context) throws IOException, UserExecException { @@ -334,6 +350,7 @@ public ExecuteResponse getResponse() { @Nullable public RemoteActionResult lookupCache(RemoteAction action) throws IOException, InterruptedException { + checkNotNull(remoteCache, "remoteCache can't be null"); ActionResult actionResult = remoteCache.downloadActionResult( action.remoteActionExecutionContext, action.actionKey, /* inlineOutErr= */ false); @@ -349,6 +366,7 @@ public RemoteActionResult lookupCache(RemoteAction action) @Nullable public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult result) throws InterruptedException, IOException, ExecException { + checkNotNull(remoteCache, "remoteCache can't be null"); RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode; boolean downloadOutputs = shouldDownloadAllSpawnOutputs( @@ -383,6 +401,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re /** Upload outputs of a remote action which was executed locally to remote cache. */ public void uploadOutputs(RemoteAction action) throws InterruptedException, IOException, ExecException { + checkNotNull(remoteCache, "remoteCache can't be null"); Collection outputFiles = action.spawn.getOutputFiles().stream() .map((inp) -> execRoot.getRelative(inp.getExecPath())) @@ -404,7 +423,8 @@ public void uploadOutputs(RemoteAction action) */ public void uploadInputsIfNotPresent(RemoteAction action) throws IOException, InterruptedException { - Preconditions.checkState(remoteCache instanceof RemoteExecutionCache); + checkNotNull(remoteCache, "remoteCache can't be null"); + checkState(remoteCache instanceof RemoteExecutionCache); RemoteExecutionCache remoteExecutionCache = (RemoteExecutionCache) remoteCache; // Upload the command and all the inputs into the remote cache. Map additionalInputs = Maps.newHashMapWithExpectedSize(2); @@ -423,7 +443,7 @@ public void uploadInputsIfNotPresent(RemoteAction action) public RemoteActionResult execute( RemoteAction action, boolean acceptCachedResult, OperationObserver observer) throws IOException, InterruptedException { - Preconditions.checkNotNull(remoteExecutor, "remoteExecutor"); + checkNotNull(remoteExecutor, "remoteExecutor can't be null"); ExecuteRequest.Builder requestBuilder = ExecuteRequest.newBuilder() @@ -457,6 +477,7 @@ public static class ServerLogs { /** Downloads server logs from a remotely executed action if any. */ public ServerLogs maybeDownloadServerLogs(RemoteAction action, ExecuteResponse resp, Path logDir) throws InterruptedException, IOException { + checkNotNull(remoteCache, "remoteCache can't be null"); ServerLogs serverLogs = new ServerLogs(); serverLogs.directory = logDir.getRelative(action.getActionId()); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index 350e1afa51c49b..6be32bbd129c70 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -267,6 +267,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { if (!enableDiskCache && !enableHttpCache && !enableGrpcCache && !enableRemoteExecution) { // Quit if no remote caching or execution was enabled. + actionContextProvider = + RemoteActionContextProvider.createForPlaceholder(env, retryScheduler, digestUtil); return; } @@ -457,6 +459,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { errorMessage += System.lineSeparator() + Throwables.getStackTraceAsString(e); } env.getReporter().handle(Event.warn(errorMessage)); + actionContextProvider = + RemoteActionContextProvider.createForPlaceholder(env, retryScheduler, digestUtil); return; } else { if (verboseFailures) { @@ -809,7 +813,7 @@ public void registerSpawnStrategies( env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions"); registryBuilder.setRemoteLocalFallbackStrategyIdentifier( remoteOptions.remoteLocalFallbackStrategy); - actionContextProvider.registerRemoteSpawnStrategyIfApplicable(registryBuilder); + actionContextProvider.registerRemoteSpawnStrategy(registryBuilder); } @Override @@ -836,7 +840,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB Preconditions.checkNotNull( env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions"); RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode; - if (!remoteOutputsMode.downloadAllOutputs()) { + if (!remoteOutputsMode.downloadAllOutputs() && actionContextProvider.getRemoteCache() != null) { actionInputFetcher = new RemoteActionInputFetcher( env.getBuildRequestId(), diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index d6b3e1c92361a1..235bc627a2dd24 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java @@ -26,7 +26,6 @@ import com.google.devtools.build.lib.actions.SpawnMetrics; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.SpawnResult.Status; -import com.google.devtools.build.lib.actions.Spawns; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.events.Event; @@ -77,8 +76,10 @@ final class RemoteSpawnCache implements SpawnCache { @Override public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) throws InterruptedException, IOException, ExecException { - if (!Spawns.mayBeCached(spawn) - || (!Spawns.mayBeCachedRemotely(spawn) && useRemoteCache(options))) { + boolean mayBeCached = + remoteExecutionService.mayBeCachedRemotely(spawn) + || (!useRemoteCache(options) && remoteExecutionService.mayBeCached(spawn)); + if (!mayBeCached) { // returning SpawnCache.NO_RESULT_NO_STORE in case the caching is disabled or in case // the remote caching is disabled and the only configured cache is remote. return SpawnCache.NO_RESULT_NO_STORE; diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index 0dc4c8ab57ddfe..c34767e9817415 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -358,7 +358,7 @@ private SpawnResult downloadAndFinalizeSpawnResult( @Override public boolean canExec(Spawn spawn) { - return Spawns.mayBeExecutedRemotely(spawn); + return remoteExecutionService.mayBeExecutedRemotely(spawn); } @Override diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java index 2071b673c5321b..67183541f2b4e8 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java @@ -447,13 +447,54 @@ public void getCapabilities( remoteModule.beforeCommand(env); assertThat(Thread.interrupted()).isFalse(); - assertThat(remoteModule.getActionContextProvider()).isNull(); + RemoteActionContextProvider actionContextProvider = remoteModule.getActionContextProvider(); + assertThat(actionContextProvider).isNotNull(); + assertThat(actionContextProvider.getRemoteCache()).isNull(); + assertThat(actionContextProvider.getRemoteExecutionClient()).isNull(); } finally { cacheServer.shutdownNow(); cacheServer.awaitTermination(); } } + @Test + public void testLocalFallback_shouldIgnoreInaccessibleGrpcRemoteExecutor() throws Exception { + CapabilitiesImplBase executionServerCapabilitiesImpl = + new CapabilitiesImplBase() { + @Override + public void getCapabilities( + GetCapabilitiesRequest request, StreamObserver responseObserver) { + responseObserver.onError(new UnsupportedOperationException()); + } + }; + String executionServerName = "execution-server"; + Server executionServer = createFakeServer(executionServerName, executionServerCapabilitiesImpl); + executionServer.start(); + + try { + RemoteModule remoteModule = new RemoteModule(); + RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); + remoteOptions.remoteExecutor = executionServerName; + remoteOptions.remoteLocalFallback = true; + remoteModule.setChannelFactory( + (target, proxy, options, interceptors) -> + InProcessChannelBuilder.forName(target).directExecutor().build()); + + CommandEnvironment env = createTestCommandEnvironment(remoteOptions); + + remoteModule.beforeCommand(env); + + assertThat(Thread.interrupted()).isFalse(); + RemoteActionContextProvider actionContextProvider = remoteModule.getActionContextProvider(); + assertThat(actionContextProvider).isNotNull(); + assertThat(actionContextProvider.getRemoteCache()).isNull(); + assertThat(actionContextProvider.getRemoteExecutionClient()).isNull(); + } finally { + executionServer.shutdownNow(); + executionServer.awaitTermination(); + } + } + @Test public void testNetrc_emptyEnv_shouldIgnore() throws Exception { Map clientEnv = ImmutableMap.of(); diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 67c514757fc6e7..ba53cc638e4bba 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -467,6 +467,54 @@ EOF && fail "Expected failure" || true } +function test_local_fallback_if_no_remote_executor() { + # Test that when manually set --spawn_strategy that includes remote, but remote_executor isn't set, we ignore + # the remote strategy rather than reporting an error. See https://github.com/bazelbuild/bazel/issues/13340. + mkdir -p gen1 + cat > gen1/BUILD <<'EOF' +genrule( +name = "gen1", +srcs = [], +outs = ["out1"], +cmd = "touch \"$@\"", +) +EOF + + bazel build \ + --spawn_strategy=remote,local \ + --build_event_text_file=gen1.log \ + //gen1 >& $TEST_log \ + || fail "Expected success" + + mv gen1.log $TEST_log + expect_log "2 processes: 1 internal, 1 local" +} + +function test_local_fallback_if_remote_executor_unavailable() { + # Test that when --remote_local_fallback is set and remote_executor is unavailable when build starts, we fallback to + # local strategy. See https://github.com/bazelbuild/bazel/issues/13487. + mkdir -p gen1 + cat > gen1/BUILD <<'EOF' +genrule( +name = "gen1", +srcs = [], +outs = ["out1"], +cmd = "touch \"$@\"", +) +EOF + + bazel build \ + --spawn_strategy=remote,local \ + --remote_executor=grpc://noexist.invalid \ + --remote_local_fallback \ + --build_event_text_file=gen1.log \ + //gen1 >& $TEST_log \ + || fail "Expected success" + + mv gen1.log $TEST_log + expect_log "2 processes: 1 internal, 1 local" +} + function is_file_uploaded() { h=$(shasum -a256 < $1) if [ -e "$cas_path/${h:0:64}" ]; then return 0; else return 1; fi