Skip to content

Commit 5e617d8

Browse files
coeuvrecoeuvre
committed
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
1 parent 8572440 commit 5e617d8

7 files changed

Lines changed: 146 additions & 22 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@
4242
final class RemoteActionContextProvider implements ExecutorLifecycleListener {
4343

4444
private final CommandEnvironment env;
45-
private final RemoteCache cache;
45+
@Nullable private final RemoteCache cache;
4646
@Nullable private final RemoteExecutionClient executor;
4747
@Nullable private final ListeningScheduledExecutorService retryScheduler;
4848
private final DigestUtil digestUtil;
@@ -52,19 +52,27 @@ final class RemoteActionContextProvider implements ExecutorLifecycleListener {
5252

5353
private RemoteActionContextProvider(
5454
CommandEnvironment env,
55-
RemoteCache cache,
55+
@Nullable RemoteCache cache,
5656
@Nullable RemoteExecutionClient executor,
5757
@Nullable ListeningScheduledExecutorService retryScheduler,
5858
DigestUtil digestUtil,
5959
@Nullable Path logDir) {
6060
this.env = Preconditions.checkNotNull(env, "env");
61-
this.cache = Preconditions.checkNotNull(cache, "cache");
61+
this.cache = cache;
6262
this.executor = executor;
6363
this.retryScheduler = retryScheduler;
6464
this.digestUtil = digestUtil;
6565
this.logDir = logDir;
6666
}
6767

68+
public static RemoteActionContextProvider createForPlaceholder(
69+
CommandEnvironment env,
70+
ListeningScheduledExecutorService retryScheduler,
71+
DigestUtil digestUtil) {
72+
return new RemoteActionContextProvider(
73+
env, /*cache=*/ null, /*executor=*/ null, retryScheduler, digestUtil, /*logDir=*/ null);
74+
}
75+
6876
public static RemoteActionContextProvider createForRemoteCaching(
6977
CommandEnvironment env,
7078
RemoteCache cache,
@@ -125,12 +133,7 @@ RemoteExecutionService getRemoteExecutionService() {
125133
*
126134
* @param registryBuilder builder with which to register the strategy
127135
*/
128-
public void registerRemoteSpawnStrategyIfApplicable(
129-
SpawnStrategyRegistry.Builder registryBuilder) {
130-
if (executor == null) {
131-
return; // Can't use a spawn strategy without executor.
132-
}
133-
136+
public void registerRemoteSpawnStrategy(SpawnStrategyRegistry.Builder registryBuilder) {
134137
boolean verboseFailures =
135138
checkNotNull(env.getOptions().getOptions(ExecutionOptions.class)).verboseFailures;
136139
RemoteSpawnRunner spawnRunner =
@@ -168,6 +171,10 @@ RemoteCache getRemoteCache() {
168171
return cache;
169172
}
170173

174+
RemoteExecutionClient getRemoteExecutionClient() {
175+
return executor;
176+
}
177+
171178
void setFilesToDownload(ImmutableSet<ActionInput> topLevelOutputs) {
172179
this.filesToDownload = Preconditions.checkNotNull(topLevelOutputs, "filesToDownload");
173180
}
@@ -181,7 +188,9 @@ public void executionPhaseStarting(
181188

182189
@Override
183190
public void executionPhaseEnding() {
184-
cache.close();
191+
if (cache != null) {
192+
cache.close();
193+
}
185194
if (executor != null) {
186195
executor.close();
187196
}

‎src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
package com.google.devtools.build.lib.remote;
1515

1616
import static com.google.common.base.Preconditions.checkArgument;
17+
import static com.google.common.base.Preconditions.checkNotNull;
18+
import static com.google.common.base.Preconditions.checkState;
1719
import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture;
1820
import static com.google.devtools.build.lib.remote.util.Utils.getInMemoryOutputPath;
1921
import static com.google.devtools.build.lib.remote.util.Utils.hasFilesToDownload;
@@ -29,7 +31,6 @@
2931
import build.bazel.remote.execution.v2.LogFile;
3032
import build.bazel.remote.execution.v2.Platform;
3133
import build.bazel.remote.execution.v2.RequestMetadata;
32-
import com.google.common.base.Preconditions;
3334
import com.google.common.base.Strings;
3435
import com.google.common.collect.ImmutableList;
3536
import com.google.common.collect.ImmutableMap;
@@ -81,7 +82,7 @@ public class RemoteExecutionService {
8182
private final String commandId;
8283
private final DigestUtil digestUtil;
8384
private final RemoteOptions remoteOptions;
84-
private final RemoteCache remoteCache;
85+
@Nullable private final RemoteCache remoteCache;
8586
@Nullable private final RemoteExecutionClient remoteExecutor;
8687
private final ImmutableSet<ActionInput> filesToDownload;
8788

@@ -92,7 +93,7 @@ public RemoteExecutionService(
9293
String commandId,
9394
DigestUtil digestUtil,
9495
RemoteOptions remoteOptions,
95-
RemoteCache remoteCache,
96+
@Nullable RemoteCache remoteCache,
9697
@Nullable RemoteExecutionClient remoteExecutor,
9798
ImmutableSet<ActionInput> filesToDownload) {
9899
this.execRoot = execRoot;
@@ -213,6 +214,21 @@ public NetworkTime getNetworkTime() {
213214
}
214215
}
215216

217+
/** Returns {@code true} if the result of spawn may be cached remotely. */
218+
public boolean mayBeCachedRemotely(Spawn spawn) {
219+
return remoteCache != null && Spawns.mayBeCached(spawn) && Spawns.mayBeCachedRemotely(spawn);
220+
}
221+
222+
/** Returns {@code true} if the result of spawn may be cached. */
223+
public boolean mayBeCached(Spawn spawn) {
224+
return remoteCache != null && Spawns.mayBeCached(spawn);
225+
}
226+
227+
/** Returns {@code true} if the spawn may be executed remotely. */
228+
public boolean mayBeExecutedRemotely(Spawn spawn) {
229+
return remoteCache != null && remoteExecutor != null && Spawns.mayBeExecutedRemotely(spawn);
230+
}
231+
216232
/** Creates a new {@link RemoteAction} instance from spawn. */
217233
public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context)
218234
throws IOException, UserExecException {
@@ -334,6 +350,7 @@ public ExecuteResponse getResponse() {
334350
@Nullable
335351
public RemoteActionResult lookupCache(RemoteAction action)
336352
throws IOException, InterruptedException {
353+
checkNotNull(remoteCache, "remoteCache can't be null");
337354
ActionResult actionResult =
338355
remoteCache.downloadActionResult(
339356
action.remoteActionExecutionContext, action.actionKey, /* inlineOutErr= */ false);
@@ -349,6 +366,7 @@ public RemoteActionResult lookupCache(RemoteAction action)
349366
@Nullable
350367
public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult result)
351368
throws InterruptedException, IOException, ExecException {
369+
checkNotNull(remoteCache, "remoteCache can't be null");
352370
RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
353371
boolean downloadOutputs =
354372
shouldDownloadAllSpawnOutputs(
@@ -383,6 +401,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
383401
/** Upload outputs of a remote action which was executed locally to remote cache. */
384402
public void uploadOutputs(RemoteAction action)
385403
throws InterruptedException, IOException, ExecException {
404+
checkNotNull(remoteCache, "remoteCache can't be null");
386405
Collection<Path> outputFiles =
387406
action.spawn.getOutputFiles().stream()
388407
.map((inp) -> execRoot.getRelative(inp.getExecPath()))
@@ -404,7 +423,8 @@ public void uploadOutputs(RemoteAction action)
404423
*/
405424
public void uploadInputsIfNotPresent(RemoteAction action)
406425
throws IOException, InterruptedException {
407-
Preconditions.checkState(remoteCache instanceof RemoteExecutionCache);
426+
checkNotNull(remoteCache, "remoteCache can't be null");
427+
checkState(remoteCache instanceof RemoteExecutionCache);
408428
RemoteExecutionCache remoteExecutionCache = (RemoteExecutionCache) remoteCache;
409429
// Upload the command and all the inputs into the remote cache.
410430
Map<Digest, Message> additionalInputs = Maps.newHashMapWithExpectedSize(2);
@@ -423,7 +443,7 @@ public void uploadInputsIfNotPresent(RemoteAction action)
423443
public RemoteActionResult execute(
424444
RemoteAction action, boolean acceptCachedResult, OperationObserver observer)
425445
throws IOException, InterruptedException {
426-
Preconditions.checkNotNull(remoteExecutor, "remoteExecutor");
446+
checkNotNull(remoteExecutor, "remoteExecutor can't be null");
427447

428448
ExecuteRequest.Builder requestBuilder =
429449
ExecuteRequest.newBuilder()
@@ -457,6 +477,7 @@ public static class ServerLogs {
457477
/** Downloads server logs from a remotely executed action if any. */
458478
public ServerLogs maybeDownloadServerLogs(RemoteAction action, ExecuteResponse resp, Path logDir)
459479
throws InterruptedException, IOException {
480+
checkNotNull(remoteCache, "remoteCache can't be null");
460481
ServerLogs serverLogs = new ServerLogs();
461482
serverLogs.directory = logDir.getRelative(action.getActionId());
462483

‎src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
267267

268268
if (!enableDiskCache && !enableHttpCache && !enableGrpcCache && !enableRemoteExecution) {
269269
// Quit if no remote caching or execution was enabled.
270+
actionContextProvider =
271+
RemoteActionContextProvider.createForPlaceholder(env, retryScheduler, digestUtil);
270272
return;
271273
}
272274

@@ -457,6 +459,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
457459
errorMessage += System.lineSeparator() + Throwables.getStackTraceAsString(e);
458460
}
459461
env.getReporter().handle(Event.warn(errorMessage));
462+
actionContextProvider =
463+
RemoteActionContextProvider.createForPlaceholder(env, retryScheduler, digestUtil);
460464
return;
461465
} else {
462466
if (verboseFailures) {
@@ -809,7 +813,7 @@ public void registerSpawnStrategies(
809813
env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions");
810814
registryBuilder.setRemoteLocalFallbackStrategyIdentifier(
811815
remoteOptions.remoteLocalFallbackStrategy);
812-
actionContextProvider.registerRemoteSpawnStrategyIfApplicable(registryBuilder);
816+
actionContextProvider.registerRemoteSpawnStrategy(registryBuilder);
813817
}
814818

815819
@Override
@@ -836,7 +840,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
836840
Preconditions.checkNotNull(
837841
env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions");
838842
RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
839-
if (!remoteOutputsMode.downloadAllOutputs()) {
843+
if (!remoteOutputsMode.downloadAllOutputs() && actionContextProvider.getRemoteCache() != null) {
840844
actionInputFetcher =
841845
new RemoteActionInputFetcher(
842846
env.getBuildRequestId(),

‎src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import com.google.devtools.build.lib.actions.SpawnMetrics;
2727
import com.google.devtools.build.lib.actions.SpawnResult;
2828
import com.google.devtools.build.lib.actions.SpawnResult.Status;
29-
import com.google.devtools.build.lib.actions.Spawns;
3029
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
3130
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
3231
import com.google.devtools.build.lib.events.Event;
@@ -77,8 +76,10 @@ final class RemoteSpawnCache implements SpawnCache {
7776
@Override
7877
public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
7978
throws InterruptedException, IOException, ExecException {
80-
if (!Spawns.mayBeCached(spawn)
81-
|| (!Spawns.mayBeCachedRemotely(spawn) && useRemoteCache(options))) {
79+
boolean mayBeCached =
80+
remoteExecutionService.mayBeCachedRemotely(spawn)
81+
|| (!useRemoteCache(options) && remoteExecutionService.mayBeCached(spawn));
82+
if (!mayBeCached) {
8283
// returning SpawnCache.NO_RESULT_NO_STORE in case the caching is disabled or in case
8384
// the remote caching is disabled and the only configured cache is remote.
8485
return SpawnCache.NO_RESULT_NO_STORE;

‎src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ private SpawnResult downloadAndFinalizeSpawnResult(
358358

359359
@Override
360360
public boolean canExec(Spawn spawn) {
361-
return Spawns.mayBeExecutedRemotely(spawn);
361+
return remoteExecutionService.mayBeExecutedRemotely(spawn);
362362
}
363363

364364
@Override

‎src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java‎

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -447,13 +447,54 @@ public void getCapabilities(
447447
remoteModule.beforeCommand(env);
448448

449449
assertThat(Thread.interrupted()).isFalse();
450-
assertThat(remoteModule.getActionContextProvider()).isNull();
450+
RemoteActionContextProvider actionContextProvider = remoteModule.getActionContextProvider();
451+
assertThat(actionContextProvider).isNotNull();
452+
assertThat(actionContextProvider.getRemoteCache()).isNull();
453+
assertThat(actionContextProvider.getRemoteExecutionClient()).isNull();
451454
} finally {
452455
cacheServer.shutdownNow();
453456
cacheServer.awaitTermination();
454457
}
455458
}
456459

460+
@Test
461+
public void testLocalFallback_shouldIgnoreInaccessibleGrpcRemoteExecutor() throws Exception {
462+
CapabilitiesImplBase executionServerCapabilitiesImpl =
463+
new CapabilitiesImplBase() {
464+
@Override
465+
public void getCapabilities(
466+
GetCapabilitiesRequest request, StreamObserver<ServerCapabilities> responseObserver) {
467+
responseObserver.onError(new UnsupportedOperationException());
468+
}
469+
};
470+
String executionServerName = "execution-server";
471+
Server executionServer = createFakeServer(executionServerName, executionServerCapabilitiesImpl);
472+
executionServer.start();
473+
474+
try {
475+
RemoteModule remoteModule = new RemoteModule();
476+
RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class);
477+
remoteOptions.remoteExecutor = executionServerName;
478+
remoteOptions.remoteLocalFallback = true;
479+
remoteModule.setChannelFactory(
480+
(target, proxy, options, interceptors) ->
481+
InProcessChannelBuilder.forName(target).directExecutor().build());
482+
483+
CommandEnvironment env = createTestCommandEnvironment(remoteOptions);
484+
485+
remoteModule.beforeCommand(env);
486+
487+
assertThat(Thread.interrupted()).isFalse();
488+
RemoteActionContextProvider actionContextProvider = remoteModule.getActionContextProvider();
489+
assertThat(actionContextProvider).isNotNull();
490+
assertThat(actionContextProvider.getRemoteCache()).isNull();
491+
assertThat(actionContextProvider.getRemoteExecutionClient()).isNull();
492+
} finally {
493+
executionServer.shutdownNow();
494+
executionServer.awaitTermination();
495+
}
496+
}
497+
457498
@Test
458499
public void testNetrc_emptyEnv_shouldIgnore() throws Exception {
459500
Map<String, String> clientEnv = ImmutableMap.of();

‎src/test/shell/bazel/remote/remote_execution_test.sh‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,54 @@ EOF
467467
&& fail "Expected failure" || true
468468
}
469469

470+
function test_local_fallback_if_no_remote_executor() {
471+
# Test that when manually set --spawn_strategy that includes remote, but remote_executor isn't set, we ignore
472+
# the remote strategy rather than reporting an error. See https://github.com/bazelbuild/bazel/issues/13340.
473+
mkdir -p gen1
474+
cat > gen1/BUILD <<'EOF'
475+
genrule(
476+
name = "gen1",
477+
srcs = [],
478+
outs = ["out1"],
479+
cmd = "touch \"$@\"",
480+
)
481+
EOF
482+
483+
bazel build \
484+
--spawn_strategy=remote,local \
485+
--build_event_text_file=gen1.log \
486+
//gen1 >& $TEST_log \
487+
|| fail "Expected success"
488+
489+
mv gen1.log $TEST_log
490+
expect_log "2 processes: 1 internal, 1 local"
491+
}
492+
493+
function test_local_fallback_if_remote_executor_unavailable() {
494+
# Test that when --remote_local_fallback is set and remote_executor is unavailable when build starts, we fallback to
495+
# local strategy. See https://github.com/bazelbuild/bazel/issues/13487.
496+
mkdir -p gen1
497+
cat > gen1/BUILD <<'EOF'
498+
genrule(
499+
name = "gen1",
500+
srcs = [],
501+
outs = ["out1"],
502+
cmd = "touch \"$@\"",
503+
)
504+
EOF
505+
506+
bazel build \
507+
--spawn_strategy=remote,local \
508+
--remote_executor=grpc://noexist.invalid \
509+
--remote_local_fallback \
510+
--build_event_text_file=gen1.log \
511+
//gen1 >& $TEST_log \
512+
|| fail "Expected success"
513+
514+
mv gen1.log $TEST_log
515+
expect_log "2 processes: 1 internal, 1 local"
516+
}
517+
470518
function is_file_uploaded() {
471519
h=$(shasum -a256 < $1)
472520
if [ -e "$cas_path/${h:0:64}" ]; then return 0; else return 1; fi

0 commit comments

Comments
 (0)