From b96bafa6f163719ccf45fb1b9ab7e18a80f97781 Mon Sep 17 00:00:00 2001 From: larsrc Date: Wed, 27 Jan 2021 09:49:59 -0800 Subject: [PATCH] Remove fallback strategy support for workers, add flag for it in sandbox. The fallback strategy is not used by workers, as canExec() already precludes it. For sandbox strategy, it does get used, but as an invisible switch from sandbox to non-sandbox, it's a bad idea, and setting the strategy right can accomplish the same anyway. This CL adds a --incompatible_legacy_local_fallback flag, initially true, that can be flipped once I've cleared out some failing tests internally. RELNOTES: None. PiperOrigin-RevId: 354112050 --- .../build/lib/sandbox/SandboxModule.java | 33 ++++++++++++++----- .../build/lib/sandbox/SandboxOptions.java | 16 +++++++++ .../build/lib/worker/WorkerModule.java | 20 ----------- .../build/lib/worker/WorkerSpawnRunner.java | 28 +++------------- .../lib/worker/WorkerSpawnRunnerTest.java | 2 -- 5 files changed, 45 insertions(+), 54 deletions(-) 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 5562bf03996505..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 @@ -60,6 +60,7 @@ import java.time.Instant; import java.util.HashSet; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; /** This module provides the Sandbox spawn strategy. */ @@ -425,9 +426,15 @@ private static Path getPathToDockerClient(CommandEnvironment cmdEnv) { return null; } - private static SpawnRunner withFallback(CommandEnvironment env, SpawnRunner sandboxSpawnRunner) { - return new SandboxFallbackSpawnRunner( - sandboxSpawnRunner, createFallbackRunner(env), env.getReporter()); + 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; + } } private static SpawnRunner createFallbackRunner(CommandEnvironment env) { @@ -447,15 +454,16 @@ private static SpawnRunner createFallbackRunner(CommandEnvironment env) { private static final class SandboxFallbackSpawnRunner implements SpawnRunner { private final SpawnRunner sandboxSpawnRunner; private final SpawnRunner fallbackSpawnRunner; - private final ExtendedEventHandler extendedEventHandler; + private final ExtendedEventHandler reporter; + private static final AtomicBoolean warningEmitted = new AtomicBoolean(); SandboxFallbackSpawnRunner( SpawnRunner sandboxSpawnRunner, SpawnRunner fallbackSpawnRunner, - ExtendedEventHandler extendedEventHandler) { + ExtendedEventHandler reporter) { this.sandboxSpawnRunner = sandboxSpawnRunner; this.fallbackSpawnRunner = fallbackSpawnRunner; - this.extendedEventHandler = extendedEventHandler; + this.reporter = reporter; } @Override @@ -471,10 +479,15 @@ 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); } - extendedEventHandler.post( - new SpawnExecutedEvent(spawn, spawnResult, spawnExecutionStartInstant)); + reporter.post(new SpawnExecutedEvent(spawn, spawnResult, spawnExecutionStartInstant)); return spawnResult; } @@ -491,7 +504,9 @@ public boolean handlesCaching() { @Override public void cleanupSandboxBase(Path sandboxBase, TreeDeleter treeDeleter) throws IOException { sandboxSpawnRunner.cleanupSandboxBase(sandboxBase, treeDeleter); - fallbackSpawnRunner.cleanupSandboxBase(sandboxBase, treeDeleter); + if (fallbackSpawnRunner != null) { + fallbackSpawnRunner.cleanupSandboxBase(sandboxBase, treeDeleter); + } } } 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 47fc52637919b8..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 @@ -27,6 +27,7 @@ import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; +import com.google.devtools.common.options.OptionMetadataTag; import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsParsingException; import com.google.devtools.common.options.TriState; @@ -347,6 +348,21 @@ public ImmutableSet getInaccessiblePaths(FileSystem fs) { + "scheduler. This flag exists purely to support rolling this bug fix out.") public boolean delayVirtualInputMaterialization; + @Option( + name = "incompatible_legacy_local_fallback", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, + effectTags = {OptionEffectTag.EXECUTION}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + 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.") + public boolean legacyLocalFallback; + /** Converter for the number of threads used for asynchronous tree deletion. */ public static final class AsyncTreeDeletesConverter extends ResourceConverter { public AsyncTreeDeletesConverter() { diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java index 88d0962b49589b..43a3c058b6fdce 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java @@ -25,15 +25,11 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.RunfilesTreeUpdater; -import com.google.devtools.build.lib.exec.SpawnRunner; import com.google.devtools.build.lib.exec.SpawnStrategyRegistry; 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.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.Command; import com.google.devtools.build.lib.runtime.CommandEnvironment; -import com.google.devtools.build.lib.runtime.ProcessWrapper; import com.google.devtools.build.lib.runtime.commands.events.CleanStartingEvent; import com.google.devtools.build.lib.sandbox.SandboxHelpers; import com.google.devtools.build.lib.sandbox.SandboxOptions; @@ -167,7 +163,6 @@ public void registerSpawnStrategies( workerPool, options.workerMultiplex, env.getReporter(), - createFallbackRunner(env, localEnvProvider), localEnvProvider, env.getBlazeWorkspace().getBinTools(), env.getLocalResourceManager(), @@ -181,21 +176,6 @@ public void registerSpawnStrategies( "worker"); } - private static SpawnRunner createFallbackRunner( - CommandEnvironment env, LocalEnvProvider localEnvProvider) { - LocalExecutionOptions localExecutionOptions = - env.getOptions().getOptions(LocalExecutionOptions.class); - return new LocalSpawnRunner( - env.getExecRoot(), - localExecutionOptions, - env.getLocalResourceManager(), - localEnvProvider, - env.getBlazeWorkspace().getBinTools(), - ProcessWrapper.fromCommandEnvironment(env), - // TODO(buchgr): Replace singleton by a command-scoped RunfilesTreeUpdater - RunfilesTreeUpdater.INSTANCE); - } - @Subscribe public void buildComplete(BuildCompleteEvent event) { if (options != null && options.workerQuitAfterBuild) { diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java index 26f6019fa53136..354f7ca5e818a0 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java @@ -38,7 +38,6 @@ import com.google.devtools.build.lib.actions.SpawnResult.Status; import com.google.devtools.build.lib.actions.Spawns; import com.google.devtools.build.lib.actions.UserExecException; -import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.exec.BinTools; import com.google.devtools.build.lib.exec.RunfilesTreeUpdater; @@ -89,7 +88,6 @@ final class WorkerSpawnRunner implements SpawnRunner { private final WorkerPool workers; private final boolean multiplex; private final ExtendedEventHandler reporter; - private final SpawnRunner fallbackRunner; private final LocalEnvProvider localEnvProvider; private final BinTools binTools; private final ResourceManager resourceManager; @@ -103,7 +101,6 @@ public WorkerSpawnRunner( WorkerPool workers, boolean multiplex, ExtendedEventHandler reporter, - SpawnRunner fallbackRunner, LocalEnvProvider localEnvProvider, BinTools binTools, ResourceManager resourceManager, @@ -114,7 +111,6 @@ public WorkerSpawnRunner( this.workers = Preconditions.checkNotNull(workers); this.multiplex = multiplex; this.reporter = reporter; - this.fallbackRunner = fallbackRunner; this.localEnvProvider = localEnvProvider; this.binTools = binTools; this.resourceManager = resourceManager; @@ -127,24 +123,6 @@ public String getName() { return "worker"; } - @Override - public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) - throws ExecException, IOException, InterruptedException { - if (!Spawns.supportsWorkers(spawn) && !Spawns.supportsMultiplexWorkers(spawn)) { - // TODO(ulfjack): Don't circumvent SpawnExecutionPolicy. Either drop the warning here, or - // provide a mechanism in SpawnExecutionPolicy to report warnings. - reporter.handle( - Event.warn( - String.format(ERROR_MESSAGE_PREFIX + REASON_NO_EXECUTION_INFO, spawn.getMnemonic()))); - return fallbackRunner.exec(spawn, context); - } - - context.report( - ProgressStatus.SCHEDULING, - WorkerKey.makeWorkerTypeName(Spawns.supportsMultiplexWorkers(spawn))); - return actuallyExec(spawn, context); - } - @Override public boolean canExec(Spawn spawn) { if (!Spawns.supportsWorkers(spawn) && !Spawns.supportsMultiplexWorkers(spawn)) { @@ -161,8 +139,12 @@ public boolean handlesCaching() { return false; } - private SpawnResult actuallyExec(Spawn spawn, SpawnExecutionContext context) + @Override + public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) throws ExecException, IOException, InterruptedException { + context.report( + ProgressStatus.SCHEDULING, + WorkerKey.makeWorkerTypeName(Spawns.supportsMultiplexWorkers(spawn))); if (spawn.getToolFiles().isEmpty()) { throw createUserExecException( String.format(ERROR_MESSAGE_PREFIX + REASON_NO_TOOLS, spawn.getMnemonic()), diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java index 0b035a923ad465..d81f3a65ecab6e 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java @@ -101,7 +101,6 @@ public void testExecInWorker_happyPath() throws ExecException, InterruptedExcept createWorkerPool(), /* multiplex */ false, reporter, - null, localEnvProvider, /* binTools */ null, resourceManager, @@ -139,7 +138,6 @@ private void assertRecordedResponsethrowsException(String recordedResponse, Stri createWorkerPool(), /* multiplex */ false, reporter, - null, localEnvProvider, /* binTools */ null, resourceManager,