Skip to content

Commit

Permalink
Remote: Register "remote" strategy even if remote execution is not av…
Browse files Browse the repository at this point in the history 
…ailable.

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 bazelbuild#13340 and bazelbuild#13487.

Closes bazelbuild#13490.

PiperOrigin-RevId: 378339904
  • Loading branch information
@coeuvre
coeuvre committed Jul 16, 2021
1 parent 8572440 commit 5e617d8
Show file tree
Hide file tree
Showing 7 changed files with 146 additions and 22 deletions.
29 changes: 19 additions & 10 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -168,6 +171,10 @@ RemoteCache getRemoteCache() {
return cache;
}

RemoteExecutionClient getRemoteExecutionClient() {
return executor;
}

void setFilesToDownload(ImmutableSet<ActionInput> topLevelOutputs) {
this.filesToDownload = Preconditions.checkNotNull(topLevelOutputs, "filesToDownload");
}
Expand All @@ -181,7 +188,9 @@ public void executionPhaseStarting(

@Override
public void executionPhaseEnding() {
cache.close();
if (cache != null) {
cache.close();
}
if (executor != null) {
executor.close();
}
Expand Down
31 changes: 26 additions & 5 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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<ActionInput> filesToDownload;

Expand All @@ -92,7 +93,7 @@ public RemoteExecutionService(
String commandId,
DigestUtil digestUtil,
RemoteOptions remoteOptions,
RemoteCache remoteCache,
@Nullable RemoteCache remoteCache,
@Nullable RemoteExecutionClient remoteExecutor,
ImmutableSet<ActionInput> filesToDownload) {
this.execRoot = execRoot;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand All @@ -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(
Expand Down Expand Up @@ -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<Path> outputFiles =
action.spawn.getOutputFiles().stream()
.map((inp) -> execRoot.getRelative(inp.getExecPath()))
Expand All @@ -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<Digest, Message> additionalInputs = Maps.newHashMapWithExpectedSize(2);
Expand All @@ -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()
Expand Down Expand Up @@ -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());

Expand Down
8 changes: 6 additions & 2 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -809,7 +813,7 @@ public void registerSpawnStrategies(
env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions");
registryBuilder.setRemoteLocalFallbackStrategyIdentifier(
remoteOptions.remoteLocalFallbackStrategy);
actionContextProvider.registerRemoteSpawnStrategyIfApplicable(registryBuilder);
actionContextProvider.registerRemoteSpawnStrategy(registryBuilder);
}

@Override
Expand All @@ -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(),
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ private SpawnResult downloadAndFinalizeSpawnResult(

@Override
public boolean canExec(Spawn spawn) {
return Spawns.mayBeExecutedRemotely(spawn);
return remoteExecutionService.mayBeExecutedRemotely(spawn);
}

@Override
Expand Down
43 changes: 42 additions & 1 deletion src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ServerCapabilities> 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<String, String> clientEnv = ImmutableMap.of();
Expand Down
48 changes: 48 additions & 0 deletions src/test/shell/bazel/remote/remote_execution_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 5e617d8

Please sign in to comment.