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 set `--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.
  • Loading branch information
@coeuvre
coeuvre committed May 19, 2021
1 parent b6f87f1 commit 399747d
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 27 deletions.
24 changes: 15 additions & 9 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,8 @@ RemoteExecutionService getRemoteExecutionService() {
*
* @param registryBuilder builder with which to register the strategy
*/
public void registerRemoteSpawnStrategyIfApplicable(
public void registerRemoteSpawnStrategy(
SpawnStrategyRegistry.Builder registryBuilder) {
if (executor == null) {
return; // Can't use a spawn strategy without executor.
}

boolean verboseFailures =
checkNotNull(env.getOptions().getOptions(ExecutionOptions.class)).verboseFailures;
RemoteSpawnRunner spawnRunner =
Expand Down Expand Up @@ -181,7 +185,9 @@ public void executionPhaseStarting(

@Override
public void executionPhaseEnding() {
cache.close();
if (cache != null) {
cache.close();
}
if (executor != null) {
executor.close();
}
Expand Down
25 changes: 20 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 @@ -13,7 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.remote;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.*;
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 Down Expand Up @@ -81,7 +81,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 +92,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 +213,16 @@ 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 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 +344,7 @@ public ExecuteResponse getResponse() {
@Nullable
public RemoteActionResult lookupCache(RemoteAction action)
throws IOException, InterruptedException {
checkNotNull(remoteCache, "remoteCache");
ActionResult actionResult =
remoteCache.downloadActionResult(
action.remoteActionExecutionContext, action.actionKey, /* inlineOutErr= */ false);
Expand All @@ -349,6 +360,7 @@ public RemoteActionResult lookupCache(RemoteAction action)
@Nullable
public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult result)
throws InterruptedException, IOException, ExecException {
checkNotNull(remoteCache, "remoteCache");
RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
boolean downloadOutputs =
shouldDownloadAllSpawnOutputs(
Expand Down Expand Up @@ -383,6 +395,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");
Collection<Path> outputFiles =
action.spawn.getOutputFiles().stream()
.map((inp) -> execRoot.getRelative(inp.getExecPath()))
Expand All @@ -404,7 +417,8 @@ public void uploadOutputs(RemoteAction action)
*/
public void uploadInputsIfNotPresent(RemoteAction action)
throws IOException, InterruptedException {
Preconditions.checkState(remoteCache instanceof RemoteExecutionCache);
checkNotNull(remoteCache, "remoteCache");
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 +437,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");

ExecuteRequest.Builder requestBuilder =
ExecuteRequest.newBuilder()
Expand Down Expand Up @@ -457,6 +471,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");
ServerLogs serverLogs = new ServerLogs();
serverLogs.directory = logDir.getRelative(action.getActionId());

Expand Down
9 changes: 4 additions & 5 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 @@ -53,10 +53,7 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.exec.ExecutionOptions;
import com.google.devtools.build.lib.exec.ExecutorBuilder;
import com.google.devtools.build.lib.exec.ModuleActionContextRegistry;
import com.google.devtools.build.lib.exec.SpawnStrategyRegistry;
import com.google.devtools.build.lib.exec.*;
import com.google.devtools.build.lib.packages.TargetUtils;
import com.google.devtools.build.lib.remote.RemoteServerCapabilities.ServerCapabilitiesRequirement;
import com.google.devtools.build.lib.remote.common.RemoteCacheClient;
Expand Down Expand Up @@ -269,6 +266,7 @@ 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 @@ -459,6 +457,7 @@ 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 @@ -779,7 +778,7 @@ public void registerSpawnStrategies(
env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions");
registryBuilder.setRemoteLocalFallbackStrategyIdentifier(
remoteOptions.remoteLocalFallbackStrategy);
actionContextProvider.registerRemoteSpawnStrategyIfApplicable(registryBuilder);
actionContextProvider.registerRemoteSpawnStrategy(registryBuilder);
}

@Override
Expand Down
8 changes: 1 addition & 7 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,7 @@ 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))) {
if (!remoteExecutionService.mayBeCachedRemotely(spawn)) {
// 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 Expand Up @@ -238,10 +236,6 @@ private void report(Event evt) {
}
}

private static boolean useRemoteCache(RemoteOptions options) {
return !isNullOrEmpty(options.remoteCache) || !isNullOrEmpty(options.remoteExecutor);
}

private static boolean useDiskCache(RemoteOptions options) {
return options.diskCache != null && !options.diskCache.isEmpty();
}
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

0 comments on commit 399747d

Please sign in to comment.