From 1962a59a5478f5ad374700b0abf0a718b1b3a7d3 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 17 May 2021 13:19:39 -0700 Subject: [PATCH] Fix the case where if all strategies for one branch of `dynamic` execution fail to accept (that is, refuse to even take) the action given, the whole action fails. Instead of seeing whether the other branch can run and the action that that it succeeded. NOTE: This is _not_ about the local/remote execution branch trying to run it and failing. Obviously that should fail the whole dynamic execution. This is about none of the strategies of one branch even stating that they can run it (based on what they return from `canExec`) If the local execution branch requires a specific architecture/OS for some action, and users' bazel is being run a machine that isn't that combination (assuming the local strategy given is already setup to return `false` in `canExec` in this case). Previously the whole execution of the dynamic execution would fail without even trying to see if remote execution succeeded. Now it will gracefully fallback to just waiting on remote strategy. To conditionally use workers for supported actions but still simultaneously kick off a dynamic run, the options`--stragegy=TaskMnemonic=dynamic --dynamic_local_strategy=TaskMnemonic=worker --dynamic_remote_strategy=TaskMnemonic=remote` can be given. Then any action with with the mnemonic `TaskMnemonic` that can't support `worker` execution (xor `remote` execution) will wait for the other execution branch to complete. If neither set of strategies can run the action, then the task fails. Previously, this would have failed if, for example, the `worker` strategy cannot handle the action, even if `remote` could have. This at first glance seems silly as if you know TaskMnemonic doesn't have a worker enabled implementation, why specify `worker` as the local strategy? But keep in mind any action can declare most any mnemonic. In this example, say that first rule doing a TaskMnemonic is worker enabled, so you add the flags. But then someone makes a bazel/starlark rule with the same mnemonic but different implementation (for example, a "mimic" rule), and that new one doesn't support worker. Then this becomes a case of "partial" worker support for a mnemonic. RELNOTES: If all strategies of one branch (the local or remote execution branch) of the `dynamic` strategy fail to even accept (via the response they give from `canExec`) the action, `dynamic` will now try to see if the other branch can accept it. (Trying to run it and it failing will still cause a failure if it was the first result, this is about strategies claiming they can't even try the action) PiperOrigin-RevId: 374265582 --- CONTRIBUTORS | 3 +- .../lib/dynamic/DynamicSpawnStrategy.java | 190 ++++++++++++++---- src/main/protobuf/failure_details.proto | 1 + .../lib/dynamic/DynamicSpawnStrategyTest.java | 80 +++++++- .../dynamic/DynamicSpawnStrategyUnitTest.java | 179 ++++++++++++++++- 5 files changed, 405 insertions(+), 48 deletions(-) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index 26d92e0f6f370c..bf277a0649fb78 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -108,4 +108,5 @@ Jonathan Dierksen Tony Aiuto Andy Scott Jamie Snape -Irina Chernushina \ No newline at end of file +Irina Chernushina +C. Sean Young diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java index 9022baa453c64a..db769266c5232b 100644 --- a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java @@ -35,14 +35,17 @@ import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.SpawnResult.Status; import com.google.devtools.build.lib.actions.SpawnStrategy; +import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.exec.ExecutionPolicy; +import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.server.FailureDetails.DynamicExecution; import com.google.devtools.build.lib.server.FailureDetails.DynamicExecution.Code; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.Path; import java.io.IOException; +import java.util.List; import java.util.Optional; import java.util.concurrent.Callable; import java.util.concurrent.CancellationException; @@ -265,12 +268,17 @@ private static ImmutableList waitBranches( ImmutableList remoteResult = waitBranch(remoteBranch); if (remoteResult != null && localResult != null) { - throw new AssertionError("One branch did not cancel the other one"); - } else if (remoteResult != null) { - return remoteResult; + throw new AssertionError( + String.format( + "Neither branch of %s cancelled the other one.", + spawn.getResourceOwner().getPrimaryOutput().prettyPrint())); } else if (localResult != null) { return localResult; + } else if (remoteResult != null) { + return remoteResult; } else { + // TODO(b/173153395): Sometimes gets thrown for currently unknown reasons. + // (sometimes happens in relation to the whole dynamic execution being cancelled) throw new AssertionError( "Neither branch completed. Local was " + (localBranch.isCancelled() ? "" : "not ") @@ -321,18 +329,142 @@ static void verifyAvailabilityInfo(DynamicExecutionOptions options, Spawn spawn) } } + private static boolean canExecLocalSpawn( + Spawn spawn, + ExecutionPolicy executionPolicy, + ActionContext.ActionContextRegistry actionContextRegistry, + DynamicStrategyRegistry dynamicStrategyRegistry) { + if (!executionPolicy.canRunLocally()) { + return false; + } + List localStrategies = + dynamicStrategyRegistry.getDynamicSpawnActionContexts( + spawn, DynamicStrategyRegistry.DynamicMode.LOCAL); + return localStrategies.stream() + .anyMatch( + s -> + (s.canExec(spawn, actionContextRegistry) + || s.canExecWithLegacyFallback(spawn, actionContextRegistry))); + } + + private boolean canExecLocal( + Spawn spawn, + ExecutionPolicy mainSpawnExecutionPolicy, + ActionContext.ActionContextRegistry actionContextRegistry, + DynamicStrategyRegistry dynamicStrategyRegistry) { + if (!canExecLocalSpawn( + spawn, mainSpawnExecutionPolicy, actionContextRegistry, dynamicStrategyRegistry)) { + return false; + } + // Present if there is a extra local spawn. Unset if not. + Optional canLocalSpawn = + getExtraSpawnForLocalExecution + .apply(spawn) + .map( + extraSpawn -> + canExecLocalSpawn( + extraSpawn, + getExecutionPolicy.apply(extraSpawn), + actionContextRegistry, + dynamicStrategyRegistry)); + return canLocalSpawn.orElse(true); + } + + private static boolean canExecRemote( + Spawn spawn, + ExecutionPolicy executionPolicy, + ActionContext.ActionContextRegistry actionContextRegistry, + DynamicStrategyRegistry dynamicStrategyRegistry) { + if (!executionPolicy.canRunRemotely()) { + return false; + } + List remoteStrategies = + dynamicStrategyRegistry.getDynamicSpawnActionContexts( + spawn, DynamicStrategyRegistry.DynamicMode.REMOTE); + return remoteStrategies.stream().anyMatch(s -> s.canExec(spawn, actionContextRegistry)); + } + + @Override + public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry) { + ExecutionPolicy executionPolicy = getExecutionPolicy.apply(spawn); + DynamicStrategyRegistry dynamicStrategyRegistry = + actionContextRegistry.getContext(DynamicStrategyRegistry.class); + + return canExecLocal(spawn, executionPolicy, actionContextRegistry, dynamicStrategyRegistry) + || canExecRemote(spawn, executionPolicy, actionContextRegistry, dynamicStrategyRegistry); + } + + /** + * Returns an error string for being unable to execute locally and/or remotely the given execution + * state. + * + *

Usage note, this method is only to be called after an impossible condition is already + * detected by the caller, as all this does is give an error string to put in the exception. + * + * @param spawn The action that needs to be executed + * @param localAllowedBySpawnExecutionPolicy whether the execution policy for this spawn allows + * trying local execution. + * @param remoteAllowedBySpawnExecutionPolicy whether the execution policy for this spawn allows + * trying remote execution. + */ + private static String getNoCanExecFailureMessage( + Spawn spawn, + boolean localAllowedBySpawnExecutionPolicy, + boolean remoteAllowedBySpawnExecutionPolicy) { + // TODO(b/188387840): Can't use Spawn.toString() here because tests report FakeOwner instances + // as the resource owner, and those cause toStrings to throw if no primary output. + // TODO(b/188402092): Even if the above is fixed, we still don't want to use Spawn.toString() + // until the mnemonic is included in the output unconditionally. Too useful for the error + // message. + if (!localAllowedBySpawnExecutionPolicy && !remoteAllowedBySpawnExecutionPolicy) { + return "Neither local nor remote execution allowed for action " + spawn.getMnemonic(); + } else if (!remoteAllowedBySpawnExecutionPolicy) { + return "No usable dynamic_local_strategy found (and remote execution disabled) for action " + + spawn.getMnemonic(); + } else if (!localAllowedBySpawnExecutionPolicy) { + return "No usable dynamic_remote_strategy found (and local execution disabled) for action " + + spawn.getMnemonic(); + } else { + return "No usable dynamic_local_strategy or dynamic_remote_strategy found for action " + + spawn.getMnemonic(); + } + } + @Override public ImmutableList exec( final Spawn spawn, final ActionExecutionContext actionExecutionContext) throws ExecException, InterruptedException { DynamicSpawnStrategy.verifyAvailabilityInfo(options, spawn); ExecutionPolicy executionPolicy = getExecutionPolicy.apply(spawn); - if (executionPolicy.canRunLocallyOnly()) { - return runLocally(spawn, actionExecutionContext, null); - } - if (executionPolicy.canRunRemotelyOnly()) { + + DynamicStrategyRegistry dynamicStrategyRegistry = + actionExecutionContext.getContext(DynamicStrategyRegistry.class); + boolean localCanExec = + canExecLocal(spawn, executionPolicy, actionExecutionContext, dynamicStrategyRegistry); + + boolean remoteCanExec = + canExecRemote(spawn, executionPolicy, actionExecutionContext, dynamicStrategyRegistry); + + if (!localCanExec && !remoteCanExec) { + FailureDetail failure = + FailureDetail.newBuilder() + .setMessage( + getNoCanExecFailureMessage( + spawn, executionPolicy.canRunLocally(), executionPolicy.canRunRemotely())) + .setDynamicExecution( + DynamicExecution.newBuilder().setCode(Code.NO_USABLE_STRATEGY_FOUND).build()) + .setSpawn( + FailureDetails.Spawn.newBuilder() + .setCode(FailureDetails.Spawn.Code.NO_USABLE_STRATEGY_FOUND) + .build()) + .build(); + throw new UserExecException(failure); + } else if (!localCanExec && remoteCanExec) { return runRemotely(spawn, actionExecutionContext, null); + } else if (localCanExec && !remoteCanExec) { + return runLocally(spawn, actionExecutionContext, null); } + // else both can exec. Fallthrough to below. // Semaphores to track termination of each branch. These are necessary to wait for the branch to // finish its own cleanup (e.g. terminating subprocesses) once it has been cancelled. @@ -448,28 +580,6 @@ public ImmutableList callImpl(ActionExecutionContext context) } } - @Override - public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry) { - DynamicStrategyRegistry dynamicStrategyRegistry = - actionContextRegistry.getContext(DynamicStrategyRegistry.class); - for (SandboxedSpawnStrategy strategy : - dynamicStrategyRegistry.getDynamicSpawnActionContexts( - spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) { - if (strategy.canExec(spawn, actionContextRegistry) - || strategy.canExecWithLegacyFallback(spawn, actionContextRegistry)) { - return true; - } - } - for (SandboxedSpawnStrategy strategy : - dynamicStrategyRegistry.getDynamicSpawnActionContexts( - spawn, DynamicStrategyRegistry.DynamicMode.REMOTE)) { - if (strategy.canExec(spawn, actionContextRegistry)) { - return true; - } - } - return false; - } - @Override public void usedContext(ActionContext.ActionContextRegistry actionContextRegistry) { actionContextRegistry @@ -486,6 +596,12 @@ private static FileOutErr getSuffixedFileOutErr(FileOutErr fileOutErr, String su outDir.getChild(outBaseName + suffix), errDir.getChild(errBaseName + suffix)); } + /** + * Try to run the given spawn locally. + * + *

Precondition: At least one {@code dynamic_local_strategy} returns {@code true} from its + * {@link SpawnStrategy#canExec canExec} method for the given {@code spawn}. + */ private ImmutableList runLocally( Spawn spawn, ActionExecutionContext actionExecutionContext, @@ -529,12 +645,15 @@ private static ImmutableList runSpawnLocally( return strategy.exec(spawn, actionExecutionContext, stopConcurrentSpawns); } } - throw new RuntimeException( - String.format( - "executorCreated not yet called or no default dynamic_local_strategy set for %s", - spawn.getMnemonic())); + throw new AssertionError("canExec passed but no usable local strategy for action " + spawn); } + /** + * Try to run the given spawn locally. + * + *

Precondition: At least one {@code dynamic_remote_strategy} returns {@code true} from its + * {@link SpawnStrategy#canExec canExec} method for the given {@code spawn}. + */ private static ImmutableList runRemotely( Spawn spawn, ActionExecutionContext actionExecutionContext, @@ -550,10 +669,7 @@ private static ImmutableList runRemotely( return strategy.exec(spawn, actionExecutionContext, stopConcurrentSpawns); } } - throw new RuntimeException( - String.format( - "executorCreated not yet called or no default dynamic_remote_strategy set for %s", - spawn.getMnemonic())); + throw new AssertionError("canExec passed but no usable remote strategy for action " + spawn); } /** diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto index 3fae56ee4e88e2..b03a180f8a5c4e 100644 --- a/src/main/protobuf/failure_details.proto +++ b/src/main/protobuf/failure_details.proto @@ -1036,6 +1036,7 @@ message DynamicExecution { XCODE_RELATED_PREREQ_UNMET = 1 [(metadata) = { exit_code: 36 }]; ACTION_LOG_MOVE_FAILURE = 2 [(metadata) = { exit_code: 1 }]; RUN_FAILURE = 3 [(metadata) = { exit_code: 1 }]; + NO_USABLE_STRATEGY_FOUND = 4 [(metadata) = { exit_code: 2 }]; } Code code = 1; diff --git a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java index f3a2f5b9d0392f..9e7412c8be6af7 100644 --- a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java @@ -137,14 +137,22 @@ private class MockSpawnStrategy implements SandboxedSpawnStrategy { private final DoExec doExecAfterStop; + private final boolean canExec; + MockSpawnStrategy(String name) { this(name, DoExec.NOTHING, DoExec.NOTHING); } MockSpawnStrategy(String name, DoExec doExecBeforeStop, DoExec doExecAfterStop) { + this(name, doExecBeforeStop, doExecAfterStop, true); + } + + MockSpawnStrategy( + String name, DoExec doExecBeforeStop, DoExec doExecAfterStop, boolean canExec) { this.name = name; this.doExecBeforeStop = doExecBeforeStop; this.doExecAfterStop = doExecAfterStop; + this.canExec = canExec; } /** Helper to record an execution failure from within {@link #doExecBeforeStop}. */ @@ -201,7 +209,7 @@ public ImmutableList exec( @Override public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry) { - return true; + return canExec; } @Nullable @@ -561,6 +569,48 @@ public void actionSucceedsIfRemoteExecutionSucceedsEvenIfLocalFailsLater() throw assertThat(outErr.outAsLatin1()).doesNotContain("MockLocalSpawnStrategy"); } + @Test + public void actionSucceedsIfLocalExecutionSucceedsEvenIfRemoteRunsNothing() throws Exception { + MockSpawnStrategy localStrategy = new MockSpawnStrategy("MockLocalSpawnStrategy"); + + MockSpawnStrategy remoteStrategy = + new MockSpawnStrategy("MockRemoteSpawnStrategy", DoExec.NOTHING, DoExec.NOTHING, false); + + StrategyAndContext strategyAndContext = createSpawnStrategy(localStrategy, remoteStrategy); + + Spawn spawn = newDynamicSpawn(); + strategyAndContext.exec(spawn); + + assertThat(localStrategy.getExecutedSpawn()).isEqualTo(spawn); + assertThat(localStrategy.succeeded()).isTrue(); + assertThat(remoteStrategy.getExecutedSpawn()).isNull(); + assertThat(remoteStrategy.succeeded()).isFalse(); + + assertThat(outErr.outAsLatin1()).contains("output files written with MockLocalSpawnStrategy"); + assertThat(outErr.outAsLatin1()).doesNotContain("MockRemoteSpawnStrategy"); + } + + @Test + public void actionSucceedsIfRemoteExecutionSucceedsEvenIfLocalRunsNothing() throws Exception { + MockSpawnStrategy localStrategy = + new MockSpawnStrategy("MockLocalSpawnStrategy", DoExec.NOTHING, DoExec.NOTHING, false); + + MockSpawnStrategy remoteStrategy = new MockSpawnStrategy("MockRemoteSpawnStrategy"); + + StrategyAndContext strategyAndContext = createSpawnStrategy(localStrategy, remoteStrategy); + + Spawn spawn = newDynamicSpawn(); + strategyAndContext.exec(spawn); + + assertThat(localStrategy.getExecutedSpawn()).isNull(); + assertThat(localStrategy.succeeded()).isFalse(); + assertThat(remoteStrategy.getExecutedSpawn()).isEqualTo(spawn); + assertThat(remoteStrategy.succeeded()).isTrue(); + + assertThat(outErr.outAsLatin1()).contains("output files written with MockRemoteSpawnStrategy"); + assertThat(outErr.outAsLatin1()).doesNotContain("MockLocalSpawnStrategy"); + } + @Test public void actionFailsIfLocalFailsImmediatelyEvenIfRemoteSucceedsLater() throws Exception { CountDownLatch countDownLatch = new CountDownLatch(2); @@ -671,6 +721,34 @@ public void actionFailsIfLocalAndRemoteFail() throws Exception { assertThat(remoteStrategy.succeeded()).isFalse(); } + @Test + public void actionFailsIfLocalAndRemoteRunNothing() throws Exception { + MockSpawnStrategy localStrategy = + new MockSpawnStrategy("MockLocalSpawnStrategy", DoExec.NOTHING, DoExec.NOTHING, false); + + MockSpawnStrategy remoteStrategy = + new MockSpawnStrategy("MockRemoteSpawnStrategy", DoExec.NOTHING, DoExec.NOTHING, false); + + StrategyAndContext strategyAndContext = createSpawnStrategy(localStrategy, remoteStrategy); + + Spawn spawn = newDynamicSpawn(); + ExecException e = assertThrows(UserExecException.class, () -> strategyAndContext.exec(spawn)); + + // Has "No usable", followed by both dynamic_local_strategy and dynamic_remote_strategy in, + // followed by the action's mnemonic. + String regexMatch = + "[nN]o usable\\b.*\\bdynamic_local_strategy\\b.*\\bdynamic_remote_strategy\\b.*\\b" + + spawn.getMnemonic() + + "\\b"; + + assertThat(e).hasMessageThat().containsMatch(regexMatch); + + assertThat(localStrategy.getExecutedSpawn()).isNull(); + assertThat(localStrategy.succeeded()).isFalse(); + assertThat(remoteStrategy.getExecutedSpawn()).isNull(); + assertThat(remoteStrategy.succeeded()).isFalse(); + } + @Test public void stopConcurrentSpawnsWaitForCompletion() throws Exception { if (legacyBehavior) { diff --git a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyUnitTest.java b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyUnitTest.java index 60638d87b7d0c5..773f17a4a24bf0 100644 --- a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyUnitTest.java +++ b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyUnitTest.java @@ -16,6 +16,7 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.truth.Truth.assertThat; import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static org.junit.Assert.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNotNull; @@ -28,12 +29,14 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.ActionExecutionContext; +import com.google.devtools.build.lib.actions.Artifact.SourceArtifact; import com.google.devtools.build.lib.actions.DynamicStrategyRegistry; import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy; import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy.StopConcurrentSpawns; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.SpawnResult.Status; +import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.exec.ExecutionPolicy; import com.google.devtools.build.lib.exec.util.SpawnBuilder; import com.google.devtools.build.lib.server.FailureDetails.Execution; @@ -65,11 +68,29 @@ public class DynamicSpawnStrategyUnitTest { private ExecutorService executorServiceForCleanup; + /** + * {@see org.mockito.Mockito#verifyZeroInteractions} + * + *

TODO(b/188373809): {@link org.mockito.Mockito#verifyZeroInteractions + * Mockito.verifyZeroInteractions} is deprecated in Mockito 3.0.1, replaced with {@code + * verifyNoInteractions}. However, some of the builders on Google Bazel Presubmits on BuildKite + * have an older version of that, so we can't replace these calls yet. This function will serve to + * mute the depcrecation warnings until they are. At which point this method should be removed and + * {@code verifyNoInteractions} just be called instead. + */ + @SuppressWarnings("deprecation") + private static void verifyNoInteractions(Object... objs) { + verifyZeroInteractions(objs); + } + @Mock private Function> mockGetPostProcessingSpawn; @Before public void initMocks() { MockitoAnnotations.initMocks(this); + // Mockito can't see that we want the function to return Optional.empty() instead + // of null on apply by default (thanks generic type erasure). Set that up ourselves. + when(mockGetPostProcessingSpawn.apply(any())).thenReturn(Optional.empty()); } @After @@ -82,7 +103,7 @@ public void stopExecutorService() throws InterruptedException { } @Test - public void exec_remoteOnlySpawn_doesNotGetLocalPostProcessingSpawn() throws Exception { + public void exec_remoteOnlySpawn_doesNotExecLocalPostProcessingSpawn() throws Exception { DynamicSpawnStrategy dynamicSpawnStrategy = createDynamicSpawnStrategy( ExecutionPolicy.REMOTE_EXECUTION_ONLY, mockGetPostProcessingSpawn); @@ -102,6 +123,28 @@ public void exec_remoteOnlySpawn_doesNotGetLocalPostProcessingSpawn() throws Exc assertThat(remoteSpawnCaptor.getAllValues()).containsExactly(spawn); } + @Test + public void exec_remoteOnlySpawn_noneCanExec_fails() throws Exception { + Spawn spawn = new SpawnBuilder().withMnemonic("TheThing").build(); + DynamicSpawnStrategy dynamicSpawnStrategy = + createDynamicSpawnStrategy( + ExecutionPolicy.REMOTE_EXECUTION_ONLY, mockGetPostProcessingSpawn); + SandboxedSpawnStrategy local = createMockSpawnStrategy(); + SandboxedSpawnStrategy remote = createMockSpawnStrategy(false); + ActionExecutionContext actionExecutionContext = createMockActionExecutionContext(local, remote); + + UserExecException thrown = + assertThrows( + UserExecException.class, + () -> dynamicSpawnStrategy.exec(spawn, actionExecutionContext)); + assertThat(thrown).hasMessageThat().doesNotContain("dynamic_local_strategy"); + assertThat(thrown).hasMessageThat().containsMatch("\\bdynamic_remote_strategy\\b"); + assertThat(thrown).hasMessageThat().containsMatch("\\bTheThing\\b"); + verifyNoInteractions(local); + // No post processing because local never ran. + verify(mockGetPostProcessingSpawn, never()).apply(any()); + } + @Test public void exec_localOnlySpawn_runsLocalPostProcessingSpawn() throws Exception { Spawn spawn = new SpawnBuilder("command").build(); @@ -119,15 +162,15 @@ public void exec_localOnlySpawn_runsLocalPostProcessingSpawn() throws Exception ImmutableList results = dynamicSpawnStrategy.exec(spawn, actionExecutionContext); assertThat(results).containsExactly(SUCCESSFUL_SPAWN_RESULT, SUCCESSFUL_SPAWN_RESULT); - verifyZeroInteractions(remote); + verifyNoInteractions(remote); assertThat(localSpawnCaptor.getAllValues()) .containsExactly(spawn, postProcessingSpawn) .inOrder(); } @Test - public void exec_failedLocalSpawn_doesNotGetLocalPostProcessingSpawn() throws Exception { - testExecFailedLocalSpawnDoesNotGetLocalPostProcessingSpawn( + public void exec_failedLocalSpawn_doesNotExecLocalPostProcessingSpawn() throws Exception { + testExecFailedLocalSpawnDoesNotExecLocalPostProcessingSpawn( new SpawnResult.Builder() .setRunnerName("test") .setStatus(Status.TIMEOUT) @@ -137,8 +180,31 @@ public void exec_failedLocalSpawn_doesNotGetLocalPostProcessingSpawn() throws Ex } @Test - public void exec_nonZeroExitCodeLocalSpawn_doesNotGetLocalPostProcessingSpawn() throws Exception { - testExecFailedLocalSpawnDoesNotGetLocalPostProcessingSpawn( + public void exec_localOnlySpawn_noneCanExec_fails() throws Exception { + Spawn spawn = new SpawnBuilder().withMnemonic("TheThing").build(); + DynamicSpawnStrategy dynamicSpawnStrategy = + createDynamicSpawnStrategy( + ExecutionPolicy.LOCAL_EXECUTION_ONLY, mockGetPostProcessingSpawn); + SandboxedSpawnStrategy local = createMockSpawnStrategy(false); + SandboxedSpawnStrategy remote = createMockSpawnStrategy(); + ActionExecutionContext actionExecutionContext = createMockActionExecutionContext(local, remote); + + UserExecException thrown = + assertThrows( + UserExecException.class, + () -> dynamicSpawnStrategy.exec(spawn, actionExecutionContext)); + assertThat(thrown).hasMessageThat().containsMatch("\\bdynamic_local_strategy\\b"); + assertThat(thrown).hasMessageThat().doesNotContain("dynamic_remote_strategy"); + assertThat(thrown).hasMessageThat().containsMatch("\\bTheThing\\b"); + verifyNoInteractions(remote); + // No post processing because local never completed. + verify(mockGetPostProcessingSpawn, never()).apply(any()); + } + + @Test + public void exec_nonZeroExitCodeLocalSpawn_doesNotExecLocalPostProcessingSpawn() + throws Exception { + testExecFailedLocalSpawnDoesNotExecLocalPostProcessingSpawn( new SpawnResult.Builder() .setRunnerName("test") .setStatus(Status.EXECUTION_FAILED) @@ -147,7 +213,7 @@ public void exec_nonZeroExitCodeLocalSpawn_doesNotGetLocalPostProcessingSpawn() .build()); } - private void testExecFailedLocalSpawnDoesNotGetLocalPostProcessingSpawn(SpawnResult failedResult) + private void testExecFailedLocalSpawnDoesNotExecLocalPostProcessingSpawn(SpawnResult failedResult) throws Exception { DynamicSpawnStrategy dynamicSpawnStrategy = createDynamicSpawnStrategy( @@ -165,7 +231,6 @@ private void testExecFailedLocalSpawnDoesNotGetLocalPostProcessingSpawn(SpawnRes assertThat(results).containsExactly(failedResult); assertThat(localSpawnCaptor.getAllValues()).containsExactly(spawn); verify(remote, never()).exec(any(), any(), any()); - verify(mockGetPostProcessingSpawn, never()).apply(any()); } @Test @@ -205,6 +270,97 @@ public void exec_runAnywhereSpawn_runsLocalPostProcessingSpawn() throws Exceptio assertThat(results).containsExactly(SUCCESSFUL_SPAWN_RESULT, SUCCESSFUL_SPAWN_RESULT); } + @Test + public void waitBranches_givesDebugOutputOnWeirdCases() throws Exception { + Spawn spawn = + new SpawnBuilder() + .withOwnerPrimaryOutput(new SourceArtifact(rootDir, PathFragment.create("/foo"), null)) + .build(); + AssertionError error = + assertThrows( + AssertionError.class, + () -> + DynamicSpawnStrategy.waitBranches( + Futures.immediateFuture(null), Futures.immediateFuture(null), spawn)); + assertThat(error).hasMessageThat().contains("Neither branch of /foo completed."); + } + + @Test + public void exec_runAnywhereSpawn_localCantExec_runsRemote() throws Exception { + Spawn spawn = new SpawnBuilder().build(); + DynamicSpawnStrategy dynamicSpawnStrategy = + createDynamicSpawnStrategy(ExecutionPolicy.ANYWHERE, mockGetPostProcessingSpawn); + SandboxedSpawnStrategy local = createMockSpawnStrategy(false); + SandboxedSpawnStrategy remote = createMockSpawnStrategy(); + when(remote.exec(eq(spawn), any(), any())) + .thenAnswer( + invocation -> { + StopConcurrentSpawns stopConcurrentSpawns = invocation.getArgument(2); + if (stopConcurrentSpawns != null) { + stopConcurrentSpawns.stop(); + } + return ImmutableList.of(SUCCESSFUL_SPAWN_RESULT); + }); + ActionExecutionContext actionExecutionContext = createMockActionExecutionContext(local, remote); + + ImmutableList results = dynamicSpawnStrategy.exec(spawn, actionExecutionContext); + + assertThat(results).containsExactly(SUCCESSFUL_SPAWN_RESULT); + // Never runs anything as it says it can't execute anything at all. + verify(local, never()).exec(any(), any(), any()); + verify(mockGetPostProcessingSpawn, never()).apply(any()); + } + + @Test + public void exec_runAnywhereSpawn_remoteCantExec_runsLocal() throws Exception { + Spawn spawn = new SpawnBuilder().build(); + Spawn postProcessingSpawn = new SpawnBuilder("extra_command").build(); + DynamicSpawnStrategy dynamicSpawnStrategy = + createDynamicSpawnStrategy( + ExecutionPolicy.ANYWHERE, ignored -> Optional.of(postProcessingSpawn)); + SandboxedSpawnStrategy local = createMockSpawnStrategy(); + ArgumentCaptor localSpawnCaptor = ArgumentCaptor.forClass(Spawn.class); + when(local.exec(localSpawnCaptor.capture(), any(), any())) + .thenAnswer( + invocation -> { + StopConcurrentSpawns stopConcurrentSpawns = invocation.getArgument(2); + if (stopConcurrentSpawns != null) { + stopConcurrentSpawns.stop(); + } + return ImmutableList.of(SUCCESSFUL_SPAWN_RESULT); + }); + SandboxedSpawnStrategy remote = createMockSpawnStrategy(false); + ActionExecutionContext actionExecutionContext = createMockActionExecutionContext(local, remote); + + ImmutableList results = dynamicSpawnStrategy.exec(spawn, actionExecutionContext); + + assertThat(results).containsExactly(SUCCESSFUL_SPAWN_RESULT, SUCCESSFUL_SPAWN_RESULT); + assertThat(localSpawnCaptor.getAllValues()) + .containsExactly(spawn, postProcessingSpawn) + .inOrder(); + verify(remote, never()).exec(any(), any(), any()); + } + + @Test + public void exec_runAnywhereSpawn_noneCanExec_fails() throws Exception { + Spawn spawn = new SpawnBuilder().withMnemonic("TheThing").build(); + DynamicSpawnStrategy dynamicSpawnStrategy = + createDynamicSpawnStrategy(ExecutionPolicy.ANYWHERE, mockGetPostProcessingSpawn); + SandboxedSpawnStrategy local = createMockSpawnStrategy(false); + SandboxedSpawnStrategy remote = createMockSpawnStrategy(false); + ActionExecutionContext actionExecutionContext = createMockActionExecutionContext(local, remote); + + UserExecException thrown = + assertThrows( + UserExecException.class, + () -> dynamicSpawnStrategy.exec(spawn, actionExecutionContext)); + assertThat(thrown).hasMessageThat().containsMatch("\\bdynamic_local_strategy\\b"); + assertThat(thrown).hasMessageThat().containsMatch("\\bdynamic_remote_strategy\\b"); + assertThat(thrown).hasMessageThat().containsMatch("\\bTheThing\\b"); + // No post processing because local never completed. + verify(mockGetPostProcessingSpawn, never()).apply(any()); + } + private DynamicSpawnStrategy createDynamicSpawnStrategy( ExecutionPolicy executionPolicy, Function> getPostProcessingSpawnForLocalExecution) { @@ -246,8 +402,13 @@ public void notifyUsedDynamic(ActionContextRegistry actionContextRegistry) {} } private static SandboxedSpawnStrategy createMockSpawnStrategy() throws InterruptedException { + return createMockSpawnStrategy(true); + } + + private static SandboxedSpawnStrategy createMockSpawnStrategy(boolean canExec) + throws InterruptedException { SandboxedSpawnStrategy strategy = mock(SandboxedSpawnStrategy.class); - when(strategy.canExec(any(), any())).thenReturn(true); + when(strategy.canExec(any(), any())).thenReturn(canExec); when(strategy.beginExecution(any(), any())).thenThrow(UnsupportedOperationException.class); return strategy; }