From eea808cf058561ad6ab5ffc58e241dfdfe144ace Mon Sep 17 00:00:00 2001 From: Simon Bjorklen Date: Tue, 5 Nov 2024 15:26:49 +0100 Subject: [PATCH 1/2] Allow timeout propagation for remote actions When building using remote execution, action timeouts sometimes need to be adjusted per action to ensure reasonable timeouts are set. This change will allow better control over how actions behave and encourage users to optimize their actions since the global default timeout does not have to be set to the absolute largest action timeout in the graph. Change-Id: I0ea63787ff7769e25dd597575cbfd7fd1050797e --- .../build/lib/packages/TargetUtils.java | 1 + ...arlarkRuleImplementationFunctionsTest.java | 25 +++++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java index 55e084acfae340..c08ef7ce5e5aa7 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java +++ b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java @@ -58,6 +58,7 @@ private static boolean legalExecInfoKeys(String tag) { || tag.startsWith("disable-") || tag.startsWith("cpu:") || tag.equals(ExecutionRequirements.LOCAL) + || tag.equals(ExecutionRequirements.TIMEOUT) || tag.equals(ExecutionRequirements.WORKER_KEY_MNEMONIC) || tag.startsWith("resources:"); } diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java index ad270e0ebfe260..890c1817ef3db1 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java @@ -416,7 +416,7 @@ public void testCreateSpawnActionEnvAndExecInfo() throws Exception { " inputs = ruleContext.files.srcs,", " outputs = ruleContext.files.srcs,", " env = {'a' : 'b'},", - " execution_requirements = {'timeout' : '10', 'block-network' : 'foo'},", + " execution_requirements = {'block-network' : 'foo'},", " mnemonic = 'DummyMnemonic',", " command = 'dummy_command',", " progress_message = 'dummy_message')"); @@ -425,10 +425,31 @@ public void testCreateSpawnActionEnvAndExecInfo() throws Exception { Iterables.getOnlyElement( ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions()); assertThat(action.getIncompleteEnvironmentForTesting()).containsExactly("a", "b"); - // We expect "timeout" to be filtered by TargetUtils. assertThat(action.getExecutionInfo()).containsExactly("block-network", "foo"); } + @Test + public void testCreateSpawnActionEnvAndExecInfo_withTimeout() throws Exception { + StarlarkRuleContext ruleContext = createRuleContext("//foo:foo"); + setRuleContext(ruleContext); + ev.exec( + "ruleContext.actions.run_shell(", + " inputs = ruleContext.files.srcs,", + " outputs = ruleContext.files.srcs,", + " execution_requirements = {", + " 'timeout': '42',", + " },", + " mnemonic = 'DummyMnemonic',", + " command = 'dummy_command',", + " progress_message = 'dummy_message')"); + SpawnAction action = + (SpawnAction) + Iterables.getOnlyElement( + ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions()); + assertThat(action.getExecutionInfo()) + .containsExactly("timeout", "42"); + } + @Test public void testCreateSpawnActionEnvAndExecInfo_withWorkerKeyMnemonic() throws Exception { StarlarkRuleContext ruleContext = createRuleContext("//foo:foo"); From e22c7da69533f6e5089f39f8f4246524e4fd8da7 Mon Sep 17 00:00:00 2001 From: Simon Bjorklen Date: Fri, 15 Nov 2024 11:30:36 +0100 Subject: [PATCH 2/2] Add support for timeout: tags to populate ExecutionInfo Timeout may either be specified as a tag on format tags = ["tag:"] or as execution_requirements on any action on dict format execution_requirements = { "timeout": "" } Change-Id: Ida3bf173a291e7d949046bc4df3d1d4a02d8328d --- .../lib/actions/ExecutionRequirements.java | 26 ++++++- .../devtools/build/lib/actions/Spawns.java | 76 ++++++++++++++++--- .../lib/exec/StandaloneTestStrategy.java | 2 +- .../build/lib/packages/TargetUtils.java | 2 +- src/main/protobuf/failure_details.proto | 2 + .../build/lib/packages/TargetUtilsTest.java | 13 ++++ 6 files changed, 107 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java index 94576c64fa41c6..3089c4ff61bf3e 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java @@ -118,7 +118,31 @@ public String parseIfMatches(String tag) throws ValidationException { } /** If specified, the timeout of this action in seconds. Must be decimal integer. */ - public static final String TIMEOUT = "timeout"; + public static final ParseableRequirement TIMEOUT = + ParseableRequirement.create( + "timeout:", + Pattern.compile("timeout:(.+)"), + s -> { + Preconditions.checkNotNull(s); + + int value; + try { + value = Integer.parseInt(s); + } catch (NumberFormatException e) { + return "can't be parsed as an integer"; + } + + // De-and-reserialize & compare to only allow canonical integer formats. + if (!Integer.toString(value).equals(s)) { + return "must be in canonical format (e.g. '4' instead of '+04')"; + } + + if (value < 1) { + return "can't be zero or negative"; + } + + return null; + }); /** If an action would not successfully run other than on Darwin. */ public static final String REQUIRES_DARWIN = "requires-darwin"; diff --git a/src/main/java/com/google/devtools/build/lib/actions/Spawns.java b/src/main/java/com/google/devtools/build/lib/actions/Spawns.java index 428075539e5b41..34e5c11f69a6b3 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Spawns.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Spawns.java @@ -14,9 +14,12 @@ package com.google.devtools.build.lib.actions; +import com.google.devtools.build.lib.actions.ExecutionRequirements.ParseableRequirement.ValidationException; +import com.google.devtools.build.lib.actions.ExecutionRequirements.ParseableRequirement; import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code; + import java.io.IOException; import java.time.Duration; import java.util.Map; @@ -152,21 +155,72 @@ public static Duration getTimeout(Spawn spawn) throws ExecException { * defaultTimeout, or {@code Duration.ZERO} if that is null. */ public static Duration getTimeout(Spawn spawn, Duration defaultTimeout) throws ExecException { - String timeoutStr = spawn.getExecutionInfo().get(ExecutionRequirements.TIMEOUT); - if (timeoutStr == null) { - return defaultTimeout == null ? Duration.ZERO : defaultTimeout; + Duration timeout = null; + + for (Map.Entry entry: spawn.getExecutionInfo().entrySet()) { + String value = getTimeoutValueFromExecutionInfoKeyOrValue(spawn,entry); + if (value != null) { + if (timeout != null) { + throw new UserExecException( + FailureDetail.newBuilder() + .setMessage("multiple timeout values specified") + .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.DUPLICATE_TIMEOUT_TAGS)) + .build()); + } + timeout = parseTimeoutValue(value); + } + } + + if (timeout == null) { + timeout = defaultTimeout == null ? Duration.ZERO : defaultTimeout; } + return timeout; +} + +/** + * Retrieves the timeout value from the execution info key or value. + * Timeout may either be specified as a tag on format timeout: + * or as a key-value pair when specified as an execution_requirement. + */ +private static String getTimeoutValueFromExecutionInfoKeyOrValue(Spawn spawn, Map.Entry entry) throws ExecException { + ParseableRequirement requirement = ExecutionRequirements.TIMEOUT; + String value = null; + + if (entry.getKey().equals("timeout")) { + value = entry.getValue(); + } else { + try { + value = requirement.parseIfMatches(entry.getKey()); + } catch (ValidationException e) { + throw new UserExecException( + e, + FailureDetail.newBuilder() + .setMessage( + String.format( + "%s has a '%s' tag, but its value '%s' didn't pass validation: %s", + spawn.getTargetLabel(), requirement.userFriendlyName(), e.getTagValue(), e.getMessage())) + .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.INVALID_TIMEOUT_TAG)) + .build()); + } + } + return value; +} + +/** + * Parses a timeout value from a string and returns it as a {@link Duration}. + */ +private static Duration parseTimeoutValue(String value) throws ExecException { try { - return Duration.ofSeconds(Integer.parseInt(timeoutStr)); + return Duration.ofSeconds(Integer.parseInt(value)); } catch (NumberFormatException e) { - throw new UserExecException( - e, - FailureDetail.newBuilder() - .setMessage("could not parse timeout") - .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.INVALID_TIMEOUT)) - .build()); + throw new UserExecException( + e, + FailureDetail.newBuilder() + .setMessage("could not parse timeout") + .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.INVALID_TIMEOUT)) + .build()); } - } +} /** * Returns whether a local {@link Spawn} runner implementation should prefetch the inputs before diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index 4d2c212477e87f..61fa77c89ee396 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java @@ -119,7 +119,7 @@ public TestRunnerSpawn createTestRunnerSpawn( // for this. executionInfo.put(ExecutionRequirements.NO_CACHE, ""); } - executionInfo.put(ExecutionRequirements.TIMEOUT, "" + getTimeout(action).toSeconds()); + executionInfo.put("timeout", "" + getTimeout(action).toSeconds()); SimpleSpawn.LocalResourcesSupplier localResourcesSupplier = () -> diff --git a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java index c08ef7ce5e5aa7..926c4e06cf5c36 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java +++ b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java @@ -58,7 +58,7 @@ private static boolean legalExecInfoKeys(String tag) { || tag.startsWith("disable-") || tag.startsWith("cpu:") || tag.equals(ExecutionRequirements.LOCAL) - || tag.equals(ExecutionRequirements.TIMEOUT) + || tag.startsWith("timeout") || tag.equals(ExecutionRequirements.WORKER_KEY_MNEMONIC) || tag.startsWith("resources:"); } diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto index 979f8cb717137f..91b519b6da8488 100644 --- a/src/main/protobuf/failure_details.proto +++ b/src/main/protobuf/failure_details.proto @@ -222,6 +222,8 @@ message Spawn { COMMAND_LINE_EXPANSION_FAILURE = 7 [(metadata) = { exit_code: 1 }]; EXEC_IO_EXCEPTION = 8 [(metadata) = { exit_code: 36 }]; INVALID_TIMEOUT = 9 [(metadata) = { exit_code: 1 }]; + DUPLICATE_TIMEOUT_TAGS = 16 [(metadata) = { exit_code: 1 }]; + INVALID_TIMEOUT_TAG = 17 [(metadata) = { exit_code: 1 }]; INVALID_REMOTE_EXECUTION_PROPERTIES = 10 [(metadata) = { exit_code: 1 }]; NO_USABLE_STRATEGY_FOUND = 11 [(metadata) = { exit_code: 1 }]; // TODO(b/138456686): this code should be deprecated when SpawnResult is diff --git a/src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java b/src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java index 5352474f57b985..9e752b6366cb05 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java @@ -223,6 +223,19 @@ public void testExecutionInfo_withPrefixCpu() throws Exception { assertThat(execInfo).containsExactly("cpu:123", ""); } + @Test + public void testExecutionInfo_withPrefixTimeout() throws Exception { + scratch.file( + "tests/BUILD", + "load('//test_defs:foo_binary.bzl', 'foo_binary')", + "foo_binary(name = 'with-prefix-timeout', srcs=['sh.sh'], tags=['timeout:123', 'wrong-tag'])"); + + Rule withCpuPrefix = (Rule) getTarget("//tests:with-prefix-timeout"); + + Map execInfo = TargetUtils.getExecutionInfo(withCpuPrefix); + assertThat(execInfo).containsExactly("timeout:123", ""); + } + @Test public void testExecutionInfo_withLocalTag() throws Exception { scratch.file(