Skip to content

Commit

Permalink
Set a fallback dynamic local strategy even when the dynamic_local_str…
Browse files Browse the repository at this point in the history
…ategy flag is passed.

RELNOTES: n/a
PiperOrigin-RevId: 344274807
  • Loading branch information
larsrc-google authored and copybara-github committed Nov 25, 2020
1 parent 3c9ad2d commit b32349f
Show file tree
Hide file tree
Showing 5 changed files with 178 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.devtools.build.lib.actions.Spawn;
Expand All @@ -35,6 +35,7 @@
import com.google.devtools.build.lib.util.AbruptExitException;
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.common.options.OptionsBase;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ExecutorService;
Expand Down Expand Up @@ -69,40 +70,53 @@ public void beforeCommand(CommandEnvironment env) {
env.getEventBus().register(this);
}

private List<Map.Entry<String, List<String>>> getLocalStrategies(
DynamicExecutionOptions options) {
@VisibleForTesting
ImmutableMap<String, List<String>> getLocalStrategies(DynamicExecutionOptions options)
throws AbruptExitException {
// Options that set "allowMultiple" to true ignore the default value, so we replicate that
// functionality here. Additionally, since we are still supporting --dynamic_worker_strategy,
// but will deprecate it soon, we add its functionality to --dynamic_local_strategy. This allows
// users to set --dynamic_local_strategy and not --dynamic_worker_strategy to stop defaulting to
// worker strategy.
// worker strategy. For simplicity, we add the default strategy first, it may be overridden
// later.
// ImmutableMap.Builder fails on duplicates, so we use a regular map first to remove dups.
Map<String, List<String>> localAndWorkerStrategies = new HashMap<>();
// TODO(steinman): Deprecate --dynamic_worker_strategy and clean this up.
if (options.dynamicLocalStrategy == null || options.dynamicLocalStrategy.isEmpty()) {
String workerStrategy =
options.dynamicWorkerStrategy.isEmpty() ? "worker" : options.dynamicWorkerStrategy;
return ImmutableList.of(
Maps.immutableEntry("", ImmutableList.of(workerStrategy, "sandboxed")));
}

ImmutableList.Builder<Map.Entry<String, List<String>>> localAndWorkerStrategies =
ImmutableList.builder();
for (Map.Entry<String, List<String>> entry : options.dynamicLocalStrategy) {
if ("".equals(entry.getKey())) {
List<String> newValue = Lists.newArrayList(options.dynamicWorkerStrategy);
newValue.addAll(entry.getValue());
localAndWorkerStrategies.add(Maps.immutableEntry("", newValue));
} else {
localAndWorkerStrategies.add(entry);
List<String> defaultValue = Lists.newArrayList();
String workerStrategy =
options.dynamicWorkerStrategy.isEmpty() ? "worker" : options.dynamicWorkerStrategy;
defaultValue.addAll(ImmutableList.of(workerStrategy, "sandboxed"));
throwIfContainsDynamic(defaultValue, "--dynamic_local_strategy");
localAndWorkerStrategies.put("", defaultValue);

if (!options.dynamicLocalStrategy.isEmpty()) {
for (Map.Entry<String, List<String>> entry : options.dynamicLocalStrategy) {
if ("".equals(entry.getKey())) {
List<String> newValue = Lists.newArrayList();
if (!options.dynamicWorkerStrategy.isEmpty()) {
newValue.add(options.dynamicWorkerStrategy);
}
newValue.addAll(entry.getValue());
localAndWorkerStrategies.put("", newValue);
} else {
localAndWorkerStrategies.put(entry.getKey(), entry.getValue());
}
throwIfContainsDynamic(entry.getValue(), "--dynamic_local_strategy");
}
}
return localAndWorkerStrategies.build();
return ImmutableMap.copyOf(localAndWorkerStrategies);
}

private List<Map.Entry<String, List<String>>> getRemoteStrategies(
DynamicExecutionOptions options) {
return (options.dynamicRemoteStrategy == null || options.dynamicRemoteStrategy.isEmpty())
? ImmutableList.of(Maps.immutableEntry("", ImmutableList.of("remote")))
: options.dynamicRemoteStrategy;
private ImmutableMap<String, List<String>> getRemoteStrategies(DynamicExecutionOptions options)
throws AbruptExitException {
Map<String, List<String>> strategies = new HashMap<>(); // Needed to dedup
for (Map.Entry<String, List<String>> e : options.dynamicRemoteStrategy) {
throwIfContainsDynamic(e.getValue(), "--dynamic_remote_strategy");
strategies.put(e.getKey(), e.getValue());
}
return options.dynamicRemoteStrategy.isEmpty()
? ImmutableMap.of("", ImmutableList.of("remote"))
: ImmutableMap.copyOf(strategies);
}

@Override
Expand Down Expand Up @@ -130,16 +144,8 @@ final void registerSpawnStrategies(
}
registryBuilder.registerStrategy(strategy, "dynamic", "dynamic_worker");

for (Map.Entry<String, List<String>> mnemonicToStrategies : getLocalStrategies(options)) {
throwIfContainsDynamic(mnemonicToStrategies.getValue(), "--dynamic_local_strategy");
registryBuilder.addDynamicLocalStrategiesByMnemonic(
mnemonicToStrategies.getKey(), mnemonicToStrategies.getValue());
}
for (Map.Entry<String, List<String>> mnemonicToStrategies : getRemoteStrategies(options)) {
throwIfContainsDynamic(mnemonicToStrategies.getValue(), "--dynamic_remote_strategy");
registryBuilder.addDynamicRemoteStrategiesByMnemonic(
mnemonicToStrategies.getKey(), mnemonicToStrategies.getValue());
}
registryBuilder.addDynamicLocalStrategies(getLocalStrategies(options));
registryBuilder.addDynamicRemoteStrategies(getRemoteStrategies(options));
}

private void throwIfContainsDynamic(List<String> strategies, String flagName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -334,28 +334,28 @@ public Builder resetDefaultStrategies() {
}

/**
* Sets the strategy names to use in the remote branch of dynamic execution for a given action
* mnemonic.
* Sets the strategy names to use in the remote branch of dynamic execution for a set of action
* mnemonics.
*
* <p>During execution, each strategy is {@linkplain SpawnStrategy#canExec(Spawn,
* ActionContextRegistry) asked} whether it can execute a given Spawn. The first strategy in the
* list that says so will get the job.
*/
public Builder addDynamicRemoteStrategiesByMnemonic(String mnemonic, List<String> strategies) {
mnemonicToRemoteIdentifiers.put(mnemonic, strategies);
public Builder addDynamicRemoteStrategies(Map<String, List<String>> strategies) {
mnemonicToRemoteIdentifiers.putAll(strategies);
return this;
}

/**
* Sets the strategy names to use in the local branch of dynamic execution for a given action
* mnemonic.
* Sets the strategy names to use in the local branch of dynamic execution for a number of
* action mnemonics.
*
* <p>During execution, each strategy is {@linkplain SpawnStrategy#canExec(Spawn,
* ActionContextRegistry) asked} whether it can execute a given Spawn. The first strategy in the
* list that says so will get the job.
*/
public Builder addDynamicLocalStrategiesByMnemonic(String mnemonic, List<String> strategies) {
mnemonicToLocalIdentifiers.put(mnemonic, strategies);
public Builder addDynamicLocalStrategies(Map<String, List<String>> strategies) {
mnemonicToLocalIdentifiers.putAll(strategies);
return this;
}

Expand Down
14 changes: 14 additions & 0 deletions src/test/java/com/google/devtools/build/lib/dynamic/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,20 @@ java_test(
],
)

java_test(
name = "DynamicExecutionModuleTest",
size = "small",
srcs = ["DynamicExecutionModuleTest.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib/dynamic",
"//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
"//src/main/java/com/google/devtools/common/options",
"//third_party:guava",
"//third_party:junit4",
"//third_party:truth",
],
)

test_suite(
name = "windows_tests",
tags = [
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
// Copyright 2020 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.devtools.build.lib.dynamic;

import static com.google.common.truth.Truth.assertThat;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.devtools.build.lib.util.AbruptExitException;
import com.google.devtools.common.options.Converters;
import com.google.devtools.common.options.OptionsParsingException;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/** Tests for {@link com.google.devtools.build.lib.dynamic.DynamicExecutionModule}. */
@RunWith(JUnit4.class)
public class DynamicExecutionModuleTest {
private DynamicExecutionModule module;
private DynamicExecutionOptions options;

@Before
public void setUp() {
module = new DynamicExecutionModule();
options = new DynamicExecutionOptions();
options.dynamicWorkerStrategy = ""; // default
options.dynamicLocalStrategy = Collections.emptyList(); // default
options.dynamicRemoteStrategy = Collections.emptyList(); // default
}

@Test
public void testGetLocalStrategies_getsDefaultWithNoOptions()
throws AbruptExitException, OptionsParsingException {
assertThat(module.getLocalStrategies(options)).isEqualTo(parseStrategies("worker,sandboxed"));
}

@Test
public void testGetLocalStrategies_dynamicWorkerStrategyTakesSingleValue()
throws AbruptExitException, OptionsParsingException {
options.dynamicWorkerStrategy = "local,worker";
// This looks weird, but it's expected behaviour that dynamic_worker_strategy
// doesn't get parsed.
Map<String, List<String>> expected = parseStrategies("sandboxed");
expected.get("").add(0, "local,worker");
assertThat(module.getLocalStrategies(options))
.isEqualTo(ImmutableMap.copyOf(expected.entrySet()));
}

@Test
public void testGetLocalStrategies_genericOptionOverridesFallbacks()
throws AbruptExitException, OptionsParsingException {
options.dynamicLocalStrategy = parseStrategiesToOptions("local,worker");
assertThat(module.getLocalStrategies(options)).isEqualTo(parseStrategies("local,worker"));
}

@Test
public void testGetLocalStrategies_specificOptionKeepsFallbacks()
throws AbruptExitException, OptionsParsingException {
options.dynamicLocalStrategy = parseStrategiesToOptions("Foo=local,worker");
assertThat(module.getLocalStrategies(options))
.isEqualTo(parseStrategies("Foo=local,worker", "worker,sandboxed"));
}

@Test
public void testGetLocalStrategies_canMixSpecificsAndGenericOptions()
throws AbruptExitException, OptionsParsingException {
options.dynamicLocalStrategy = parseStrategiesToOptions("Foo=local,worker", "worker");
assertThat(module.getLocalStrategies(options))
.isEqualTo(parseStrategies("Foo=local,worker", "worker"));
}

private static List<Map.Entry<String, List<String>>> parseStrategiesToOptions(
String... strategies) throws OptionsParsingException {
Map<String, List<String>> result = parseStrategies(strategies);
return Lists.newArrayList(result.entrySet());
}

private static Map<String, List<String>> parseStrategies(String... strategies)
throws OptionsParsingException {
Map<String, List<String>> result = new LinkedHashMap<>();
Converters.StringToStringListConverter converter = new Converters.StringToStringListConverter();
for (String s : strategies) {
Map.Entry<String, List<String>> converted = converter.convert(s);
// Have to avoid using Immutable* to allow overwriting elements.
result.put(converted.getKey(), Lists.newArrayList(converted.getValue()));
}
return result;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -314,8 +314,8 @@ public void testDynamicStrategies() throws Exception {
SpawnStrategyRegistry.builder()
.registerStrategy(strategy1, "foo")
.registerStrategy(strategy2, "bar")
.addDynamicLocalStrategiesByMnemonic("mnem", ImmutableList.of("bar"))
.addDynamicRemoteStrategiesByMnemonic("mnem", ImmutableList.of("foo"))
.addDynamicLocalStrategies(ImmutableMap.of("mnem", ImmutableList.of("bar")))
.addDynamicRemoteStrategies(ImmutableMap.of("mnem", ImmutableList.of("foo")))
.build();

assertThat(
Expand All @@ -337,7 +337,7 @@ public void testDynamicStrategyNotPresent() {
() ->
SpawnStrategyRegistry.builder()
.registerStrategy(strategy1, "foo")
.addDynamicLocalStrategiesByMnemonic("mnem", ImmutableList.of("bar"))
.addDynamicLocalStrategies(ImmutableMap.of("mnem", ImmutableList.of("bar")))
.build());

assertThat(exception).hasMessageThat().containsMatch("bar.*Valid.*foo");
Expand All @@ -352,7 +352,7 @@ public void testDynamicStrategyNotSandboxed() {
() ->
SpawnStrategyRegistry.builder()
.registerStrategy(strategy1, "foo")
.addDynamicLocalStrategiesByMnemonic("mnem", ImmutableList.of("foo"))
.addDynamicLocalStrategies(ImmutableMap.of("mnem", ImmutableList.of("foo")))
.build());

assertThat(exception).hasMessageThat().containsMatch("sandboxed strategy");
Expand Down Expand Up @@ -423,8 +423,8 @@ public void testNotifyUsed() throws Exception {
.setDefaultStrategies(ImmutableList.of("9"))
.setDefaultStrategies(ImmutableList.of("3"))
.setRemoteLocalFallbackStrategyIdentifier("4")
.addDynamicLocalStrategiesByMnemonic("oy", ImmutableList.of("5"))
.addDynamicRemoteStrategiesByMnemonic("oy", ImmutableList.of("6"))
.addDynamicLocalStrategies(ImmutableMap.of("oy", ImmutableList.of("5")))
.addDynamicRemoteStrategies(ImmutableMap.of("oy", ImmutableList.of("6")))
.build();

strategyRegistry.notifyUsed(null);
Expand Down Expand Up @@ -463,9 +463,9 @@ public void testNotifyUsedDynamic() throws Exception {
.addDescriptionFilter(ELLO_MATCHER, ImmutableList.of("2"))
.setDefaultStrategies(ImmutableList.of("3"))
.setRemoteLocalFallbackStrategyIdentifier("4")
.addDynamicLocalStrategiesByMnemonic("oy", ImmutableList.of("7"))
.addDynamicLocalStrategiesByMnemonic("oy", ImmutableList.of("5"))
.addDynamicRemoteStrategiesByMnemonic("oy", ImmutableList.of("6"))
.addDynamicLocalStrategies(ImmutableMap.of("oy", ImmutableList.of("7")))
.addDynamicLocalStrategies(ImmutableMap.of("oy", ImmutableList.of("5")))
.addDynamicRemoteStrategies(ImmutableMap.of("oy", ImmutableList.of("6")))
.build();

strategyRegistry.notifyUsedDynamic(null);
Expand Down

0 comments on commit b32349f

Please sign in to comment.