From b32349f50ff3d958613aef9275751ad9d50d344c Mon Sep 17 00:00:00 2001 From: larsrc Date: Wed, 25 Nov 2020 10:16:08 -0800 Subject: [PATCH] Set a fallback dynamic local strategy even when the dynamic_local_strategy flag is passed. RELNOTES: n/a PiperOrigin-RevId: 344274807 --- .../lib/dynamic/DynamicExecutionModule.java | 78 +++++++------ .../build/lib/exec/SpawnStrategyRegistry.java | 16 +-- .../google/devtools/build/lib/dynamic/BUILD | 14 +++ .../dynamic/DynamicExecutionModuleTest.java | 105 ++++++++++++++++++ .../lib/exec/SpawnStrategyRegistryTest.java | 18 +-- 5 files changed, 178 insertions(+), 53 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModuleTest.java diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java index 0d4b2d77adbb41..93cadeac100e70 100644 --- a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java +++ b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java @@ -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; @@ -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; @@ -69,40 +70,53 @@ public void beforeCommand(CommandEnvironment env) { env.getEventBus().register(this); } - private List>> getLocalStrategies( - DynamicExecutionOptions options) { + @VisibleForTesting + ImmutableMap> 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> 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>> localAndWorkerStrategies = - ImmutableList.builder(); - for (Map.Entry> entry : options.dynamicLocalStrategy) { - if ("".equals(entry.getKey())) { - List newValue = Lists.newArrayList(options.dynamicWorkerStrategy); - newValue.addAll(entry.getValue()); - localAndWorkerStrategies.add(Maps.immutableEntry("", newValue)); - } else { - localAndWorkerStrategies.add(entry); + List 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> entry : options.dynamicLocalStrategy) { + if ("".equals(entry.getKey())) { + List 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>> getRemoteStrategies( - DynamicExecutionOptions options) { - return (options.dynamicRemoteStrategy == null || options.dynamicRemoteStrategy.isEmpty()) - ? ImmutableList.of(Maps.immutableEntry("", ImmutableList.of("remote"))) - : options.dynamicRemoteStrategy; + private ImmutableMap> getRemoteStrategies(DynamicExecutionOptions options) + throws AbruptExitException { + Map> strategies = new HashMap<>(); // Needed to dedup + for (Map.Entry> 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 @@ -130,16 +144,8 @@ final void registerSpawnStrategies( } registryBuilder.registerStrategy(strategy, "dynamic", "dynamic_worker"); - for (Map.Entry> mnemonicToStrategies : getLocalStrategies(options)) { - throwIfContainsDynamic(mnemonicToStrategies.getValue(), "--dynamic_local_strategy"); - registryBuilder.addDynamicLocalStrategiesByMnemonic( - mnemonicToStrategies.getKey(), mnemonicToStrategies.getValue()); - } - for (Map.Entry> 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 strategies, String flagName) diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java index 558640c08048bd..f654f74e3a3542 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java @@ -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. * *

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 strategies) { - mnemonicToRemoteIdentifiers.put(mnemonic, strategies); + public Builder addDynamicRemoteStrategies(Map> 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. * *

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 strategies) { - mnemonicToLocalIdentifiers.put(mnemonic, strategies); + public Builder addDynamicLocalStrategies(Map> strategies) { + mnemonicToLocalIdentifiers.putAll(strategies); return this; } diff --git a/src/test/java/com/google/devtools/build/lib/dynamic/BUILD b/src/test/java/com/google/devtools/build/lib/dynamic/BUILD index 4f8760ddb061de..9c6e719b94649c 100644 --- a/src/test/java/com/google/devtools/build/lib/dynamic/BUILD +++ b/src/test/java/com/google/devtools/build/lib/dynamic/BUILD @@ -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 = [ diff --git a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModuleTest.java b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModuleTest.java new file mode 100644 index 00000000000000..17a8c19cdb742c --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModuleTest.java @@ -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> 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>> parseStrategiesToOptions( + String... strategies) throws OptionsParsingException { + Map> result = parseStrategies(strategies); + return Lists.newArrayList(result.entrySet()); + } + + private static Map> parseStrategies(String... strategies) + throws OptionsParsingException { + Map> result = new LinkedHashMap<>(); + Converters.StringToStringListConverter converter = new Converters.StringToStringListConverter(); + for (String s : strategies) { + Map.Entry> converted = converter.convert(s); + // Have to avoid using Immutable* to allow overwriting elements. + result.put(converted.getKey(), Lists.newArrayList(converted.getValue())); + } + return result; + } +} diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java index 99af6a65caa7eb..6f7e2fca1c4693 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java @@ -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( @@ -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"); @@ -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"); @@ -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); @@ -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);