Skip to content

Commit

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

This reverts commit b2231c5.
  • Loading branch information
larsrc-google committed Jul 30, 2021
1 parent e6809c9 commit be4cbc7
Show file tree
Hide file tree
Showing 7 changed files with 95 additions and 162 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -50,23 +50,9 @@ default SpawnContinuation beginExecution(
}
}

/**
* 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.
*/
/** Returns whether this SpawnActionContext supports executing the given Spawn. */
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 @@ -433,8 +433,7 @@ public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionCo
for (SandboxedSpawnStrategy strategy :
dynamicStrategyRegistry.getDynamicSpawnActionContexts(
spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) {
if (strategy.canExec(spawn, actionContextRegistry)
|| strategy.canExecWithLegacyFallback(spawn, actionContextRegistry)) {
if (strategy.canExec(spawn, actionContextRegistry)) {
return true;
}
}
Expand Down Expand Up @@ -475,8 +474,7 @@ private static ImmutableList<SpawnResult> runLocally(
for (SandboxedSpawnStrategy strategy :
dynamicStrategyRegistry.getDynamicSpawnActionContexts(
spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) {
if (strategy.canExec(spawn, actionExecutionContext)
|| strategy.canExecWithLegacyFallback(spawn, actionExecutionContext)) {
if (strategy.canExec(spawn, actionExecutionContext)) {
return strategy.exec(spawn, actionExecutionContext, stopConcurrentSpawns);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,6 @@ 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 @@ -255,11 +255,6 @@ 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,37 +88,26 @@ public List<? extends SpawnStrategy> resolve(
.getContext(SpawnStrategyRegistry.class)
.getStrategies(spawn, actionExecutionContext.getEventHandler());

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

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;
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());
}

return execableStrategies;
return strategies;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -429,11 +429,12 @@ private static Path getPathToDockerClient(CommandEnvironment cmdEnv) {
private static SpawnRunner withFallback(
CommandEnvironment env, AbstractSandboxSpawnRunner sandboxSpawnRunner) {
SandboxOptions sandboxOptions = env.getOptions().getOptions(SandboxOptions.class);
return new SandboxFallbackSpawnRunner(
sandboxSpawnRunner,
createFallbackRunner(env),
env.getReporter(),
sandboxOptions != null && sandboxOptions.legacyLocalFallback);
if (sandboxOptions != null && sandboxOptions.legacyLocalFallback) {
return new SandboxFallbackSpawnRunner(
sandboxSpawnRunner, createFallbackRunner(env), env.getReporter());
} else {
return sandboxSpawnRunner;
}
}

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

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

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

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,12 @@ 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 @@ -114,19 +115,21 @@ 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 @@ -184,13 +187,14 @@ 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 @@ -243,48 +247,50 @@ 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 @@ -353,9 +359,8 @@ 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. Use"
+ " --strategy, --spawn_strategy, or --dynamic_local_strategy to configure fallbacks"
+ " instead.")
+ " 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. */
Expand Down

0 comments on commit be4cbc7

Please sign in to comment.