chore: set default timeouts #4066

vvagaytsev · 2023-04-11T09:07:34Z

What this PR does / why we need it:
Open questions:

Should we increase the default timeouts for all actions? Currently, we have 20 min for Builds, 10 min for Runs and Tests, and no limits for Deploys (there are some configurable ones for exec Deploys). -> This can be discussed in the code review round.
For Deploys, it also might make sense to introduce some timeouts. -> discussion is in https://discord.com/channels/817392104711651328/1088795679159222292/1095340640314855554 Many (but not all) Deploy have the timeout in the action type-specific spec field. We might need to pull it up to the root config level like in Builds, Runs, and Deploys. That will be done in a separate PR.
~~It might be a good idea to emit a warning message about the default action timeouts on Garden startup.~~

Which issue(s) this PR fixes:

Special notes for your reviewer:
See individual commits for details. Squash the commits before the merge.

vvagaytsev · 2023-04-11T13:31:06Z

Let's have a pre-review round @thsig @edvald @Orzelius @stefreak
I've prepared a list of questions to be discussed in the PR description.

Orzelius · 2023-04-11T14:01:50Z

my opinions

Should we increase the default timeouts for all actions?

why? I think these are plenty long enough

For Deploys, it also might make sense to introduce some timeouts.

I'm thinking between 3 to 5 min, don't see how a normal deploy could take longer than that

It might be a good idea to emit a warning message about the default action timeouts on Garden startup.

Nah, I don't think so. User's will see the timeout if they ever run into it and most probably won't unless they're doing something wrong or very special

vvagaytsev · 2023-04-12T07:51:24Z

@Orzelius thanks for the feedback!

I've been thinking about this again and have realized that warning messages can be too annoying for users. It's better to emphasize that all actions have timeouts in the release notes. Anyway, it is a breaking change and will be announced accordingly.

On the default values, I'm not sure about deployments. The deployment time depends on the deployment policy and the number of app instances to be deployed.

Theoretically, every action can take a long time. But, of course, the default values should be something sane :)
Maybe keep the current values as they are, and make Deploy timeout the same as Run/Test one, i.e. 10 min.

Update. We do have timeouts for Deploy action, but not for all types of those. The timeout settings of Deploy actions reside in the type-specific spec field.

vvagaytsev · 2023-04-12T12:16:37Z

core/src/plugins/exec/config.ts

@@ -97,7 +100,7 @@ export interface ExecDeployActionSpec extends CommonKeys {
  cleanupCommand?: string[]
  deployCommand: string[]
  statusCommand?: string[]
-  timeout?: number
+  timeout: number


@edvald it looks like this value is never used. Removing this and its initialization in the converter brings no compilation errors.

The timeout value is never passed to the runPersistent function which, in turn, does not seem to support timeouts.

There is external logic on the runPersistent caller's side that relies on the statusTimeout and statusCommand. Do we still need this timeout field?

vvagaytsev · 2023-04-25T13:22:00Z

There are some test failures, I'll fix those soon.

thsig

Looks good to me! Mostly had one comment there.

thsig · 2023-04-28T11:47:16Z

core/src/plugins/kubernetes/container/run.ts

@@ -32,6 +33,7 @@ export const k8sContainerRun: RunActionHandler<"run", ContainerRunAction> = asyn
    dropCapabilities,
  } = action.getSpec()

+  const timeout = action.getConfig("timeout") || DEFAULT_RUN_TIMEOUT_SEC


Isn't the || DEFAULT_RUN_TIMEOUT_SEC part unnecessary, since the timeout value is always set on the action?

The same question applies elsewhere in this PR where the default value is used as a fallback in plugin code (where it shouldn't be necessary, i.e. the default value should already be set on the action).

@thsig, yes, that's correct. We can safely declare the timeout field as non-optional in BuildActionConfig, RunActionConfig, and TestActionConfig.

it would be also nice to pull that field to the base class BaseActionConfig, and make it mandatory in DeployActionConfig too.

core/src/constants.ts

core/src/plugin/base.ts

core/test/unit/src/garden.ts

Walther

Looks good to me as far as I can tell, good work! 🎉

Constant `DEFAULT_BUILD_TIMEOUT` was used only in tests, and was a duplicate of `DEFAULT_BUILD_TIMEOUT_SEC`.

The version was changed because the `timeout` field was set in the config.

Now timeout is always defined in the root-level config and in each run/task spec.

Use the same set pattern as in the Test action. The non-null timeout value is set in the downstream functions, so there is no harm. All action are planned to have non-null timeout, so the default value here will be omitted in the later commits.

It's always set in the caller-side functions.

Non-optional value is always set from outside.

vvagaytsev force-pushed the 0.13-timeouts branch 2 times, most recently from cd0c584 to 0760794 Compare April 11, 2023 12:01

vvagaytsev changed the title ~~0.13 timeouts~~ chore: set default timeouts Apr 11, 2023

vvagaytsev added the 0.13 label Apr 11, 2023

vvagaytsev commented Apr 12, 2023

View reviewed changes

vvagaytsev requested review from edvald and thsig April 13, 2023 10:44

vvagaytsev marked this pull request as ready for review April 13, 2023 10:44

vvagaytsev requested review from Walther, eysi09 and Orzelius April 25, 2023 12:20

vvagaytsev force-pushed the 0.13-timeouts branch from da5817f to 0e32ab5 Compare April 25, 2023 13:24

thsig reviewed Apr 28, 2023

View reviewed changes

vvagaytsev force-pushed the 0.13-timeouts branch 5 times, most recently from c2401c1 to 35a343e Compare May 5, 2023 09:43

vvagaytsev force-pushed the 0.13-timeouts branch 2 times, most recently from 9bb240c to 209d501 Compare May 10, 2023 10:11

Walther reviewed May 11, 2023

View reviewed changes

core/src/constants.ts Outdated Show resolved Hide resolved

Walther reviewed May 11, 2023

View reviewed changes

core/src/plugin/base.ts Outdated Show resolved Hide resolved

Walther reviewed May 11, 2023

View reviewed changes

core/test/unit/src/garden.ts Outdated Show resolved Hide resolved

Walther previously approved these changes May 11, 2023

View reviewed changes

vvagaytsev marked this pull request as draft May 11, 2023 11:52

vvagaytsev dismissed Walther’s stale review via 27b81a0 May 11, 2023 13:19

vvagaytsev added 22 commits May 11, 2023 23:45

refactor: extract named constant

03ac5ea

chore: default timeout value for module-based tasks and tests

cac6955

chore: set min timeout value for module builds

d754f43

refactor: non-nullable timeout in BaseTestSpec

41caeb7

refactor: non-nullable timeout in BaseTaskSpec

f35b1b4

chore: replace DEFAULT_BUILD_TIMEOUT by DEFAULT_BUILD_TIMEOUT_SEC

f375f77

Constant `DEFAULT_BUILD_TIMEOUT` was used only in tests, and was a duplicate of `DEFAULT_BUILD_TIMEOUT_SEC`.

test: fix module version assertions

71979cd

The version was changed because the `timeout` field was set in the config.

test: fix test assertions

87efd53

Now timeout is always defined in the root-level config and in each run/task spec.

chore: require integer as a k8s deployment timeout

dd916c7

refactor: non-nullable timeout in BaseBuildSpec

9869b0e

style: linting

e5e9266

chore: non-optional timeout in BaseRunParams

93af1ac

It's always set in the caller-side functions.

chore: added some TODOs

4cffa52

chore: set default timeout for exec service

8665c28

chore: set default timeout for exec Deploy action

72208a2

refactor: make timeout nonnullable in KubernetesTypeCommonDeploySpec

e032d4f

refactor: add optional timeout field in BaseActionConfig

243a4a6

chore: fix typo

61e9f50

refactor: shrink default build timeout to 600 sec

0613f4c

test: fix test assertions

e0f8515

chore: make timeout required in runBaseParams

f7c1cbf

Non-optional value is always set from outside.

vvagaytsev dismissed thsig’s stale review via f7c1cbf May 11, 2023 21:56

vvagaytsev force-pushed the 0.13-timeouts branch from 2dc878b to f7c1cbf Compare May 11, 2023 21:56

vvagaytsev requested a review from thsig May 11, 2023 22:07

thsig approved these changes May 12, 2023

View reviewed changes

vvagaytsev merged commit e55a595 into 0.13 May 12, 2023
34 checks passed

vvagaytsev deleted the 0.13-timeouts branch May 12, 2023 09:09

vvagaytsev mentioned this pull request May 12, 2023

Set non-null default values for all timeout fields, and require a number #3516

Closed

stefreak mentioned this pull request May 15, 2023

0.13 rootless containers #4273

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: set default timeouts #4066

chore: set default timeouts #4066

vvagaytsev commented Apr 11, 2023 •

edited

vvagaytsev commented Apr 11, 2023

Orzelius commented Apr 11, 2023

vvagaytsev commented Apr 12, 2023 •

edited

vvagaytsev Apr 12, 2023

vvagaytsev commented Apr 25, 2023

thsig left a comment

thsig Apr 28, 2023 •

edited

vvagaytsev May 2, 2023

Walther left a comment

chore: set default timeouts #4066

chore: set default timeouts #4066

Conversation

vvagaytsev commented Apr 11, 2023 • edited

vvagaytsev commented Apr 11, 2023

Orzelius commented Apr 11, 2023

my opinions

vvagaytsev commented Apr 12, 2023 • edited

vvagaytsev Apr 12, 2023

Choose a reason for hiding this comment

vvagaytsev commented Apr 25, 2023

thsig left a comment

Choose a reason for hiding this comment

thsig Apr 28, 2023 • edited

Choose a reason for hiding this comment

vvagaytsev May 2, 2023

Choose a reason for hiding this comment

Walther left a comment

Choose a reason for hiding this comment

vvagaytsev commented Apr 11, 2023 •

edited

vvagaytsev commented Apr 12, 2023 •

edited

thsig Apr 28, 2023 •

edited