From 7643d3abb654dec9cf7b49e2fce684dd45e36bc0 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 be7de3c267d7c8..1db6b1cd262b63 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 @@ private 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 060e20bf05e116..420fe5faa48522 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; @@ -82,7 +83,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; @@ -93,7 +94,7 @@ public RemoteExecutionService( String commandId, DigestUtil digestUtil, RemoteOptions remoteOptions, - RemoteCache remoteCache, + @Nullable RemoteCache remoteCache, @Nullable RemoteExecutionClient remoteExecutor, ImmutableSet filesToDownload) { this.execRoot = execRoot; @@ -214,6 +215,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, ForbiddenActionInputException { @@ -335,6 +351,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); @@ -350,6 +367,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( @@ -384,6 +402,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())) @@ -405,7 +424,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); @@ -424,7 +444,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() @@ -458,6 +478,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 e8b5a94c59237e..bc0394b32296d8 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 @@ -269,6 +269,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; } @@ -459,6 +461,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) { @@ -779,7 +783,7 @@ public void registerSpawnStrategies( env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions"); registryBuilder.setRemoteLocalFallbackStrategyIdentifier( remoteOptions.remoteLocalFallbackStrategy); - actionContextProvider.registerRemoteSpawnStrategyIfApplicable(registryBuilder); + actionContextProvider.registerRemoteSpawnStrategy(registryBuilder); } @Override @@ -806,7 +810,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 340210ba3a13db..b83d9ca87861e3 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 @@ -27,7 +27,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; @@ -78,8 +77,10 @@ final class RemoteSpawnCache implements SpawnCache { @Override public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) throws InterruptedException, IOException, ExecException, ForbiddenActionInputException { - 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 ea31f5dc3ee393..5dbc437b085687 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 @@ -359,7 +359,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 342c4813dc92bf..5b41d27efd6483 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 @@ -430,13 +430,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 abb1b01032830a..51e871fabd22a8 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