diff options
7 files changed, 143 insertions, 58 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java index cb40215ad8..4be5e977c4 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java @@ -58,6 +58,17 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte this.spawnRunner = spawnRunner; } + /** + * Get's the {@link SpawnRunner} that this {@link AbstractSpawnStrategy} uses to actually run + * spawns. + * + * <p>This is considered a stop-gap until we refactor the entire SpawnStrategy / SpawnRunner + * mechanism to no longer need Spawn strategies. + */ + public SpawnRunner getSpawnRunner() { + return spawnRunner; + } + @Override public List<SpawnResult> exec(Spawn spawn, ActionExecutionContext actionExecutionContext) throws ExecException, InterruptedException { 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 88bc907a1e..2b44f481cd 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 @@ -15,22 +15,24 @@ package com.google.devtools.build.lib.remote; import static com.google.common.base.Preconditions.checkNotNull; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.ActionContext; -import com.google.devtools.build.lib.actions.ResourceManager; +import com.google.devtools.build.lib.actions.ExecutionStrategy; +import com.google.devtools.build.lib.actions.ExecutorInitException; +import com.google.devtools.build.lib.exec.AbstractSpawnStrategy; import com.google.devtools.build.lib.exec.ActionContextProvider; import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.SpawnRunner; -import com.google.devtools.build.lib.exec.apple.XcodeLocalEnvProvider; -import com.google.devtools.build.lib.exec.local.LocalEnvProvider; -import com.google.devtools.build.lib.exec.local.LocalExecutionOptions; -import com.google.devtools.build.lib.exec.local.LocalSpawnRunner; -import com.google.devtools.build.lib.exec.local.PosixLocalEnvProvider; -import com.google.devtools.build.lib.exec.local.WindowsLocalEnvProvider; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.runtime.CommandEnvironment; -import com.google.devtools.build.lib.util.OS; +import com.google.devtools.build.lib.util.ExitCode; import com.google.devtools.build.lib.vfs.Path; +import java.util.Arrays; +import java.util.Collections; +import java.util.SortedSet; +import java.util.TreeSet; +import java.util.concurrent.atomic.AtomicReference; import javax.annotation.Nullable; /** @@ -43,6 +45,7 @@ final class RemoteActionContextProvider extends ActionContextProvider { private final RemoteRetrier retrier; private final DigestUtil digestUtil; private final Path logDir; + private final AtomicReference<SpawnRunner> fallbackRunner = new AtomicReference<>(); RemoteActionContextProvider( CommandEnvironment env, @@ -64,7 +67,7 @@ final class RemoteActionContextProvider extends ActionContextProvider { ExecutionOptions executionOptions = checkNotNull(env.getOptions().getOptions(ExecutionOptions.class)); RemoteOptions remoteOptions = checkNotNull(env.getOptions().getOptions(RemoteOptions.class)); - String buildRequestId = env.getBuildRequestId().toString(); + String buildRequestId = env.getBuildRequestId(); String commandId = env.getCommandId().toString(); if (executor == null && cache != null) { @@ -84,7 +87,7 @@ final class RemoteActionContextProvider extends ActionContextProvider { env.getExecRoot(), remoteOptions, env.getOptions().getOptions(ExecutionOptions.class), - createFallbackRunner(env), + fallbackRunner, executionOptions.verboseFailures, env.getReporter(), buildRequestId, @@ -98,21 +101,37 @@ final class RemoteActionContextProvider extends ActionContextProvider { } } - private static SpawnRunner createFallbackRunner(CommandEnvironment env) { - LocalExecutionOptions localExecutionOptions = - env.getOptions().getOptions(LocalExecutionOptions.class); - LocalEnvProvider localEnvProvider = - OS.getCurrent() == OS.DARWIN - ? new XcodeLocalEnvProvider(env.getClientEnv()) - : (OS.getCurrent() == OS.WINDOWS - ? new WindowsLocalEnvProvider(env.getClientEnv()) - : new PosixLocalEnvProvider(env.getClientEnv())); - return - new LocalSpawnRunner( - env.getExecRoot(), - localExecutionOptions, - ResourceManager.instance(), - localEnvProvider); + @Override + public void executorCreated(Iterable<ActionContext> usedContexts) throws ExecutorInitException { + SortedSet<String> validStrategies = new TreeSet<>(); + fallbackRunner.set(null); + + RemoteOptions remoteOptions = env.getOptions().getOptions(RemoteOptions.class); + String strategyName = remoteOptions.remoteLocalFallbackStrategy; + + for (ActionContext context : usedContexts) { + if (context instanceof AbstractSpawnStrategy) { + ExecutionStrategy annotation = context.getClass().getAnnotation(ExecutionStrategy.class); + if (annotation != null) { + Collections.addAll(validStrategies, annotation.name()); + if (!strategyName.equals("remote") + && Arrays.asList(annotation.name()).contains(strategyName)) { + AbstractSpawnStrategy spawnStrategy = (AbstractSpawnStrategy) context; + SpawnRunner spawnRunner = Preconditions.checkNotNull(spawnStrategy.getSpawnRunner()); + fallbackRunner.set(spawnRunner); + } + } + } + } + + if (fallbackRunner.get() == null) { + validStrategies.remove("remote"); + throw new ExecutorInitException( + String.format( + "'%s' is an invalid value for --remote_local_fallback_strategy. Valid values are: %s", + strategyName, validStrategies), + ExitCode.COMMAND_LINE_ERROR); + } } @Override 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 7141d35621..14f373eb04 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 @@ -280,14 +280,8 @@ public final class RemoteModule extends BlazeModule { @Override public Iterable<Class<? extends OptionsBase>> getCommandOptions(Command command) { return "build".equals(command.name()) - ? ImmutableList.<Class<? extends OptionsBase>>of( - RemoteOptions.class, AuthAndTLSOptions.class) - : ImmutableList.<Class<? extends OptionsBase>>of(); - } - - public static boolean remoteEnabled(RemoteOptions options) { - return SimpleBlobStoreFactory.isRemoteCacheOptions(options) - || GrpcRemoteCache.isRemoteCacheOptions(options); + ? ImmutableList.of(RemoteOptions.class, AuthAndTLSOptions.class) + : ImmutableList.of(); } static RemoteRetrier createExecuteRetrier( diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java index 90f42077c4..b73784f529 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java @@ -103,6 +103,14 @@ public final class RemoteOptions extends OptionsBase { public boolean remoteLocalFallback; @Option( + name = "remote_local_fallback_strategy", + defaultValue = "local", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "The strategy to use when remote execution has to fallback to local execution.") + public String remoteLocalFallbackStrategy; + + @Option( name = "remote_upload_local_results", defaultValue = "true", documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, 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 95511d0599..e99cc6ecff 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 @@ -72,6 +72,7 @@ import java.util.Map; import java.util.SortedMap; import java.util.TreeSet; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import javax.annotation.Nullable; /** A client for the remote execution service. */ @@ -82,7 +83,7 @@ class RemoteSpawnRunner implements SpawnRunner { private final Path execRoot; private final RemoteOptions remoteOptions; private final ExecutionOptions executionOptions; - private final SpawnRunner fallbackRunner; + private final AtomicReference<SpawnRunner> fallbackRunner; private final boolean verboseFailures; @Nullable private final Reporter cmdlineReporter; @@ -101,7 +102,7 @@ class RemoteSpawnRunner implements SpawnRunner { Path execRoot, RemoteOptions remoteOptions, ExecutionOptions executionOptions, - SpawnRunner fallbackRunner, + AtomicReference<SpawnRunner> fallbackRunner, boolean verboseFailures, @Nullable Reporter cmdlineReporter, String buildRequestId, @@ -135,7 +136,7 @@ class RemoteSpawnRunner implements SpawnRunner { public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) throws ExecException, InterruptedException, IOException { if (!Spawns.mayBeExecutedRemotely(spawn) || remoteCache == null) { - return fallbackRunner.exec(spawn, context); + return fallbackRunner.get().exec(spawn, context); } context.report(ProgressStatus.EXECUTING, getName()); @@ -445,7 +446,7 @@ class RemoteSpawnRunner implements SpawnRunner { if (uploadToCache && remoteCache != null && actionKey != null) { return execLocallyAndUpload(spawn, context, inputMap, remoteCache, actionKey); } - return fallbackRunner.exec(spawn, context); + return fallbackRunner.get().exec(spawn, context); } @VisibleForTesting @@ -457,7 +458,7 @@ class RemoteSpawnRunner implements SpawnRunner { ActionKey actionKey) throws ExecException, IOException, InterruptedException { Map<Path, Long> ctimesBefore = getInputCtimes(inputMap); - SpawnResult result = fallbackRunner.exec(spawn, context); + SpawnResult result = fallbackRunner.get().exec(spawn, context); Map<Path, Long> ctimesAfter = getInputCtimes(inputMap); for (Map.Entry<Path, Long> e : ctimesBefore.entrySet()) { // Skip uploading to remote cache, because an input was modified during execution. @@ -494,7 +495,10 @@ class RemoteSpawnRunner implements SpawnRunner { } } - /** Resolve a collection of {@link com.google.build.lib.actions.ActionInput}s to {@link Path}s. */ + /** + * Resolve a collection of {@link com.google.devtools.build.lib.actions.ActionInput}s to {@link + * Path}s. + */ static Collection<Path> resolveActionInputs( Path execRoot, Collection<? extends ActionInput> actionInputs) { return actionInputs diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index 6b024b58aa..12d77f206b 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -82,6 +82,7 @@ import java.time.Duration; import java.util.Collection; import java.util.SortedMap; import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicReference; import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; @@ -168,7 +169,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -228,7 +229,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -282,7 +283,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -333,7 +334,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -369,7 +370,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), false, reporter, "build-req-id", @@ -427,7 +428,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -468,7 +469,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -506,7 +507,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -543,7 +544,7 @@ public class RemoteSpawnRunnerTest { execRoot, Options.getDefaults(RemoteOptions.class), Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -585,7 +586,7 @@ public class RemoteSpawnRunnerTest { execRoot, Options.getDefaults(RemoteOptions.class), Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -630,7 +631,7 @@ public class RemoteSpawnRunnerTest { execRoot, Options.getDefaults(RemoteOptions.class), Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -670,7 +671,7 @@ public class RemoteSpawnRunnerTest { execRoot, Options.getDefaults(RemoteOptions.class), Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -712,7 +713,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -755,7 +756,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -804,7 +805,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -851,7 +852,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -893,7 +894,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -931,7 +932,7 @@ public class RemoteSpawnRunnerTest { execRoot, options, Options.getDefaults(ExecutionOptions.class), - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", @@ -966,7 +967,7 @@ public class RemoteSpawnRunnerTest { execRoot, Options.getDefaults(RemoteOptions.class), executionOptions, - localRunner, + new AtomicReference<>(localRunner), true, /*cmdlineReporter=*/ null, "build-req-id", diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index d5e0b9fad9..9b2ac86c65 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -171,6 +171,54 @@ EOF # TODO(ulfjack): Check that the test failure gets reported correctly. } +function test_local_fallback_works_with_local_strategy() { + mkdir -p gen1 + cat > gen1/BUILD <<'EOF' +genrule( +name = "gen1", +srcs = [], +outs = ["out1"], +cmd = "touch \"$@\"", +tags = ["no-remote"], +) +EOF + + bazel build \ + --spawn_strategy=remote \ + --remote_executor=localhost:${worker_port} \ + --remote_local_fallback_strategy=local \ + --build_event_text_file=gen1.log \ + //gen1 >& $TEST_log \ + || fail "Expected success" + + mv gen1.log $TEST_log + expect_log "1 process: 1 local" +} + +function test_local_fallback_works_with_sandboxed_strategy() { + mkdir -p gen2 + cat > gen2/BUILD <<'EOF' +genrule( +name = "gen2", +srcs = [], +outs = ["out2"], +cmd = "touch \"$@\"", +tags = ["no-remote"], +) +EOF + + bazel build \ + --spawn_strategy=remote \ + --remote_executor=localhost:${worker_port} \ + --remote_local_fallback_strategy=sandboxed \ + --build_event_text_file=gen2.log \ + //gen2 >& $TEST_log \ + || fail "Expected success" + + mv gen2.log $TEST_log + expect_log "1 process: 1 .*-sandbox" +} + function is_file_uploaded() { h=$(shasum -a256 < $1) if [ -e "$cas_path/${h:0:64}" ]; then return 0; else return 1; fi |