From be4cbc7a67196414e3d3f323be8ab55fb5e530f7 Mon Sep 17 00:00:00 2001 From: Lars Clausen Date: Fri, 30 Jul 2021 15:04:33 +0200 Subject: [PATCH] Revert "Move use of legacy sandbox -> local fallback to only be used after all strategies have been tried, and improve messages around it." This reverts commit b2231c56d78c6d37bcb6f11e1e50fe68ee336b4a. --- .../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, 95 insertions(+), 162 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 fe2eef2447c368..2ce9e7894cc8b0 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,23 +50,9 @@ default SpawnContinuation beginExecution( } } - /** - * 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. - */ + /** Returns whether this SpawnActionContext supports executing the given Spawn. */ 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 78ce0e876be25f..ac45e6cd836df2 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 @@ -433,8 +433,7 @@ public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionCo for (SandboxedSpawnStrategy strategy : dynamicStrategyRegistry.getDynamicSpawnActionContexts( spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) { - if (strategy.canExec(spawn, actionContextRegistry) - || strategy.canExecWithLegacyFallback(spawn, actionContextRegistry)) { + if (strategy.canExec(spawn, actionContextRegistry)) { return true; } } @@ -475,8 +474,7 @@ private static ImmutableList runLocally( for (SandboxedSpawnStrategy strategy : dynamicStrategyRegistry.getDynamicSpawnActionContexts( spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) { - if (strategy.canExec(spawn, actionExecutionContext) - || strategy.canExecWithLegacyFallback(spawn, actionExecutionContext)) { + if (strategy.canExec(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 e4fed39e3d31c4..26abf5847e247a 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 @@ -94,12 +94,6 @@ 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 c19be841b224b2..759ee6bf2f3000 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 @@ -255,11 +255,6 @@ 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 a5c9ac9bccd721..da5a9ca6fd98aa 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,37 +88,26 @@ public List resolve( .getContext(SpawnStrategyRegistry.class) .getStrategies(spawn, actionExecutionContext.getEventHandler()); - List execableStrategies = + strategies = strategies.stream() .filter(spawnActionContext -> spawnActionContext.canExec(spawn, actionExecutionContext)) .collect(Collectors.toList()); - 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; + 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()); } - return execableStrategies; + return strategies; } } 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 87542fba7d3b2b..d1fe0425f564ab 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,11 +429,12 @@ private static Path getPathToDockerClient(CommandEnvironment cmdEnv) { private static SpawnRunner withFallback( CommandEnvironment env, AbstractSandboxSpawnRunner sandboxSpawnRunner) { SandboxOptions sandboxOptions = env.getOptions().getOptions(SandboxOptions.class); - return new SandboxFallbackSpawnRunner( - sandboxSpawnRunner, - createFallbackRunner(env), - env.getReporter(), - sandboxOptions != null && sandboxOptions.legacyLocalFallback); + if (sandboxOptions != null && sandboxOptions.legacyLocalFallback) { + return new SandboxFallbackSpawnRunner( + sandboxSpawnRunner, createFallbackRunner(env), env.getReporter()); + } else { + return sandboxSpawnRunner; + } } private static SpawnRunner createFallbackRunner(CommandEnvironment env) { @@ -450,29 +451,19 @@ 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, - boolean fallbackAllowed) { + ExtendedEventHandler reporter) { this.sandboxSpawnRunner = sandboxSpawnRunner; this.fallbackSpawnRunner = fallbackSpawnRunner; this.reporter = reporter; - this.fallbackAllowed = fallbackAllowed; } @Override @@ -488,6 +479,12 @@ 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)); @@ -496,38 +493,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) @Override public boolean canExec(Spawn 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; + return sandboxSpawnRunner.canExec(spawn) || fallbackSpawnRunner.canExec(spawn); } @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 22f2e9f4b4999a..57d78dcc309ef7 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,11 +80,12 @@ 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( @@ -114,19 +115,21 @@ 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( @@ -184,13 +187,14 @@ 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( @@ -243,48 +247,50 @@ 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( @@ -353,9 +359,8 @@ 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. Use" - + " --strategy, --spawn_strategy, or --dynamic_local_strategy to configure fallbacks" - + " instead.") + + " This flag will eventually default to false and then become a no-op. You should" + + " use --strategy or --spawn_strategy to configure fallbacks instead.") public boolean legacyLocalFallback; /** Converter for the number of threads used for asynchronous tree deletion. */