Skip to content

Commit 3b3e642

Browse files
larsrc-googlecopybara-github
larsrc-google
authored and
copybara-github
committed
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
1 parent 50723bb commit 3b3e642

5 files changed

Lines changed: 45 additions & 54 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@
6060
import java.time.Instant;
6161
import java.util.HashSet;
6262
import java.util.Set;
63+
import java.util.concurrent.atomic.AtomicBoolean;
6364
import javax.annotation.Nullable;
6465

6566
/** This module provides the Sandbox spawn strategy. */
@@ -425,9 +426,15 @@ private static Path getPathToDockerClient(CommandEnvironment cmdEnv) {
425426
return null;
426427
}
427428

428-
private static SpawnRunner withFallback(CommandEnvironment env, SpawnRunner sandboxSpawnRunner) {
429-
return new SandboxFallbackSpawnRunner(
430-
sandboxSpawnRunner, createFallbackRunner(env), env.getReporter());
429+
private static SpawnRunner withFallback(
430+
CommandEnvironment env, AbstractSandboxSpawnRunner sandboxSpawnRunner) {
431+
SandboxOptions sandboxOptions = env.getOptions().getOptions(SandboxOptions.class);
432+
if (sandboxOptions != null && sandboxOptions.legacyLocalFallback) {
433+
return new SandboxFallbackSpawnRunner(
434+
sandboxSpawnRunner, createFallbackRunner(env), env.getReporter());
435+
} else {
436+
return sandboxSpawnRunner;
437+
}
431438
}
432439

433440
private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
@@ -447,15 +454,16 @@ private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
447454
private static final class SandboxFallbackSpawnRunner implements SpawnRunner {
448455
private final SpawnRunner sandboxSpawnRunner;
449456
private final SpawnRunner fallbackSpawnRunner;
450-
private final ExtendedEventHandler extendedEventHandler;
457+
private final ExtendedEventHandler reporter;
458+
private static final AtomicBoolean warningEmitted = new AtomicBoolean();
451459

452460
SandboxFallbackSpawnRunner(
453461
SpawnRunner sandboxSpawnRunner,
454462
SpawnRunner fallbackSpawnRunner,
455-
ExtendedEventHandler extendedEventHandler) {
463+
ExtendedEventHandler reporter) {
456464
this.sandboxSpawnRunner = sandboxSpawnRunner;
457465
this.fallbackSpawnRunner = fallbackSpawnRunner;
458-
this.extendedEventHandler = extendedEventHandler;
466+
this.reporter = reporter;
459467
}
460468

461469
@Override
@@ -471,10 +479,15 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
471479
if (sandboxSpawnRunner.canExec(spawn)) {
472480
spawnResult = sandboxSpawnRunner.exec(spawn, context);
473481
} else {
482+
if (warningEmitted.compareAndSet(false, true)) {
483+
reporter.handle(
484+
Event.warn(
485+
"Use of implicit local fallback will go away soon, please"
486+
+ " set a fallback strategy instead. See --legacy_local_fallback option."));
487+
}
474488
spawnResult = fallbackSpawnRunner.exec(spawn, context);
475489
}
476-
extendedEventHandler.post(
477-
new SpawnExecutedEvent(spawn, spawnResult, spawnExecutionStartInstant));
490+
reporter.post(new SpawnExecutedEvent(spawn, spawnResult, spawnExecutionStartInstant));
478491
return spawnResult;
479492
}
480493

@@ -491,7 +504,9 @@ public boolean handlesCaching() {
491504
@Override
492505
public void cleanupSandboxBase(Path sandboxBase, TreeDeleter treeDeleter) throws IOException {
493506
sandboxSpawnRunner.cleanupSandboxBase(sandboxBase, treeDeleter);
494-
fallbackSpawnRunner.cleanupSandboxBase(sandboxBase, treeDeleter);
507+
if (fallbackSpawnRunner != null) {
508+
fallbackSpawnRunner.cleanupSandboxBase(sandboxBase, treeDeleter);
509+
}
495510
}
496511
}
497512

‎src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import com.google.devtools.common.options.Option;
2828
import com.google.devtools.common.options.OptionDocumentationCategory;
2929
import com.google.devtools.common.options.OptionEffectTag;
30+
import com.google.devtools.common.options.OptionMetadataTag;
3031
import com.google.devtools.common.options.OptionsBase;
3132
import com.google.devtools.common.options.OptionsParsingException;
3233
import com.google.devtools.common.options.TriState;
@@ -347,6 +348,21 @@ public ImmutableSet<Path> getInaccessiblePaths(FileSystem fs) {
347348
+ "scheduler. This flag exists purely to support rolling this bug fix out.")
348349
public boolean delayVirtualInputMaterialization;
349350

351+
@Option(
352+
name = "incompatible_legacy_local_fallback",
353+
defaultValue = "true",
354+
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
355+
effectTags = {OptionEffectTag.EXECUTION},
356+
metadataTags = {
357+
OptionMetadataTag.INCOMPATIBLE_CHANGE,
358+
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
359+
},
360+
help =
361+
"If set to true, enables the legacy implicit fallback from sandboxed to local strategy."
362+
+ " This flag will eventually default to false and then become a no-op. You should"
363+
+ " use --strategy or --spawn_strategy to configure fallbacks instead.")
364+
public boolean legacyLocalFallback;
365+
350366
/** Converter for the number of threads used for asynchronous tree deletion. */
351367
public static final class AsyncTreeDeletesConverter extends ResourceConverter {
352368
public AsyncTreeDeletesConverter() {

‎src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -25,15 +25,11 @@
2525
import com.google.devtools.build.lib.events.Event;
2626
import com.google.devtools.build.lib.exec.ExecutionOptions;
2727
import com.google.devtools.build.lib.exec.RunfilesTreeUpdater;
28-
import com.google.devtools.build.lib.exec.SpawnRunner;
2928
import com.google.devtools.build.lib.exec.SpawnStrategyRegistry;
3029
import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
31-
import com.google.devtools.build.lib.exec.local.LocalExecutionOptions;
32-
import com.google.devtools.build.lib.exec.local.LocalSpawnRunner;
3330
import com.google.devtools.build.lib.runtime.BlazeModule;
3431
import com.google.devtools.build.lib.runtime.Command;
3532
import com.google.devtools.build.lib.runtime.CommandEnvironment;
36-
import com.google.devtools.build.lib.runtime.ProcessWrapper;
3733
import com.google.devtools.build.lib.runtime.commands.events.CleanStartingEvent;
3834
import com.google.devtools.build.lib.sandbox.SandboxHelpers;
3935
import com.google.devtools.build.lib.sandbox.SandboxOptions;
@@ -167,7 +163,6 @@ public void registerSpawnStrategies(
167163
workerPool,
168164
options.workerMultiplex,
169165
env.getReporter(),
170-
createFallbackRunner(env, localEnvProvider),
171166
localEnvProvider,
172167
env.getBlazeWorkspace().getBinTools(),
173168
env.getLocalResourceManager(),
@@ -181,21 +176,6 @@ public void registerSpawnStrategies(
181176
"worker");
182177
}
183178

184-
private static SpawnRunner createFallbackRunner(
185-
CommandEnvironment env, LocalEnvProvider localEnvProvider) {
186-
LocalExecutionOptions localExecutionOptions =
187-
env.getOptions().getOptions(LocalExecutionOptions.class);
188-
return new LocalSpawnRunner(
189-
env.getExecRoot(),
190-
localExecutionOptions,
191-
env.getLocalResourceManager(),
192-
localEnvProvider,
193-
env.getBlazeWorkspace().getBinTools(),
194-
ProcessWrapper.fromCommandEnvironment(env),
195-
// TODO(buchgr): Replace singleton by a command-scoped RunfilesTreeUpdater
196-
RunfilesTreeUpdater.INSTANCE);
197-
}
198-
199179
@Subscribe
200180
public void buildComplete(BuildCompleteEvent event) {
201181
if (options != null && options.workerQuitAfterBuild) {

‎src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java‎

Lines changed: 5 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@
3838
import com.google.devtools.build.lib.actions.SpawnResult.Status;
3939
import com.google.devtools.build.lib.actions.Spawns;
4040
import com.google.devtools.build.lib.actions.UserExecException;
41-
import com.google.devtools.build.lib.events.Event;
4241
import com.google.devtools.build.lib.events.ExtendedEventHandler;
4342
import com.google.devtools.build.lib.exec.BinTools;
4443
import com.google.devtools.build.lib.exec.RunfilesTreeUpdater;
@@ -89,7 +88,6 @@ final class WorkerSpawnRunner implements SpawnRunner {
8988
private final WorkerPool workers;
9089
private final boolean multiplex;
9190
private final ExtendedEventHandler reporter;
92-
private final SpawnRunner fallbackRunner;
9391
private final LocalEnvProvider localEnvProvider;
9492
private final BinTools binTools;
9593
private final ResourceManager resourceManager;
@@ -103,7 +101,6 @@ public WorkerSpawnRunner(
103101
WorkerPool workers,
104102
boolean multiplex,
105103
ExtendedEventHandler reporter,
106-
SpawnRunner fallbackRunner,
107104
LocalEnvProvider localEnvProvider,
108105
BinTools binTools,
109106
ResourceManager resourceManager,
@@ -114,7 +111,6 @@ public WorkerSpawnRunner(
114111
this.workers = Preconditions.checkNotNull(workers);
115112
this.multiplex = multiplex;
116113
this.reporter = reporter;
117-
this.fallbackRunner = fallbackRunner;
118114
this.localEnvProvider = localEnvProvider;
119115
this.binTools = binTools;
120116
this.resourceManager = resourceManager;
@@ -127,24 +123,6 @@ public String getName() {
127123
return "worker";
128124
}
129125

130-
@Override
131-
public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
132-
throws ExecException, IOException, InterruptedException {
133-
if (!Spawns.supportsWorkers(spawn) && !Spawns.supportsMultiplexWorkers(spawn)) {
134-
// TODO(ulfjack): Don't circumvent SpawnExecutionPolicy. Either drop the warning here, or
135-
// provide a mechanism in SpawnExecutionPolicy to report warnings.
136-
reporter.handle(
137-
Event.warn(
138-
String.format(ERROR_MESSAGE_PREFIX + REASON_NO_EXECUTION_INFO, spawn.getMnemonic())));
139-
return fallbackRunner.exec(spawn, context);
140-
}
141-
142-
context.report(
143-
ProgressStatus.SCHEDULING,
144-
WorkerKey.makeWorkerTypeName(Spawns.supportsMultiplexWorkers(spawn)));
145-
return actuallyExec(spawn, context);
146-
}
147-
148126
@Override
149127
public boolean canExec(Spawn spawn) {
150128
if (!Spawns.supportsWorkers(spawn) && !Spawns.supportsMultiplexWorkers(spawn)) {
@@ -161,8 +139,12 @@ public boolean handlesCaching() {
161139
return false;
162140
}
163141

164-
private SpawnResult actuallyExec(Spawn spawn, SpawnExecutionContext context)
142+
@Override
143+
public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
165144
throws ExecException, IOException, InterruptedException {
145+
context.report(
146+
ProgressStatus.SCHEDULING,
147+
WorkerKey.makeWorkerTypeName(Spawns.supportsMultiplexWorkers(spawn)));
166148
if (spawn.getToolFiles().isEmpty()) {
167149
throw createUserExecException(
168150
String.format(ERROR_MESSAGE_PREFIX + REASON_NO_TOOLS, spawn.getMnemonic()),

‎src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,6 @@ public void testExecInWorker_happyPath() throws ExecException, InterruptedExcept
101101
createWorkerPool(),
102102
/* multiplex */ false,
103103
reporter,
104-
null,
105104
localEnvProvider,
106105
/* binTools */ null,
107106
resourceManager,
@@ -139,7 +138,6 @@ private void assertRecordedResponsethrowsException(String recordedResponse, Stri
139138
createWorkerPool(),
140139
/* multiplex */ false,
141140
reporter,
142-
null,
143141
localEnvProvider,
144142
/* binTools */ null,
145143
resourceManager,

0 commit comments

Comments
 (0)