From b2231c56d78c6d37bcb6f11e1e50fe68ee336b4a Mon Sep 17 00:00:00 2001 From: larsrc Date: Tue, 4 May 2021 02:37:47 -0700 Subject: [PATCH] Move use of legacy sandbox -> local fallback to only be used after all strategies have been tried, and improve messages around it. RELNOTES: None. PiperOrigin-RevId: 371873129 --- .../build/lib/actions/SpawnStrategy.java | 16 ++- .../lib/dynamic/DynamicSpawnStrategy.java | 6 +- .../build/lib/exec/AbstractSpawnStrategy.java | 6 + .../devtools/build/lib/exec/SpawnRunner.java | 5 + .../build/lib/exec/SpawnStrategyResolver.java | 41 +++--- .../build/lib/sandbox/SandboxModule.java | 62 +++++++-- .../build/lib/sandbox/SandboxOptions.java | 121 +++++++++--------- 7 files changed, 162 insertions(+), 95 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnStrategy.java index 2ce9e7894cc8b0..fe2eef2447c368 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/SpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnStrategy.java @@ -50,9 +50,23 @@ default SpawnContinuation beginExecution( } } - /** Returns whether this SpawnActionContext supports executing the given Spawn. */ + /** + * Returns whether this SpawnActionContext supports executing the given Spawn. This does not allow + * using the legacy fallback to local execution controlled by the {@code + * --incompatible_legacy_local_fallback} flag. + */ boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry); + /** + * Returns true if this SpawnActionContext supports executing the given Spawn through a legacy + * fallback system. This will only be used if no SpawnActionContexts were able to execute it by + * normal means. + */ + default boolean canExecWithLegacyFallback( + Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry) { + return false; + } + /** * Performs any actions conditional on this strategy not only being registered but triggered as * used because its identifier was requested and it was not overridden. diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java index 97049f35695799..aa91cf7b457679 100644 --- a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java @@ -445,7 +445,8 @@ public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionCo for (SandboxedSpawnStrategy strategy : dynamicStrategyRegistry.getDynamicSpawnActionContexts( spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) { - if (strategy.canExec(spawn, actionContextRegistry)) { + if (strategy.canExec(spawn, actionContextRegistry) + || strategy.canExecWithLegacyFallback(spawn, actionContextRegistry)) { return true; } } @@ -513,7 +514,8 @@ private static ImmutableList runSpawnLocally( for (SandboxedSpawnStrategy strategy : dynamicStrategyRegistry.getDynamicSpawnActionContexts( spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) { - if (strategy.canExec(spawn, actionExecutionContext)) { + if (strategy.canExec(spawn, actionExecutionContext) + || strategy.canExecWithLegacyFallback(spawn, actionExecutionContext)) { return strategy.exec(spawn, actionExecutionContext, stopConcurrentSpawns); } } 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 1084a63315a185..079c54a540aeb7 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 @@ -96,6 +96,12 @@ public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionCo return spawnRunner.canExec(spawn); } + @Override + public boolean canExecWithLegacyFallback( + Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry) { + return spawnRunner.canExecWithLegacyFallback(spawn); + } + @Override public ImmutableList exec(Spawn spawn, ActionExecutionContext actionExecutionContext) throws ExecException, InterruptedException { diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java index d2f5bf29c61d28..7b79837baeabdf 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java @@ -269,6 +269,11 @@ SpawnResult exec(Spawn spawn, SpawnExecutionContext context) /** Returns whether this SpawnRunner supports executing the given Spawn. */ boolean canExec(Spawn spawn); + /** Returns whether this SpawnRunner supports executing the given Spawn using legacy fallbacks. */ + default boolean canExecWithLegacyFallback(Spawn spawn) { + return false; + } + /** Returns whether this SpawnRunner handles caching of actions internally. */ boolean handlesCaching(); diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java index da5a9ca6fd98aa..a5c9ac9bccd721 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java @@ -88,26 +88,37 @@ public List resolve( .getContext(SpawnStrategyRegistry.class) .getStrategies(spawn, actionExecutionContext.getEventHandler()); - strategies = + List execableStrategies = strategies.stream() .filter(spawnActionContext -> spawnActionContext.canExec(spawn, actionExecutionContext)) .collect(Collectors.toList()); - if (strategies.isEmpty()) { - String message = - String.format( - "No usable spawn strategy found for spawn with mnemonic %s. Your" - + " --spawn_strategy, --genrule_strategy and/or --strategy flags are probably too" - + " strict. Visit https://github.com/bazelbuild/bazel/issues/7480 for" - + " migration advice", - spawn.getMnemonic()); - throw new UserExecException( - FailureDetail.newBuilder() - .setMessage(message) - .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.NO_USABLE_STRATEGY_FOUND)) - .build()); + if (execableStrategies.isEmpty()) { + // Legacy implicit fallbacks should be a last-ditch option after all other strategies are + // found non-executable. + List fallbackStrategies = + strategies.stream() + .filter( + spawnActionContext -> + spawnActionContext.canExecWithLegacyFallback(spawn, actionExecutionContext)) + .collect(Collectors.toList()); + + if (fallbackStrategies.isEmpty()) { + String message = + String.format( + "No usable spawn strategy found for spawn with mnemonic %s. Your --spawn_strategy," + + " --genrule_strategy and/or --strategy flags are probably too strict. Visit" + + " https://github.com/bazelbuild/bazel/issues/7480 for migration advice", + spawn.getMnemonic()); + throw new UserExecException( + FailureDetail.newBuilder() + .setMessage(message) + .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.NO_USABLE_STRATEGY_FOUND)) + .build()); + } + return fallbackStrategies; } - return strategies; + return execableStrategies; } } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java index d1fe0425f564ab..87542fba7d3b2b 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java @@ -429,12 +429,11 @@ private static Path getPathToDockerClient(CommandEnvironment cmdEnv) { private static SpawnRunner withFallback( CommandEnvironment env, AbstractSandboxSpawnRunner sandboxSpawnRunner) { SandboxOptions sandboxOptions = env.getOptions().getOptions(SandboxOptions.class); - if (sandboxOptions != null && sandboxOptions.legacyLocalFallback) { - return new SandboxFallbackSpawnRunner( - sandboxSpawnRunner, createFallbackRunner(env), env.getReporter()); - } else { - return sandboxSpawnRunner; - } + return new SandboxFallbackSpawnRunner( + sandboxSpawnRunner, + createFallbackRunner(env), + env.getReporter(), + sandboxOptions != null && sandboxOptions.legacyLocalFallback); } private static SpawnRunner createFallbackRunner(CommandEnvironment env) { @@ -451,19 +450,29 @@ private static SpawnRunner createFallbackRunner(CommandEnvironment env) { RunfilesTreeUpdater.INSTANCE); } + /** + * A SpawnRunner that does sandboxing if possible, but might fall back to local execution if + * ----incompatible_legacy_local_fallback is true and no other strategy has been usable. This is a + * legacy functionality from before the strategies system was added, and can deceive the user into + * thinking a build is hermetic when it isn't really. TODO(b/178356138): Flip flag to default to + * false and then later remove this code entirely. + */ private static final class SandboxFallbackSpawnRunner implements SpawnRunner { private final SpawnRunner sandboxSpawnRunner; private final SpawnRunner fallbackSpawnRunner; private final ExtendedEventHandler reporter; private static final AtomicBoolean warningEmitted = new AtomicBoolean(); + private final boolean fallbackAllowed; SandboxFallbackSpawnRunner( SpawnRunner sandboxSpawnRunner, SpawnRunner fallbackSpawnRunner, - ExtendedEventHandler reporter) { + ExtendedEventHandler reporter, + boolean fallbackAllowed) { this.sandboxSpawnRunner = sandboxSpawnRunner; this.fallbackSpawnRunner = fallbackSpawnRunner; this.reporter = reporter; + this.fallbackAllowed = fallbackAllowed; } @Override @@ -479,12 +488,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) if (sandboxSpawnRunner.canExec(spawn)) { spawnResult = sandboxSpawnRunner.exec(spawn, context); } else { - if (warningEmitted.compareAndSet(false, true)) { - reporter.handle( - Event.warn( - "Use of implicit local fallback will go away soon, please" - + " set a fallback strategy instead. See --legacy_local_fallback option.")); - } spawnResult = fallbackSpawnRunner.exec(spawn, context); } reporter.post(new SpawnExecutedEvent(spawn, spawnResult, spawnExecutionStartInstant)); @@ -493,7 +496,38 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) @Override public boolean canExec(Spawn spawn) { - return sandboxSpawnRunner.canExec(spawn) || fallbackSpawnRunner.canExec(spawn); + return sandboxSpawnRunner.canExec(spawn); + } + + @Override + public boolean canExecWithLegacyFallback(Spawn spawn) { + boolean canExec = !sandboxSpawnRunner.canExec(spawn) && fallbackSpawnRunner.canExec(spawn); + if (canExec) { + // We give a single warning to use strategies instead, whether or not we allow the fallback + // to happen. This allows people to switch early, but also explains why the build fails + // once we flip the flag. Unfortunately, we can't easily tell if the flag was explicitly + // set, if we could we should omit the warnings in that case. + if (warningEmitted.compareAndSet(false, true)) { + if (fallbackAllowed) { + reporter.handle( + Event.warn( + String.format( + "%s uses implicit fallback from sandbox to local, which is deprecated" + + " because it is not hermetic. Prefer setting an explicit list of" + + " strategies.", + spawn.getMnemonic()))); + } else { + reporter.handle( + Event.warn( + String.format( + "Implicit fallback from sandbox to local is deprecated. Prefer setting an" + + " explicit list of strategies for %s, or for now pass" + + " --incompatible_legacy_local_fallback.", + spawn.getMnemonic()))); + } + } + } + return canExec && fallbackAllowed; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java index 57d78dcc309ef7..22f2e9f4b4999a 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java @@ -80,12 +80,11 @@ public String getTypeDescription() { } @Option( - name = "ignore_unsupported_sandboxing", - defaultValue = "false", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.UNKNOWN}, - help = "Do not print a warning when sandboxed execution is not supported on this system." - ) + name = "ignore_unsupported_sandboxing", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "Do not print a warning when sandboxed execution is not supported on this system.") public boolean ignoreUnsupportedSandboxing; @Option( @@ -115,21 +114,19 @@ public String getTypeDescription() { public String sandboxBase; @Option( - name = "sandbox_fake_hostname", - defaultValue = "false", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.UNKNOWN}, - help = "Change the current hostname to 'localhost' for sandboxed actions." - ) + name = "sandbox_fake_hostname", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "Change the current hostname to 'localhost' for sandboxed actions.") public boolean sandboxFakeHostname; @Option( - name = "sandbox_fake_username", - defaultValue = "false", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.UNKNOWN}, - help = "Change the current username to 'nobody' for sandboxed actions." - ) + name = "sandbox_fake_username", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "Change the current username to 'nobody' for sandboxed actions.") public boolean sandboxFakeUsername; @Option( @@ -187,14 +184,13 @@ public String getTypeDescription() { public TriState useSandboxfs; @Option( - name = "experimental_sandboxfs_path", - defaultValue = "sandboxfs", - documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, - effectTags = {OptionEffectTag.UNKNOWN}, - help = - "Path to the sandboxfs binary to use when --experimental_use_sandboxfs is true. If a " - + "bare name, use the first binary of that name found in the PATH." - ) + name = "experimental_sandboxfs_path", + defaultValue = "sandboxfs", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "Path to the sandboxfs binary to use when --experimental_use_sandboxfs is true. If a " + + "bare name, use the first binary of that name found in the PATH.") public String sandboxfsPath; @Option( @@ -247,50 +243,48 @@ public ImmutableSet getInaccessiblePaths(FileSystem fs) { } @Option( - name = "experimental_collect_local_sandbox_action_metrics", - defaultValue = "false", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.EXECUTION}, - help = - "When enabled, execution statistics (such as user and system time) are recorded for " - + "locally executed actions which use sandboxing" - ) + name = "experimental_collect_local_sandbox_action_metrics", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.EXECUTION}, + help = + "When enabled, execution statistics (such as user and system time) are recorded for " + + "locally executed actions which use sandboxing") public boolean collectLocalSandboxExecutionStatistics; @Option( - name = "experimental_enable_docker_sandbox", - defaultValue = "false", - documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, - effectTags = {OptionEffectTag.EXECUTION}, - help = "Enable Docker-based sandboxing. This option has no effect if Docker is not installed.") + name = "experimental_enable_docker_sandbox", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, + effectTags = {OptionEffectTag.EXECUTION}, + help = + "Enable Docker-based sandboxing. This option has no effect if Docker is not installed.") public boolean enableDockerSandbox; @Option( - name = "experimental_docker_image", - defaultValue = "", - documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, - effectTags = {OptionEffectTag.EXECUTION}, - help = - "Specify a Docker image name (e.g. \"ubuntu:latest\") that should be used to execute " - + "a sandboxed action when using the docker strategy and the action itself doesn't " - + "already have a container-image attribute in its remote_execution_properties in the " - + "platform description. The value of this flag is passed verbatim to 'docker run', so " - + "it supports the same syntax and mechanisms as Docker itself." - ) + name = "experimental_docker_image", + defaultValue = "", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, + effectTags = {OptionEffectTag.EXECUTION}, + help = + "Specify a Docker image name (e.g. \"ubuntu:latest\") that should be used to execute a" + + " sandboxed action when using the docker strategy and the action itself doesn't" + + " already have a container-image attribute in its remote_execution_properties in" + + " the platform description. The value of this flag is passed verbatim to 'docker" + + " run', so it supports the same syntax and mechanisms as Docker itself.") public String dockerImage; @Option( - name = "experimental_docker_use_customized_images", - defaultValue = "true", - documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, - effectTags = {OptionEffectTag.EXECUTION}, - help = - "If enabled, injects the uid and gid of the current user into the Docker image before " - + "using it. This is required if your build / tests depend on the user having a name " - + "and home directory inside the container. This is on by default, but you can disable " - + "it in case the automatic image customization feature doesn't work in your case or " - + "you know that you don't need it." - ) + name = "experimental_docker_use_customized_images", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, + effectTags = {OptionEffectTag.EXECUTION}, + help = + "If enabled, injects the uid and gid of the current user into the Docker image before" + + " using it. This is required if your build / tests depend on the user having a name" + + " and home directory inside the container. This is on by default, but you can" + + " disable it in case the automatic image customization feature doesn't work in your" + + " case or you know that you don't need it.") public boolean dockerUseCustomizedImages; @Option( @@ -359,8 +353,9 @@ public ImmutableSet getInaccessiblePaths(FileSystem fs) { }, help = "If set to true, enables the legacy implicit fallback from sandboxed to local strategy." - + " This flag will eventually default to false and then become a no-op. You should" - + " use --strategy or --spawn_strategy to configure fallbacks instead.") + + " This flag will eventually default to false and then become a no-op. Use" + + " --strategy, --spawn_strategy, or --dynamic_local_strategy to configure fallbacks" + + " instead.") public boolean legacyLocalFallback; /** Converter for the number of threads used for asynchronous tree deletion. */