Skip to content

Filter spawn strategies by execution platform #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,11 @@ public static Platform getPlatformProto(
? remoteOptions.getRemoteDefaultExecProperties()
: ImmutableSortedMap.of();

if (spawn.getExecutionPlatform() == null
if ((spawn.getExecutionPlatform() == null
|| (
spawn.getExecutionPlatform().execProperties().isEmpty()
&& spawn.getExecutionPlatform().remoteExecutionProperties().isEmpty()
))
&& spawn.getCombinedExecProperties().isEmpty()
&& defaultExecProperties.isEmpty()
&& additionalProperties.isEmpty()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.devtools.build.lib.analysis.actions.FileWriteActionContext;
import com.google.devtools.build.lib.analysis.actions.TemplateExpansionContext;
import com.google.devtools.build.lib.buildtool.BuildRequest;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.exec.ExecutionOptions;
import com.google.devtools.build.lib.exec.ModuleActionContextRegistry;
import com.google.devtools.build.lib.exec.SpawnCache;
Expand Down Expand Up @@ -90,5 +91,9 @@ public void registerSpawnStrategies(
for (Map.Entry<RegexFilter, List<String>> entry : options.strategyByRegexp) {
registryBuilder.addDescriptionFilter(entry.getKey(), entry.getValue());
}

for (Map.Entry<String, List<String>> strategy : options.allowedStrategiesByExecPlatform) {
registryBuilder.addExecPlatformFilter(Label.parseCanonicalUnchecked(strategy.getKey()), strategy.getValue());
}
Comment on lines +95 to +97
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Implementation looks good but lacks error handling for malformed labels.

The new loop properly iterates over the platform-specific strategy mappings and adds them to the registry. This implements the core functionality needed for the platform-scoped spawn strategies.

However, parseCanonicalUnchecked could throw a runtime exception if a malformed label is provided. Consider whether adding error handling for malformed labels would improve user experience:


🏁 Script executed:

#!/bin/bash
# Check for any error handling around Label parsing elsewhere in the codebase
rg "parseCanonicalUnchecked.*try|try.*parseCanonicalUnchecked" --type java

Length of output: 2053


Add error handling for malformed platform labels

While the loop correctly registers each <exec_platform, strategies> entry, a malformed platform label will surface as an uncaught IllegalArgumentException at runtime. To improve usability, wrap the Label.parseCanonicalUnchecked call in a try/catch and emit a clear, user-facing error (for example, an OptionsParsingException) indicating which key was invalid:

• File: src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java
Lines ~95–97

Suggested adjustment:

for (Map.Entry<String, List<String>> strategy
     : options.allowedStrategiesByExecPlatform) {
  try {
    Label platform = Label.parseCanonicalUnchecked(strategy.getKey());
    registryBuilder.addExecPlatformFilter(platform, strategy.getValue());
  } catch (IllegalArgumentException e) {
    throw new OptionsParsingException(
        String.format(
            "Invalid exec‐platform label '%s' in --allowed_strategies_by_exec_platform",
            strategy.getKey()),
        e);
  }
}

This ensures malformed labels are caught early and reported with a descriptive message.

}
}
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/exec/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/util:resource_converter",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/common/options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
"//third_party:guava",
],
)
Expand Down Expand Up @@ -364,6 +365,7 @@ java_library(
":remote_local_fallback_registry",
":spawn_strategy_policy",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,23 @@ public class ExecutionOptions extends OptionsBase {
+ "the 'local' strategy, but reversing the order would run it with 'sandboxed'. ")
public List<Map.Entry<RegexFilter, List<String>>> strategyByRegexp;

@Option(
name = "allowed_strategies_by_exec_platform",
allowMultiple = true,
converter = Converters.StringToStringListConverter.class,
defaultValue = "null",
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
effectTags = {OptionEffectTag.EXECUTION},
help =
"""
Filters spawn strategies by the execution platform.
Example: `--allowed_strategies_by_exec_platform=@platforms//host:host=local,sandboxed,worker`
to prevent actions configured for the host platform from being spawned remotely.
Example: `--allowed_strategies_by_exec_platform=//:linux_amd64=remote` to prevent actions
configured for a platform `//:linux_amd64` from being spawned locally.
""")
public List<Map.Entry<String, List<String>>> allowedStrategiesByExecPlatform;

@Option(
name = "materialize_param_files",
defaultValue = "false",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.devtools.build.lib.exec;

import com.google.devtools.build.lib.actions.ActionContext;
import com.google.devtools.build.lib.actions.Spawn;
import javax.annotation.Nullable;

/**
Expand All @@ -29,5 +30,5 @@ public interface RemoteLocalFallbackRegistry extends ActionContext {
* @return remote fallback strategy or {@code null} if none was registered
*/
@Nullable
AbstractSpawnStrategy getRemoteLocalFallbackStrategy();
AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn);
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.ListMultimap;
Expand All @@ -34,6 +35,7 @@
import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnStrategy;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.Reporter;
Expand Down Expand Up @@ -69,6 +71,7 @@ public final class SpawnStrategyRegistry

private final ImmutableListMultimap<String, SpawnStrategy> mnemonicToStrategies;
private final StrategyRegexFilter strategyRegexFilter;
private final StrategyPlatformFilter strategyPlatformFilter;
private final ImmutableList<? extends SpawnStrategy> defaultStrategies;
private final ImmutableMultimap<String, SandboxedSpawnStrategy> mnemonicToRemoteDynamicStrategies;
private final ImmutableMultimap<String, SandboxedSpawnStrategy> mnemonicToLocalDynamicStrategies;
Expand All @@ -77,12 +80,14 @@ public final class SpawnStrategyRegistry
private SpawnStrategyRegistry(
ImmutableListMultimap<String, SpawnStrategy> mnemonicToStrategies,
StrategyRegexFilter strategyRegexFilter,
StrategyPlatformFilter strategyPlatformFilter,
ImmutableList<? extends SpawnStrategy> defaultStrategies,
ImmutableMultimap<String, SandboxedSpawnStrategy> mnemonicToRemoteDynamicStrategies,
ImmutableMultimap<String, SandboxedSpawnStrategy> mnemonicToLocalDynamicStrategies,
@Nullable AbstractSpawnStrategy remoteLocalFallbackStrategy) {
this.mnemonicToStrategies = mnemonicToStrategies;
this.strategyRegexFilter = strategyRegexFilter;
this.strategyPlatformFilter = strategyPlatformFilter;
this.defaultStrategies = defaultStrategies;
this.mnemonicToRemoteDynamicStrategies = mnemonicToRemoteDynamicStrategies;
this.mnemonicToLocalDynamicStrategies = mnemonicToLocalDynamicStrategies;
Expand All @@ -105,7 +110,8 @@ private SpawnStrategyRegistry(
* using the given {@link Reporter}.
*/
public List<? extends SpawnStrategy> getStrategies(Spawn spawn, @Nullable EventHandler reporter) {
return getStrategies(spawn.getResourceOwner(), spawn.getMnemonic(), reporter);
return strategyPlatformFilter.getStrategies(
spawn, getStrategies(spawn.getResourceOwner(), spawn.getMnemonic(), reporter));
}

Comment on lines +113 to 116
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Platform filtering is applied, but null execution-platforms will crash builds

getStrategies unconditionally calls strategyPlatformFilter.getStrategies(spawn, …).
StrategyPlatformFilter#getStrategies has

Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform.");

Many host-only actions (coverage report, build‐info extraction, etc.) still run with a null execution platform. Today that only logs a warning – after this change Bazel will abort with NullPointerException / failed precondition.

Please make the filter a no-op when spawn.getExecutionPlatformLabel() is null:

- var platformLabel = spawn.getExecutionPlatformLabel();
- Preconditions.checkNotNull(platformLabel,
-     "Attempting to spawn action without an execution platform.");
+ @Nullable var platformLabel = spawn.getExecutionPlatformLabel();
+ if (platformLabel == null) {
+   // Legacy or host actions – no platform-specific restrictions.
+   return candidateStrategies;
+ }

/**
Expand All @@ -116,6 +122,8 @@ public List<? extends SpawnStrategy> getStrategies(Spawn spawn, @Nullable EventH
*
* <p>If the reason for selecting the context is worth mentioning to the user, logs a message
* using the given {@link Reporter}.
*
* NOTE: This method is public for Blaze, getStrategies(Spawn, EventHandler) must be used in Bazel.
*/
public List<? extends SpawnStrategy> getStrategies(
ActionExecutionMetadata resourceOwner, String mnemonic, @Nullable EventHandler reporter) {
Expand Down Expand Up @@ -154,7 +162,7 @@ public ImmutableCollection<SandboxedSpawnStrategy> getDynamicSpawnActionContexts
? mnemonicToRemoteDynamicStrategies
: mnemonicToLocalDynamicStrategies;
if (mnemonicToDynamicStrategies.containsKey(spawn.getMnemonic())) {
return mnemonicToDynamicStrategies.get(spawn.getMnemonic());
return strategyPlatformFilter.getStrategies(spawn, mnemonicToDynamicStrategies.get(spawn.getMnemonic()));
}
if (mnemonicToDynamicStrategies.containsKey("")) {
return mnemonicToDynamicStrategies.get("");
Expand All @@ -164,8 +172,9 @@ public ImmutableCollection<SandboxedSpawnStrategy> getDynamicSpawnActionContexts

@Nullable
@Override
public AbstractSpawnStrategy getRemoteLocalFallbackStrategy() {
return remoteLocalFallbackStrategy;
public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) {
return strategyPlatformFilter.getStrategies(spawn, Lists.newArrayList(remoteLocalFallbackStrategy))
.getFirst();
}

/**
Expand Down Expand Up @@ -203,9 +212,16 @@ void logSpawnStrategies() {
strategyRegexFilter.getFilterToStrategies().asMap().entrySet()) {
Collection<SpawnStrategy> value = entry.getValue();
logger.atInfo().log(
"FilterToStrategyImplementations: \"%s\" = [%s]",
"FilterDescriptionToStrategyImplementations: \"%s\" = [%s]",
entry.getKey(), toImplementationNames(value));
}
for (Map.Entry<Label, ImmutableList<SpawnStrategy>> entry :
strategyPlatformFilter.getFilterToStrategies().entrySet()) {
Collection<SpawnStrategy> value = entry.getValue();
logger.atInfo().log(
"FilterPlatformToStrategyImplementations: \"%s\" = [%s]",
entry.getKey().getCanonicalName(), toImplementationNames(value));
Comment on lines +218 to +223

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider using a more appropriate logging level, such as DEBUG, since this information might not be relevant for all users.

      logger.atDebug().log(
          "FilterPlatformToStrategyImplementations: \"%s\" = [%s]",
          entry.getKey().getCanonicalName(), toImplementationNames(value));

}

logger.atInfo().log(
"DefaultStrategyImplementations: [%s]", toImplementationNames(defaultStrategies));
Expand Down Expand Up @@ -287,6 +303,7 @@ public static final class Builder {
private final HashMap<String, List<String>> mnemonicToRemoteDynamicIdentifiers =
new HashMap<>();
private final HashMap<String, List<String>> mnemonicToLocalDynamicIdentifiers = new HashMap<>();
private final HashMap<Label, List<String>> execPlatformFilters = new HashMap<>();

@Nullable private String remoteLocalFallbackStrategyIdentifier;

Expand Down Expand Up @@ -314,6 +331,12 @@ public Builder addDescriptionFilter(RegexFilter filter, List<String> identifiers
return this;
}

@CanIgnoreReturnValue
public Builder addExecPlatformFilter(Label execPlatform, List<String> identifiers) {
this.execPlatformFilters.put(execPlatform, identifiers);
return this;
}

/**
* Adds a filter limiting any spawn whose {@linkplain Spawn#getMnemonic() mnemonic}
* (case-sensitively) matches the given mnemonic to only use strategies with the given
Expand Down Expand Up @@ -444,6 +467,14 @@ public SpawnStrategyRegistry build() throws AbruptExitException {
}
}

ImmutableMap.Builder<Label, ImmutableList<SpawnStrategy>> platformToStrategies = ImmutableMap.builder();
for (Map.Entry<Label, List<String>> entry : execPlatformFilters.entrySet()) {
Label platform = entry.getKey();
platformToStrategies.put(
platform,
strategyMapper.toStrategies(entry.getValue(), "platform " + platform.getCanonicalName()));
}

ImmutableListMultimap.Builder<String, SpawnStrategy> mnemonicToStrategies =
new ImmutableListMultimap.Builder<>();
for (Map.Entry<String, List<String>> entry : mnemonicToIdentifiers.entrySet()) {
Expand Down Expand Up @@ -514,6 +545,7 @@ public SpawnStrategyRegistry build() throws AbruptExitException {
mnemonicToStrategies.build(),
new StrategyRegexFilter(
strategyMapper, strategyPolicy, filterToIdentifiers, filterToStrategies),
new StrategyPlatformFilter(strategyMapper, platformToStrategies.build()),
defaultStrategies,
mnemonicToRemoteStrategies.build(),
mnemonicToLocalStrategies.build(),
Expand Down Expand Up @@ -595,6 +627,46 @@ public String toString() {
}
}

private static class StrategyPlatformFilter {
private final StrategyMapper strategyMapper;
private final ImmutableMap<Label, ImmutableList<SpawnStrategy>> platformToStrategies;

private StrategyPlatformFilter(
StrategyMapper strategyMapper,
ImmutableMap<Label, ImmutableList<SpawnStrategy>> platformToStrategies) {
this.strategyMapper = strategyMapper;
this.platformToStrategies = platformToStrategies;
}

public <T extends SpawnStrategy> List<T> getStrategies(
Spawn spawn, List<T> candidateStrategies) {
var platformLabel = spawn.getExecutionPlatformLabel();
Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform.");
Comment on lines +643 to +644

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This Preconditions.checkNotNull call could throw a NullPointerException if platformLabel is null. Consider adding a null check before dereferencing platformLabel to avoid this. Also, the message is not very informative. It should be updated to provide more context about why the execution platform is required.

      if (platformLabel == null) {
        // Log a warning or throw a more informative exception here
        return candidateStrategies;
      }
      Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform.");


var allowedStrategies = platformToStrategies.get(platformLabel);
if (allowedStrategies != null) {
List<T> filteredStrategies = new ArrayList<>();
for (var strategy : candidateStrategies) {
if (allowedStrategies.contains(strategy)) {
filteredStrategies.add(strategy);
}
}
return filteredStrategies;
}

return candidateStrategies;
}

public <T extends SpawnStrategy> ImmutableCollection<T> getStrategies(
Spawn spawn, ImmutableCollection<T> candidateStrategies) {
return ImmutableList.copyOf(getStrategies(spawn, Lists.newCopyOnWriteArrayList(candidateStrategies)));
}

ImmutableMap<Label, ImmutableList<SpawnStrategy>> getFilterToStrategies() {
return platformToStrategies;
}
}

/* Maps the strategy identifier (e.g. "local", "worker"..) to the real strategy. */
private static class StrategyMapper {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,12 @@ public List<? extends SpawnStrategy> resolve(
if (fallbackStrategies.isEmpty()) {
String message =
String.format(
"%s spawn cannot be executed with any of the available strategies: %s. Your"
+ " --spawn_strategy, --genrule_strategy and/or --strategy flags are probably"
+ " too strict. Visit https://github.com/bazelbuild/bazel/issues/7480 for"
+ " advice",
"""
%s spawn cannot be executed with any of the available strategies: %s. Your \
--spawn_strategy, --genrule_strategy, strategy and/or \
--allowed_strategies_by_exec_platform flags are probably too strict. \
Visit https://github.com/bazelbuild/bazel/issues/7480 for advice.
""",
spawn.getMnemonic(), strategies);
throw new UserExecException(
FailureDetail.newBuilder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,7 @@ private SpawnResult execLocally(Spawn spawn, SpawnExecutionContext context)
context.getContext(RemoteLocalFallbackRegistry.class);
checkNotNull(localFallbackRegistry, "Expected a RemoteLocalFallbackRegistry to be registered");
AbstractSpawnStrategy remoteLocalFallbackStrategy =
localFallbackRegistry.getRemoteLocalFallbackStrategy();
localFallbackRegistry.getRemoteLocalFallbackStrategy(spawn);
checkNotNull(
remoteLocalFallbackStrategy,
"A remote local fallback strategy must be set if using remote fallback.");
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/exec/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
"//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
"//src/main/java/com/google/devtools/build/lib/analysis/platform",
"//src/main/java/com/google/devtools/build/lib/bazel/rules/python",
"//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
"//src/main/java/com/google/devtools/build/lib/clock",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.SpawnStrategy;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.events.Event;
Expand Down Expand Up @@ -300,6 +302,24 @@ public void testDuplicatedDescriptionFilter() throws Exception {
.containsExactly(strategy2);
}

@Test
public void testPlatformFilter() throws Exception {
NoopStrategy strategy1 = new NoopStrategy("1");
NoopStrategy strategy2 = new NoopStrategy("2");
SpawnStrategyRegistry strategyRegistry =
SpawnStrategyRegistry.builder()
.registerStrategy(strategy1, "foo")
.registerStrategy(strategy2, "bar")
.addExecPlatformFilter(PlatformInfo.EMPTY_PLATFORM_INFO.label(), ImmutableList.of("foo"))
.build();

assertThat(
strategyRegistry.getStrategies(
createSpawnWithMnemonicAndDescription("", ""),
SpawnStrategyRegistryTest::noopEventHandler))
.containsExactly(strategy1);
}

@Test
public void testMultipleDefaultStrategies() throws Exception {
NoopStrategy strategy1 = new NoopStrategy("1");
Expand Down Expand Up @@ -537,7 +557,7 @@ public void testRemoteLocalFallback() throws Exception {
.setRemoteLocalFallbackStrategyIdentifier("bar")
.build();

assertThat(strategyRegistry.getRemoteLocalFallbackStrategy()).isEqualTo(strategy2);
assertThat(strategyRegistry.getRemoteLocalFallbackStrategy(createSpawnWithMnemonicAndDescription("", ""))).isEqualTo(strategy2);
}

@Test
Expand All @@ -561,7 +581,7 @@ public void testRemoteLocalFallbackNotRegistered() throws Exception {
SpawnStrategyRegistry strategyRegistry =
SpawnStrategyRegistry.builder().registerStrategy(strategy1, "foo").build();

assertThat(strategyRegistry.getRemoteLocalFallbackStrategy()).isNull();
assertThat(strategyRegistry.getRemoteLocalFallbackStrategy(createSpawnWithMnemonicAndDescription("", ""))).isNull();
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,11 @@ private FakeOwner(
}

public FakeOwner(String mnemonic, String progressMessage, String ownerLabel) {
this(mnemonic, progressMessage, checkNotNull(ownerLabel), null);
this(
mnemonic,
progressMessage,
checkNotNull(ownerLabel),
PlatformInfo.EMPTY_PLATFORM_INFO);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ private FakeSpawnExecutionContext getSpawnContext(Spawn spawn) {
AbstractSpawnStrategy fakeLocalStrategy =
new AbstractSpawnStrategy(execRoot, localRunner, new ExecutionOptions()) {};
ClassToInstanceMap<ActionContext> actionContextRegistry =
ImmutableClassToInstanceMap.of(RemoteLocalFallbackRegistry.class, () -> fakeLocalStrategy);
ImmutableClassToInstanceMap.of(RemoteLocalFallbackRegistry.class, (_spawn) -> fakeLocalStrategy);

var actionInputFetcher =
new RemoteActionInputFetcher(
Expand Down