Skip to content

Commit

Permalink
Move use of legacy sandbox -> local fallback to only be used after al…
Browse files Browse the repository at this point in the history
…l strategies have been tried, and improve messages around it.

RELNOTES: None.
PiperOrigin-RevId: 371873129
  • Loading branch information
larsrc-google authored and copybara-github committed May 4, 2021
1 parent 6b1e9a6 commit b2231c5
Show file tree
Hide file tree
Showing 7 changed files with 162 additions and 95 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,23 @@ default SpawnContinuation beginExecution(
}
}

/** Returns whether this SpawnActionContext supports executing the given Spawn. */
/**
* 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.
*/
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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,8 @@ public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionCo
for (SandboxedSpawnStrategy strategy :
dynamicStrategyRegistry.getDynamicSpawnActionContexts(
spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) {
if (strategy.canExec(spawn, actionContextRegistry)) {
if (strategy.canExec(spawn, actionContextRegistry)
|| strategy.canExecWithLegacyFallback(spawn, actionContextRegistry)) {
return true;
}
}
Expand Down Expand Up @@ -513,7 +514,8 @@ private static ImmutableList<SpawnResult> runSpawnLocally(
for (SandboxedSpawnStrategy strategy :
dynamicStrategyRegistry.getDynamicSpawnActionContexts(
spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) {
if (strategy.canExec(spawn, actionExecutionContext)) {
if (strategy.canExec(spawn, actionExecutionContext)
|| strategy.canExecWithLegacyFallback(spawn, actionExecutionContext)) {
return strategy.exec(spawn, actionExecutionContext, stopConcurrentSpawns);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,12 @@ 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<SpawnResult> exec(Spawn spawn, ActionExecutionContext actionExecutionContext)
throws ExecException, InterruptedException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,11 @@ 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();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,26 +88,37 @@ public List<? extends SpawnStrategy> resolve(
.getContext(SpawnStrategyRegistry.class)
.getStrategies(spawn, actionExecutionContext.getEventHandler());

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

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());
if (execableStrategies.isEmpty()) {
// Legacy implicit fallbacks should be a last-ditch option after all other strategies are
// found non-executable.
List<? extends SpawnStrategy> 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;
}

return strategies;
return execableStrategies;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -429,12 +429,11 @@ private static Path getPathToDockerClient(CommandEnvironment cmdEnv) {
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;
}
return new SandboxFallbackSpawnRunner(
sandboxSpawnRunner,
createFallbackRunner(env),
env.getReporter(),
sandboxOptions != null && sandboxOptions.legacyLocalFallback);
}

private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
Expand All @@ -451,19 +450,29 @@ 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) {
ExtendedEventHandler reporter,
boolean fallbackAllowed) {
this.sandboxSpawnRunner = sandboxSpawnRunner;
this.fallbackSpawnRunner = fallbackSpawnRunner;
this.reporter = reporter;
this.fallbackAllowed = fallbackAllowed;
}

@Override
Expand All @@ -479,12 +488,6 @@ 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));
Expand All @@ -493,7 +496,38 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)

@Override
public boolean canExec(Spawn spawn) {
return sandboxSpawnRunner.canExec(spawn) || fallbackSpawnRunner.canExec(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;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,11 @@ 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(
Expand Down Expand Up @@ -115,21 +114,19 @@ 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(
Expand Down Expand Up @@ -187,14 +184,13 @@ 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(
Expand Down Expand Up @@ -247,50 +243,48 @@ public ImmutableSet<Path> 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(
Expand Down Expand Up @@ -359,8 +353,9 @@ public ImmutableSet<Path> 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. You should"
+ " use --strategy or --spawn_strategy to configure fallbacks instead.")
+ " 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.")
public boolean legacyLocalFallback;

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

0 comments on commit b2231c5

Please sign in to comment.