Skip to content

Commit b2231c5

Browse files
larsrc-googlecopybara-github
larsrc-google
authored and
copybara-github
committed
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
1 parent 6b1e9a6 commit b2231c5

File tree

7 files changed

+162
-95
lines changed

7 files changed

+162
-95
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/SpawnStrategy.java‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,23 @@ default SpawnContinuation beginExecution(
5050
}
5151
}
5252

53-
/** Returns whether this SpawnActionContext supports executing the given Spawn. */
53+
/**
54+
* Returns whether this SpawnActionContext supports executing the given Spawn. This does not allow
55+
* using the legacy fallback to local execution controlled by the {@code
56+
* --incompatible_legacy_local_fallback} flag.
57+
*/
5458
boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry);
5559

60+
/**
61+
* Returns true if this SpawnActionContext supports executing the given Spawn through a legacy
62+
* fallback system. This will only be used if no SpawnActionContexts were able to execute it by
63+
* normal means.
64+
*/
65+
default boolean canExecWithLegacyFallback(
66+
Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry) {
67+
return false;
68+
}
69+
5670
/**
5771
* Performs any actions conditional on this strategy not only being registered but triggered as
5872
* used because its identifier was requested and it was not overridden.

‎src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -445,7 +445,8 @@ public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionCo
445445
for (SandboxedSpawnStrategy strategy :
446446
dynamicStrategyRegistry.getDynamicSpawnActionContexts(
447447
spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) {
448-
if (strategy.canExec(spawn, actionContextRegistry)) {
448+
if (strategy.canExec(spawn, actionContextRegistry)
449+
|| strategy.canExecWithLegacyFallback(spawn, actionContextRegistry)) {
449450
return true;
450451
}
451452
}
@@ -513,7 +514,8 @@ private static ImmutableList<SpawnResult> runSpawnLocally(
513514
for (SandboxedSpawnStrategy strategy :
514515
dynamicStrategyRegistry.getDynamicSpawnActionContexts(
515516
spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) {
516-
if (strategy.canExec(spawn, actionExecutionContext)) {
517+
if (strategy.canExec(spawn, actionExecutionContext)
518+
|| strategy.canExecWithLegacyFallback(spawn, actionExecutionContext)) {
517519
return strategy.exec(spawn, actionExecutionContext, stopConcurrentSpawns);
518520
}
519521
}

‎src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,12 @@ public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionCo
9696
return spawnRunner.canExec(spawn);
9797
}
9898

99+
@Override
100+
public boolean canExecWithLegacyFallback(
101+
Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry) {
102+
return spawnRunner.canExecWithLegacyFallback(spawn);
103+
}
104+
99105
@Override
100106
public ImmutableList<SpawnResult> exec(Spawn spawn, ActionExecutionContext actionExecutionContext)
101107
throws ExecException, InterruptedException {

‎src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,11 @@ SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
269269
/** Returns whether this SpawnRunner supports executing the given Spawn. */
270270
boolean canExec(Spawn spawn);
271271

272+
/** Returns whether this SpawnRunner supports executing the given Spawn using legacy fallbacks. */
273+
default boolean canExecWithLegacyFallback(Spawn spawn) {
274+
return false;
275+
}
276+
272277
/** Returns whether this SpawnRunner handles caching of actions internally. */
273278
boolean handlesCaching();
274279

‎src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -88,26 +88,37 @@ public List<? extends SpawnStrategy> resolve(
8888
.getContext(SpawnStrategyRegistry.class)
8989
.getStrategies(spawn, actionExecutionContext.getEventHandler());
9090

91-
strategies =
91+
List<? extends SpawnStrategy> execableStrategies =
9292
strategies.stream()
9393
.filter(spawnActionContext -> spawnActionContext.canExec(spawn, actionExecutionContext))
9494
.collect(Collectors.toList());
9595

96-
if (strategies.isEmpty()) {
97-
String message =
98-
String.format(
99-
"No usable spawn strategy found for spawn with mnemonic %s. Your"
100-
+ " --spawn_strategy, --genrule_strategy and/or --strategy flags are probably too"
101-
+ " strict. Visit https://github.com/bazelbuild/bazel/issues/7480 for"
102-
+ " migration advice",
103-
spawn.getMnemonic());
104-
throw new UserExecException(
105-
FailureDetail.newBuilder()
106-
.setMessage(message)
107-
.setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.NO_USABLE_STRATEGY_FOUND))
108-
.build());
96+
if (execableStrategies.isEmpty()) {
97+
// Legacy implicit fallbacks should be a last-ditch option after all other strategies are
98+
// found non-executable.
99+
List<? extends SpawnStrategy> fallbackStrategies =
100+
strategies.stream()
101+
.filter(
102+
spawnActionContext ->
103+
spawnActionContext.canExecWithLegacyFallback(spawn, actionExecutionContext))
104+
.collect(Collectors.toList());
105+
106+
if (fallbackStrategies.isEmpty()) {
107+
String message =
108+
String.format(
109+
"No usable spawn strategy found for spawn with mnemonic %s. Your --spawn_strategy,"
110+
+ " --genrule_strategy and/or --strategy flags are probably too strict. Visit"
111+
+ " https://github.com/bazelbuild/bazel/issues/7480 for migration advice",
112+
spawn.getMnemonic());
113+
throw new UserExecException(
114+
FailureDetail.newBuilder()
115+
.setMessage(message)
116+
.setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.NO_USABLE_STRATEGY_FOUND))
117+
.build());
118+
}
119+
return fallbackStrategies;
109120
}
110121

111-
return strategies;
122+
return execableStrategies;
112123
}
113124
}

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

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -429,12 +429,11 @@ private static Path getPathToDockerClient(CommandEnvironment cmdEnv) {
429429
private static SpawnRunner withFallback(
430430
CommandEnvironment env, AbstractSandboxSpawnRunner sandboxSpawnRunner) {
431431
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-
}
432+
return new SandboxFallbackSpawnRunner(
433+
sandboxSpawnRunner,
434+
createFallbackRunner(env),
435+
env.getReporter(),
436+
sandboxOptions != null && sandboxOptions.legacyLocalFallback);
438437
}
439438

440439
private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
@@ -451,19 +450,29 @@ private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
451450
RunfilesTreeUpdater.INSTANCE);
452451
}
453452

453+
/**
454+
* A SpawnRunner that does sandboxing if possible, but might fall back to local execution if
455+
* ----incompatible_legacy_local_fallback is true and no other strategy has been usable. This is a
456+
* legacy functionality from before the strategies system was added, and can deceive the user into
457+
* thinking a build is hermetic when it isn't really. TODO(b/178356138): Flip flag to default to
458+
* false and then later remove this code entirely.
459+
*/
454460
private static final class SandboxFallbackSpawnRunner implements SpawnRunner {
455461
private final SpawnRunner sandboxSpawnRunner;
456462
private final SpawnRunner fallbackSpawnRunner;
457463
private final ExtendedEventHandler reporter;
458464
private static final AtomicBoolean warningEmitted = new AtomicBoolean();
465+
private final boolean fallbackAllowed;
459466

460467
SandboxFallbackSpawnRunner(
461468
SpawnRunner sandboxSpawnRunner,
462469
SpawnRunner fallbackSpawnRunner,
463-
ExtendedEventHandler reporter) {
470+
ExtendedEventHandler reporter,
471+
boolean fallbackAllowed) {
464472
this.sandboxSpawnRunner = sandboxSpawnRunner;
465473
this.fallbackSpawnRunner = fallbackSpawnRunner;
466474
this.reporter = reporter;
475+
this.fallbackAllowed = fallbackAllowed;
467476
}
468477

469478
@Override
@@ -479,12 +488,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
479488
if (sandboxSpawnRunner.canExec(spawn)) {
480489
spawnResult = sandboxSpawnRunner.exec(spawn, context);
481490
} 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-
}
488491
spawnResult = fallbackSpawnRunner.exec(spawn, context);
489492
}
490493
reporter.post(new SpawnExecutedEvent(spawn, spawnResult, spawnExecutionStartInstant));
@@ -493,7 +496,38 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
493496

494497
@Override
495498
public boolean canExec(Spawn spawn) {
496-
return sandboxSpawnRunner.canExec(spawn) || fallbackSpawnRunner.canExec(spawn);
499+
return sandboxSpawnRunner.canExec(spawn);
500+
}
501+
502+
@Override
503+
public boolean canExecWithLegacyFallback(Spawn spawn) {
504+
boolean canExec = !sandboxSpawnRunner.canExec(spawn) && fallbackSpawnRunner.canExec(spawn);
505+
if (canExec) {
506+
// We give a single warning to use strategies instead, whether or not we allow the fallback
507+
// to happen. This allows people to switch early, but also explains why the build fails
508+
// once we flip the flag. Unfortunately, we can't easily tell if the flag was explicitly
509+
// set, if we could we should omit the warnings in that case.
510+
if (warningEmitted.compareAndSet(false, true)) {
511+
if (fallbackAllowed) {
512+
reporter.handle(
513+
Event.warn(
514+
String.format(
515+
"%s uses implicit fallback from sandbox to local, which is deprecated"
516+
+ " because it is not hermetic. Prefer setting an explicit list of"
517+
+ " strategies.",
518+
spawn.getMnemonic())));
519+
} else {
520+
reporter.handle(
521+
Event.warn(
522+
String.format(
523+
"Implicit fallback from sandbox to local is deprecated. Prefer setting an"
524+
+ " explicit list of strategies for %s, or for now pass"
525+
+ " --incompatible_legacy_local_fallback.",
526+
spawn.getMnemonic())));
527+
}
528+
}
529+
}
530+
return canExec && fallbackAllowed;
497531
}
498532

499533
@Override

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

Lines changed: 58 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -80,12 +80,11 @@ public String getTypeDescription() {
8080
}
8181

8282
@Option(
83-
name = "ignore_unsupported_sandboxing",
84-
defaultValue = "false",
85-
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
86-
effectTags = {OptionEffectTag.UNKNOWN},
87-
help = "Do not print a warning when sandboxed execution is not supported on this system."
88-
)
83+
name = "ignore_unsupported_sandboxing",
84+
defaultValue = "false",
85+
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
86+
effectTags = {OptionEffectTag.UNKNOWN},
87+
help = "Do not print a warning when sandboxed execution is not supported on this system.")
8988
public boolean ignoreUnsupportedSandboxing;
9089

9190
@Option(
@@ -115,21 +114,19 @@ public String getTypeDescription() {
115114
public String sandboxBase;
116115

117116
@Option(
118-
name = "sandbox_fake_hostname",
119-
defaultValue = "false",
120-
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
121-
effectTags = {OptionEffectTag.UNKNOWN},
122-
help = "Change the current hostname to 'localhost' for sandboxed actions."
123-
)
117+
name = "sandbox_fake_hostname",
118+
defaultValue = "false",
119+
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
120+
effectTags = {OptionEffectTag.UNKNOWN},
121+
help = "Change the current hostname to 'localhost' for sandboxed actions.")
124122
public boolean sandboxFakeHostname;
125123

126124
@Option(
127-
name = "sandbox_fake_username",
128-
defaultValue = "false",
129-
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
130-
effectTags = {OptionEffectTag.UNKNOWN},
131-
help = "Change the current username to 'nobody' for sandboxed actions."
132-
)
125+
name = "sandbox_fake_username",
126+
defaultValue = "false",
127+
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
128+
effectTags = {OptionEffectTag.UNKNOWN},
129+
help = "Change the current username to 'nobody' for sandboxed actions.")
133130
public boolean sandboxFakeUsername;
134131

135132
@Option(
@@ -187,14 +184,13 @@ public String getTypeDescription() {
187184
public TriState useSandboxfs;
188185

189186
@Option(
190-
name = "experimental_sandboxfs_path",
191-
defaultValue = "sandboxfs",
192-
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
193-
effectTags = {OptionEffectTag.UNKNOWN},
194-
help =
195-
"Path to the sandboxfs binary to use when --experimental_use_sandboxfs is true. If a "
196-
+ "bare name, use the first binary of that name found in the PATH."
197-
)
187+
name = "experimental_sandboxfs_path",
188+
defaultValue = "sandboxfs",
189+
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
190+
effectTags = {OptionEffectTag.UNKNOWN},
191+
help =
192+
"Path to the sandboxfs binary to use when --experimental_use_sandboxfs is true. If a "
193+
+ "bare name, use the first binary of that name found in the PATH.")
198194
public String sandboxfsPath;
199195

200196
@Option(
@@ -247,50 +243,48 @@ public ImmutableSet<Path> getInaccessiblePaths(FileSystem fs) {
247243
}
248244

249245
@Option(
250-
name = "experimental_collect_local_sandbox_action_metrics",
251-
defaultValue = "false",
252-
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
253-
effectTags = {OptionEffectTag.EXECUTION},
254-
help =
255-
"When enabled, execution statistics (such as user and system time) are recorded for "
256-
+ "locally executed actions which use sandboxing"
257-
)
246+
name = "experimental_collect_local_sandbox_action_metrics",
247+
defaultValue = "false",
248+
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
249+
effectTags = {OptionEffectTag.EXECUTION},
250+
help =
251+
"When enabled, execution statistics (such as user and system time) are recorded for "
252+
+ "locally executed actions which use sandboxing")
258253
public boolean collectLocalSandboxExecutionStatistics;
259254

260255
@Option(
261-
name = "experimental_enable_docker_sandbox",
262-
defaultValue = "false",
263-
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
264-
effectTags = {OptionEffectTag.EXECUTION},
265-
help = "Enable Docker-based sandboxing. This option has no effect if Docker is not installed.")
256+
name = "experimental_enable_docker_sandbox",
257+
defaultValue = "false",
258+
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
259+
effectTags = {OptionEffectTag.EXECUTION},
260+
help =
261+
"Enable Docker-based sandboxing. This option has no effect if Docker is not installed.")
266262
public boolean enableDockerSandbox;
267263

268264
@Option(
269-
name = "experimental_docker_image",
270-
defaultValue = "",
271-
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
272-
effectTags = {OptionEffectTag.EXECUTION},
273-
help =
274-
"Specify a Docker image name (e.g. \"ubuntu:latest\") that should be used to execute "
275-
+ "a sandboxed action when using the docker strategy and the action itself doesn't "
276-
+ "already have a container-image attribute in its remote_execution_properties in the "
277-
+ "platform description. The value of this flag is passed verbatim to 'docker run', so "
278-
+ "it supports the same syntax and mechanisms as Docker itself."
279-
)
265+
name = "experimental_docker_image",
266+
defaultValue = "",
267+
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
268+
effectTags = {OptionEffectTag.EXECUTION},
269+
help =
270+
"Specify a Docker image name (e.g. \"ubuntu:latest\") that should be used to execute a"
271+
+ " sandboxed action when using the docker strategy and the action itself doesn't"
272+
+ " already have a container-image attribute in its remote_execution_properties in"
273+
+ " the platform description. The value of this flag is passed verbatim to 'docker"
274+
+ " run', so it supports the same syntax and mechanisms as Docker itself.")
280275
public String dockerImage;
281276

282277
@Option(
283-
name = "experimental_docker_use_customized_images",
284-
defaultValue = "true",
285-
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
286-
effectTags = {OptionEffectTag.EXECUTION},
287-
help =
288-
"If enabled, injects the uid and gid of the current user into the Docker image before "
289-
+ "using it. This is required if your build / tests depend on the user having a name "
290-
+ "and home directory inside the container. This is on by default, but you can disable "
291-
+ "it in case the automatic image customization feature doesn't work in your case or "
292-
+ "you know that you don't need it."
293-
)
278+
name = "experimental_docker_use_customized_images",
279+
defaultValue = "true",
280+
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
281+
effectTags = {OptionEffectTag.EXECUTION},
282+
help =
283+
"If enabled, injects the uid and gid of the current user into the Docker image before"
284+
+ " using it. This is required if your build / tests depend on the user having a name"
285+
+ " and home directory inside the container. This is on by default, but you can"
286+
+ " disable it in case the automatic image customization feature doesn't work in your"
287+
+ " case or you know that you don't need it.")
294288
public boolean dockerUseCustomizedImages;
295289

296290
@Option(
@@ -359,8 +353,9 @@ public ImmutableSet<Path> getInaccessiblePaths(FileSystem fs) {
359353
},
360354
help =
361355
"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.")
356+
+ " This flag will eventually default to false and then become a no-op. Use"
357+
+ " --strategy, --spawn_strategy, or --dynamic_local_strategy to configure fallbacks"
358+
+ " instead.")
364359
public boolean legacyLocalFallback;
365360

366361
/** Converter for the number of threads used for asynchronous tree deletion. */

0 commit comments

Comments
 (0)