Skip to content

Commit

Permalink
Remove fallback strategy support for workers, add flag for it in sand…
Browse files Browse the repository at this point in the history 
…box.

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
  • Loading branch information
@larsrc-google
larsrc-google authored and Copybara-Service committed Jan 27, 2021
1 parent 50723bb commit 3b3e642
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 54 deletions.
33 changes: 24 additions & 9 deletions src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
View file
Expand Up @@ -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. */
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand All @@ -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;
}

Expand All @@ -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);
}
}
}

Expand Down
16 changes: 16 additions & 0 deletions src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -347,6 +348,21 @@ public ImmutableSet<Path> 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() {
Expand Down
20 changes: 0 additions & 20 deletions src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -167,7 +163,6 @@ public void registerSpawnStrategies(
workerPool,
options.workerMultiplex,
env.getReporter(),
createFallbackRunner(env, localEnvProvider),
localEnvProvider,
env.getBlazeWorkspace().getBinTools(),
env.getLocalResourceManager(),
Expand All @@ -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) {
Expand Down
28 changes: 5 additions & 23 deletions src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -103,7 +101,6 @@ public WorkerSpawnRunner(
WorkerPool workers,
boolean multiplex,
ExtendedEventHandler reporter,
SpawnRunner fallbackRunner,
LocalEnvProvider localEnvProvider,
BinTools binTools,
ResourceManager resourceManager,
Expand All @@ -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;
Expand All @@ -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)) {
Expand All @@ -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()),
Expand Down
2 changes: 0 additions & 2 deletions src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java
View file
Expand Up @@ -101,7 +101,6 @@ public void testExecInWorker_happyPath() throws ExecException, InterruptedExcept
createWorkerPool(),
/* multiplex */ false,
reporter,
null,
localEnvProvider,
/* binTools */ null,
resourceManager,
Expand Down Expand Up @@ -139,7 +138,6 @@ private void assertRecordedResponsethrowsException(String recordedResponse, Stri
createWorkerPool(),
/* multiplex */ false,
reporter,
null,
localEnvProvider,
/* binTools */ null,
resourceManager,
Expand Down

0 comments on commit 3b3e642

Please sign in to comment.