Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: set default timeouts #4066

Merged
merged 30 commits into from May 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
7a08e4c
refactor: move constant `defaultBuildTimeout` to `constants.ts`
vvagaytsev Apr 11, 2023
b10a595
refactor: rename timeout constants for clarity
vvagaytsev Apr 11, 2023
a9afc8a
chore: set default build action timeout in joi schema
vvagaytsev Apr 11, 2023
9c8dc11
chore: set default run action timeout in joi schema
vvagaytsev Apr 11, 2023
3df5562
chore: set default test action timeout in joi schema
vvagaytsev Apr 11, 2023
8c81431
chore: change default run and test action timeouts to 600s
vvagaytsev Apr 11, 2023
0b99f31
chore: remove duplicate constant
vvagaytsev Apr 11, 2023
2e3f470
docs: re-generated docs
vvagaytsev Apr 11, 2023
03ac5ea
refactor: extract named constant
vvagaytsev Apr 11, 2023
cac6955
chore: default timeout value for module-based tasks and tests
vvagaytsev Apr 11, 2023
d754f43
chore: set min timeout value for module builds
vvagaytsev Apr 11, 2023
41caeb7
refactor: non-nullable timeout in `BaseTestSpec`
vvagaytsev Apr 11, 2023
f35b1b4
refactor: non-nullable timeout in `BaseTaskSpec`
vvagaytsev Apr 11, 2023
f375f77
chore: replace `DEFAULT_BUILD_TIMEOUT` by `DEFAULT_BUILD_TIMEOUT_SEC`
vvagaytsev Apr 11, 2023
71979cd
test: fix module version assertions
vvagaytsev Apr 11, 2023
87efd53
test: fix test assertions
vvagaytsev Apr 11, 2023
dd916c7
chore: require integer as a k8s deployment timeout
vvagaytsev Apr 11, 2023
9869b0e
refactor: non-nullable timeout in `BaseBuildSpec`
vvagaytsev Apr 11, 2023
e5e9266
style: linting
vvagaytsev Apr 11, 2023
4a80859
chore: use default Run action timeout (similarly to Test action)
vvagaytsev Apr 11, 2023
93af1ac
chore: non-optional timeout in `BaseRunParams`
vvagaytsev Apr 11, 2023
4cffa52
chore: added some TODOs
vvagaytsev Apr 11, 2023
8665c28
chore: set default timeout for exec service
vvagaytsev Apr 12, 2023
72208a2
chore: set default timeout for exec Deploy action
vvagaytsev Apr 12, 2023
e032d4f
refactor: make `timeout` nonnullable in `KubernetesTypeCommonDeploySpec`
vvagaytsev May 2, 2023
243a4a6
refactor: add optional `timeout` field in `BaseActionConfig`
vvagaytsev May 2, 2023
61e9f50
chore: fix typo
vvagaytsev May 9, 2023
0613f4c
refactor: shrink default build timeout to 600 sec
vvagaytsev May 11, 2023
e0f8515
test: fix test assertions
vvagaytsev May 11, 2023
f7c1cbf
chore: make `timeout` required in `runBaseParams`
vvagaytsev May 11, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion core/src/actions/build.ts
Expand Up @@ -38,6 +38,7 @@ import {
import { ResolvedConfigGraph } from "../graph/config-graph"
import { ActionVersion } from "../vcs/vcs"
import { Memoize } from "typescript-memoize"
import { DEFAULT_BUILD_TIMEOUT_SEC } from "../constants"

export interface BuildCopyFrom {
build: string
Expand All @@ -51,7 +52,6 @@ export interface BuildActionConfig<T extends string = string, S extends object =
allowPublish?: boolean
buildAtSource?: boolean
copyFrom?: BuildCopyFrom[]
timeout?: number
}

export const copyFromSchema = createSchema({
Expand Down Expand Up @@ -136,6 +136,8 @@ export const buildActionConfigSchema = () =>
timeout: joi
.number()
.integer()
.min(1)
.default(DEFAULT_BUILD_TIMEOUT_SEC)
.description("Set a timeout for the build to complete, in seconds.")
.meta({ templateContext: ActionConfigContext }),
})
Expand Down Expand Up @@ -203,6 +205,7 @@ export class ResolvedBuildAction<
this._config.spec = params.spec
this._config.internal.inputs = params.inputs
}

getExecutedDependencies() {
return this.executedDependencies
}
Expand Down
12 changes: 8 additions & 4 deletions core/src/actions/run.ts
Expand Up @@ -16,16 +16,20 @@ import {
RuntimeAction,
} from "./base"
import { Action, BaseActionConfig } from "./types"
import { DEFAULT_RUN_TIMEOUT_SEC } from "../constants"

export interface RunActionConfig<N extends string = any, S extends object = any>
extends BaseRuntimeActionConfig<"Run", N, S> {
timeout?: number
}
extends BaseRuntimeActionConfig<"Run", N, S> {}

export const runActionConfigSchema = memoize(() =>
baseRuntimeActionConfigSchema().keys({
kind: joi.string().allow("Run").only(),
timeout: joi.number().integer().description("Set a timeout for the run to complete, in seconds."),
timeout: joi
.number()
.integer()
.min(1)
.default(DEFAULT_RUN_TIMEOUT_SEC)
.description("Set a timeout for the run to complete, in seconds."),
})
)

Expand Down
12 changes: 8 additions & 4 deletions core/src/actions/test.ts
Expand Up @@ -16,16 +16,20 @@ import {
RuntimeAction,
} from "./base"
import { Action, BaseActionConfig } from "./types"
import { DEFAULT_TEST_TIMEOUT_SEC } from "../constants"

export interface TestActionConfig<N extends string = any, S extends object = any>
extends BaseRuntimeActionConfig<"Test", N, S> {
timeout?: number
}
extends BaseRuntimeActionConfig<"Test", N, S> {}

export const testActionConfigSchema = memoize(() =>
baseRuntimeActionConfigSchema().keys({
kind: joi.string().allow("Test").only(),
timeout: joi.number().integer().description("Set a timeout for the test to complete, in seconds."),
timeout: joi
.number()
.integer()
.min(1)
.default(DEFAULT_TEST_TIMEOUT_SEC)
.description("Set a timeout for the test to complete, in seconds."),
})
)

Expand Down
2 changes: 2 additions & 0 deletions core/src/actions/types.ts
Expand Up @@ -79,6 +79,8 @@ export interface BaseActionConfig<K extends ActionKind = ActionKind, T = string,
include?: string[]
exclude?: string[]

timeout?: number

// Variables
// -> Templating with ActionConfigContext allowed
variables?: DeepPrimitiveMap
Expand Down
3 changes: 2 additions & 1 deletion core/src/config/base.ts
Expand Up @@ -14,7 +14,7 @@ import { pathExists, readFile } from "fs-extra"
import { omit, isPlainObject, isArray } from "lodash"
import { coreModuleSpecSchema, baseModuleSchemaKeys, BuildDependencyConfig, ModuleConfig } from "./module"
import { ConfigurationError, FilesystemError, ParameterError } from "../exceptions"
import { DEFAULT_API_VERSION, PREVIOUS_API_VERSION } from "../constants"
import { DEFAULT_API_VERSION, DEFAULT_BUILD_TIMEOUT_SEC, PREVIOUS_API_VERSION } from "../constants"
import { ProjectConfig, ProjectResource } from "../config/project"
import { validateWithPath } from "./validation"
import { defaultDotIgnoreFile, listDirectory } from "../util/fs"
Expand Down Expand Up @@ -379,6 +379,7 @@ export function prepareModuleResource(spec: any, configPath: string, projectRoot
allowPublish: spec.allowPublish,
build: {
dependencies,
timeout: spec.build?.timeout || DEFAULT_BUILD_TIMEOUT_SEC,
},
configPath,
description: spec.description,
Expand Down
20 changes: 10 additions & 10 deletions core/src/config/module.ts
Expand Up @@ -9,24 +9,23 @@
import { memoize } from "lodash"
import { ServiceConfig, serviceConfigSchema } from "./service"
import {
apiVersionSchema,
createSchema,
DeepPrimitiveMap,
includeGuideLink,
joi,
joiArray,
joiIdentifier,
joiRepositoryUrl,
joiSparseArray,
joiUserIdentifier,
joi,
includeGuideLink,
apiVersionSchema,
DeepPrimitiveMap,
joiVariables,
joiSparseArray,
createSchema,
} from "./common"
import { TestConfig, testConfigSchema } from "./test"
import { TaskConfig, taskConfigSchema } from "./task"
import { dedent, stableStringify } from "../util/string"
import { configTemplateKind, varfileDescription } from "./base"

export const defaultBuildTimeout = 1200
import { DEFAULT_BUILD_TIMEOUT_SEC } from "../constants"

interface BuildCopySpec {
source: string
Expand Down Expand Up @@ -71,7 +70,7 @@ export const buildDependencySchema = createSchema({

export interface BaseBuildSpec {
dependencies: BuildDependencyConfig[]
timeout?: number
timeout: number
}

export interface GenerateFileSpec {
Expand Down Expand Up @@ -157,7 +156,8 @@ export const baseBuildSpecSchema = createSchema({
timeout: joi
.number()
.integer()
.default(defaultBuildTimeout)
.min(1)
.default(DEFAULT_BUILD_TIMEOUT_SEC)
.description("Maximum time in seconds to wait for build to finish."),
}),
default: () => ({ dependencies: [] }),
Expand Down
9 changes: 5 additions & 4 deletions core/src/config/task.ts
Expand Up @@ -9,6 +9,7 @@
import { joiUserIdentifier, joi, joiSparseArray, createSchema } from "./common"
import { deline, dedent } from "../util/string"
import { memoize } from "lodash"
import { DEFAULT_RUN_TIMEOUT_SEC } from "../constants"

export interface TaskSpec {}

Expand All @@ -17,7 +18,7 @@ export interface BaseTaskSpec extends TaskSpec {
dependencies: string[]
description?: string
disabled: boolean
timeout: number | null
timeout: number
}

export const cacheResultSchema = memoize(() =>
Expand Down Expand Up @@ -54,9 +55,9 @@ export const baseTaskSpecSchema = createSchema({
),
timeout: joi
.number()
.optional()
.allow(null)
.default(null)
.integer()
.min(1)
.default(DEFAULT_RUN_TIMEOUT_SEC)
.description("Maximum duration (in seconds) of the task's execution."),
}),
})
Expand Down
10 changes: 8 additions & 2 deletions core/src/config/test.ts
Expand Up @@ -8,12 +8,13 @@

import { joiUserIdentifier, joi, joiSparseArray, createSchema } from "./common"
import { deline, dedent } from "../util/string"
import { DEFAULT_TEST_TIMEOUT_SEC } from "../constants"

export interface BaseTestSpec {
name: string
dependencies: string[]
disabled: boolean
timeout: number | null
timeout: number
}

export const baseTestSpecSchema = createSchema({
Expand All @@ -35,7 +36,12 @@ export const baseTestSpecSchema = createSchema({
specific environments, e.g. only during CI.
`
),
timeout: joi.number().allow(null).default(null).description("Maximum duration (in seconds) of the test run."),
timeout: joi
.number()
.integer()
.min(1)
.default(DEFAULT_TEST_TIMEOUT_SEC)
.description("Maximum duration (in seconds) of the test run."),
}),
})

Expand Down
7 changes: 4 additions & 3 deletions core/src/constants.ts
Expand Up @@ -7,7 +7,7 @@
*/

import env from "env-var"
import { resolve, join } from "path"
import { join, resolve } from "path"
import { homedir } from "os"

export const isPkg = !!(<any>process).pkg
Expand All @@ -26,8 +26,9 @@ export const DEFAULT_PORT_PROTOCOL = "TCP"
export const PREVIOUS_API_VERSION = "garden.io/v0"
export const DEFAULT_API_VERSION = "garden.io/v1"

export const DEFAULT_TEST_TIMEOUT = 60 * 1000
export const DEFAULT_RUN_TIMEOUT = 60 * 1000
export const DEFAULT_BUILD_TIMEOUT_SEC = 600
export const DEFAULT_TEST_TIMEOUT_SEC = 600
export const DEFAULT_RUN_TIMEOUT_SEC = 600

export type SupportedPlatform = "linux" | "darwin" | "win32"
export const SUPPORTED_PLATFORMS: SupportedPlatform[] = ["linux", "darwin", "win32"]
Expand Down
2 changes: 1 addition & 1 deletion core/src/logger/log-entry.ts
Expand Up @@ -455,7 +455,7 @@ export class ActionLog extends Log<ActionLogContext> {
showDuration = true

/**
* Create a new ActioLog instance. The new instance inherits the parent context.
* Create a new ActionLog instance. The new instance inherits the parent context.
*/
createLog(params: CreateLogParams = {}): ActionLog {
return new ActionLog(this.makeLogConfig(params))
Expand Down
2 changes: 1 addition & 1 deletion core/src/plugin/base.ts
Expand Up @@ -114,7 +114,7 @@ export const moduleActionParamsSchema = () =>
export const runBaseParams = () => ({
interactive: joi.boolean().description("Whether to run interactively (i.e. attach to the terminal)."),
silent: joi.boolean().description("Set to false if the output should not be logged to the console."),
timeout: joi.number().optional().description("If set, how long to run the command before timing out."),
timeout: joi.number().required().description("If set, how long to run the command before timing out."),
})

// TODO-0.13.0: update this schema in 0.13.0
Expand Down
2 changes: 1 addition & 1 deletion core/src/plugin/handlers/base/base.ts
Expand Up @@ -55,5 +55,5 @@ export interface BaseRunParams {
command?: string[]
args: string[]
interactive: boolean
timeout?: number
timeout: number
}
4 changes: 2 additions & 2 deletions core/src/plugins/container/container.ts
Expand Up @@ -252,7 +252,7 @@ function convertContainerModuleRuntimeActions(
disabled: task.disabled,
build: buildAction?.name,
dependencies: prepareRuntimeDependencies(task.spec.dependencies, buildAction),
timeout: task.spec.timeout || undefined,
timeout: task.spec.timeout,

spec: {
...omit(task.spec, ["name", "dependencies", "disabled", "timeout"]),
Expand All @@ -271,7 +271,7 @@ function convertContainerModuleRuntimeActions(
disabled: test.disabled,
build: buildAction?.name,
dependencies: prepareRuntimeDependencies(test.spec.dependencies, buildAction),
timeout: test.spec.timeout ? test.spec.timeout : undefined,
timeout: test.spec.timeout,

spec: {
...omit(test.spec, ["name", "dependencies", "disabled", "timeout"]),
Expand Down
2 changes: 0 additions & 2 deletions core/src/plugins/container/helpers.ts
Expand Up @@ -40,8 +40,6 @@ interface DockerVersion {
server?: string
}

export const DEFAULT_BUILD_TIMEOUT = 600

export const minDockerVersion: DockerVersion = {
client: "19.03.0",
server: "17.07.0",
Expand Down
9 changes: 6 additions & 3 deletions core/src/plugins/exec/config.ts
Expand Up @@ -22,6 +22,7 @@ import {
} from "../../config/common"
import { ArtifactSpec } from "../../config/validation"
import { dedent } from "../../util/string"
import { DEFAULT_RUN_TIMEOUT_SEC } from "../../constants"

const execPathDoc = dedent`
Note that if a Build is referenced in the \`build\` field, the command will be run from the build directory for that Build action. If that Build has \`buildAtSource: true\` set, the command will be run from the source directory of the Build action. If no \`build\` reference is set, the command is run from the source directory of this action.
Expand Down Expand Up @@ -53,13 +54,15 @@ const execCommonSchema = createSchema({
`),
}),
})

// BUILD //

export interface ExecBuildActionSpec extends CommonKeys {
command?: string[] // This needs to be optional to support "dummy" builds
timeout?: number
env: StringMap
}

export type ExecBuildConfig = BuildActionConfig<"exec", ExecBuildActionSpec>
export type ExecBuild = BuildAction<ExecBuildConfig, ExecOutputs>

Expand Down Expand Up @@ -97,7 +100,7 @@ export interface ExecDeployActionSpec extends CommonKeys {
cleanupCommand?: string[]
deployCommand: string[]
statusCommand?: string[]
timeout?: number
timeout: number
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

statusTimeout: number
env: StringMap
}
Expand Down Expand Up @@ -159,8 +162,7 @@ export const execDeployActionSchema = createSchema({
${execPathDoc}
`
),
// TODO: Set a default in v0.13.
timeout: joi.number().description(dedent`
timeout: joi.number().integer().min(1).default(DEFAULT_RUN_TIMEOUT_SEC).description(dedent`
The maximum duration (in seconds) to wait for \`deployCommand\` to exit. Ignored if \`persistent: false\`.
`),
statusTimeout: joi.number().default(defaultStatusTimeout).description(dedent`
Expand Down Expand Up @@ -204,6 +206,7 @@ export const execRunActionSchema = createSchema({
// TEST //

export interface ExecTestActionSpec extends ExecRunActionSpec {}

export type ExecTestConfig = TestActionConfig<"exec", ExecTestActionSpec>
export type ExecTest = TestAction<ExecTestConfig, ExecOutputs>

Expand Down
4 changes: 2 additions & 2 deletions core/src/plugins/exec/convert.ts
Expand Up @@ -126,7 +126,7 @@ export async function convertExecModule(params: ConvertModuleParams<ExecModule>)
disabled: task.disabled,
build: buildAction ? buildAction.name : undefined,
dependencies: prepRuntimeDeps(task.spec.dependencies),
timeout: task.spec.timeout ? task.spec.timeout : undefined,
timeout: task.spec.timeout,

spec: {
shell: true, // This keeps the old pre-0.13 behavior
Expand All @@ -147,7 +147,7 @@ export async function convertExecModule(params: ConvertModuleParams<ExecModule>)
disabled: test.disabled,
build: buildAction ? buildAction.name : undefined,
dependencies: prepRuntimeDeps(test.spec.dependencies),
timeout: test.spec.timeout ? test.spec.timeout : undefined,
timeout: test.spec.timeout,

spec: {
shell: true, // This keeps the old pre-0.13 behavior
Expand Down
6 changes: 3 additions & 3 deletions core/src/plugins/exec/moduleConfig.ts
Expand Up @@ -23,6 +23,7 @@ import { artifactsSchema, ExecSyncModeSpec } from "./config"
import { ConfigureModuleParams, ConfigureModuleResult } from "../../plugin/handlers/Module/configure"
import { ConfigurationError } from "../../exceptions"
import { omit } from "lodash"
import { DEFAULT_RUN_TIMEOUT_SEC } from "../../constants"

const execPathDoc = dedent`
By default, the command is run inside the Garden build directory (under .garden/build/<module-name>).
Expand Down Expand Up @@ -87,7 +88,7 @@ export interface ExecServiceSpec extends CommonServiceSpec {
deployCommand: string[]
statusCommand?: string[]
syncMode?: ExecSyncModeSpec
timeout?: number
timeout: number
env: { [key: string]: string }
}

Expand Down Expand Up @@ -130,8 +131,7 @@ export const execServiceSchema = () =>
${execPathDoc}
`
),
// TODO: Set a default in v0.13.
timeout: joi.number().description(dedent`
timeout: joi.number().integer().min(1).default(DEFAULT_RUN_TIMEOUT_SEC).description(dedent`
The maximum duration (in seconds) to wait for a local script to exit.
`),
env: joiEnvVars().description("Environment variables to set when running the deploy and status commands."),
Expand Down