Repository navigation
Conversation
The container and exec function runtimes accept a timeout, but nothing set it, so every function ran with the default of 5 minutes. Add a --timeout flag to both commands and pass it to the runtimes. A function that times out is now reported with its stderr and a 'Timed out after' line, and kpt no longer waits past the timeout for a child process that keeps the output open. Part of kptdev#4473 Assisted-by: Claude:claude-opus-5-5 Assisted-by: Codex:gpt-6.1-sol Signed-off-by: Nikshay <nikshayyadav90@gmail.com>
✅ Deploy Preview for kptdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
🟡 Changes recommended
Timed-out exec functions can leave descendant processes running, and the container timeout path lacks execution coverage.
3 open findings
What changed in this PR
Adds configurable per-function timeouts to kpt fn render and kpt fn eval.
Changes:
- Adds and validates the
--timeoutflag. - Propagates timeouts to exec and container runtimes.
- Reports timeout failures and adds documentation/tests.
| File | Description |
|---|---|
thirdparty/kyaml/runfn/runfn.go |
Propagates eval timeouts. |
thirdparty/kyaml/runfn/runfn_test.go |
Tests eval exec timeout behavior. |
thirdparty/cmdconfig/commands/cmdeval/cmdeval.go |
Adds the eval flag and validation. |
thirdparty/cmdconfig/commands/cmdeval/cmdeval_test.go |
Tests eval flag parsing. |
pkg/lib/runneroptions/runneroptions.go |
Adds shared timeout configuration. |
pkg/fn/runtime/runner.go |
Propagates and reports render timeouts. |
pkg/fn/runtime/runner_test.go |
Tests render timeout propagation. |
pkg/fn/runtime/fnerrors.go |
Adds timeout details to execution errors. |
pkg/fn/runtime/fnerrors_test.go |
Tests timeout error formatting. |
pkg/fn/runtime/exec.go |
Implements exec timeout handling. |
pkg/fn/runtime/exec_test.go |
Tests exec timeout scenarios. |
pkg/fn/runtime/container.go |
Reports container timeout failures. |
internal/docs/generated/fndocs/docs.go |
Updates generated CLI documentation. |
documentation/content/en/reference/cli/fn/render/_index.md |
Documents render timeouts. |
documentation/content/en/reference/cli/fn/eval/_index.md |
Documents eval timeouts. |
commands/fn/render/cmdrender.go |
Adds the render flag and validation. |
commands/fn/render/cmdrender_test.go |
Tests render flag parsing. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Nikshay <nikshayyadav90@gmail.com>
Signed-off-by: Nikshay <nikshayyadav90@gmail.com> Assisted-by: Claude:claude-opus-5-5
Can you try to resolve the high/medium issues? I think it's pretty easy to do. |
Signed-off-by: Nikshay <nikshayyadav90@gmail.com> Assisted-by: Claude:claude-opus-5-5
|
@liamfallon done in 4e1534c:
The three remaining findings are the Minor |
liamfallon
left a comment
There was a problem hiding this comment.
Thanks very much for this, it's terrific. A few rather pedantic comments.
| case fe.Timeout == 0: | ||
| return "" | ||
| case fe.RunTimeout: | ||
| return fmt.Sprintf("the whole run took longer than %v", fe.Timeout) |
There was a problem hiding this comment.
| return fmt.Sprintf("the whole run took longer than %v", fe.Timeout) | |
| return fmt.Sprintf("pipeline execution timeout of %v exceeded", fe.Timeout) |
I think it's better to explicitly mention that there is a timeout and that it has been exceeded.
There was a problem hiding this comment.
Done in ba5dc16. The whole-run case now says pipeline execution timeout of X exceeded. For the per-function case (fn eval, or the 5m default per function) I used function execution timeout of X exceeded, because "pipeline" would be wrong there. Happy to change it if you'd prefer one message for both. The test expectations you pointed at are updated too.
| case fe.RunTimeout: | ||
| return fmt.Sprintf("the whole run took longer than %v", fe.Timeout) | ||
| default: | ||
| return fmt.Sprintf("the function took longer than %v", fe.Timeout) |
There was a problem hiding this comment.
| return fmt.Sprintf("the function took longer than %v", fe.Timeout) | |
| return fmt.Sprintf("pipeline execution timeout of %v exceeded", fe.Timeout) |
| }, | ||
| expected: `[Stderr]: | ||
| error message Exit Code: -1 | ||
| Timed Out: the function took longer than 30s |
There was a problem hiding this comment.
You might have to change this?
| }, | ||
| expected: `[Stderr]: | ||
| error message Exit Code: -1 | ||
| Timed Out: the whole run took longer than 30s |
|
|
||
| _, err = fr.Filter(nil) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, out.String(), "Timed out: the whole run took longer than 100ms") |
| start := time.Now() | ||
| _, err := renderer.Execute(ctx) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, outputBuffer.String(), "Timed out: the whole run took longer than 100ms") |
|
|
||
| _, err = fltr.Filter(nil) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, out.String(), "Timed out: the function took longer than 100ms") |
@Nikshay1 Thanks, that's great. The minor comments were fine. We justtry and keep the SonarCloud green if it's not too much effort. |
Signed-off-by: Nikshay <nikshayyadav90@gmail.com>
|







Description
Adds a
--timeoutflag tokpt fn renderandkpt fn eval.kpt fn render --timeout 10mlimits the run time of the whole pipeline, including the functions of subpackages. When the time has passed, the function that is running is stopped and the render fails.kpt fn eval --timeout 30slimits the run time of its function.When the flag is not set, nothing changes: there is no limit for the whole pipeline, and each function is allowed to run for 5 minutes.
The timeout is a new
Timeoutfield inRunnerOptions. For render,Renderer.Executeruns the pipeline with a context that ends when the timeout has passed (fnruntime.WithRunTimeout), andContainerFnandExecFnnow run the function with that context. For eval, the timeout is passed to theTimeoutfield thatContainerFnandExecFnalready had (thirdparty/kyaml/runfn/runfn.go).A function that is stopped by a timeout is reported like any other function failure, with one more line:
Before, the only sign of a timeout was
Exit code: -1.Motivation
Part of #4473. This is the first of two PRs: this one adds the CLI flag, and the next one will add the per-function
timeoutfield to the Kptfile pipeline. I split them because the Kptfile field changes theapimodule.Notes for reviewers
--timeoutis set onkpt fn render, it is the only limit, so the default of 5 minutes per function does not apply. This way--timeout 20malready helps a function that needs more than 5 minutes, before the Kptfile field exists. I can keep the 5 minute limit per function instead if you prefer.--timeout 0is the same as not passing the flag. A negative value is rejected.ExecError. The timeout is reported through the existingExecError, with two new fields,TimeoutandRunTimeout, so the stderr of the function and the exit code are still printed and still written toresults.yaml.WaitDelayfor exec functions. An exec function that is a shell script can start another process that keeps the output open. kpt then waited for that process even after the timeout had killed the script, so--timeout 100msonsh -c 'sleep 30; true'took 30 seconds.ExecFnnow setscmd.WaitDelayto 1 second. This also applies when there is no timeout: a function that exits but leaves a background process holding its output now fails after 1 second, where kpt used to wait for that process. I can remove this if you prefer to keep the PR smaller.docker/podman/nerdctlCLI, as it did before. I did not change that, and I could not check on my machine whether the container itself is stopped, so the docs only say that the function is stopped and the render fails.Testing
TestExecFn_Timeout: a function that outlives its timeout (stderr and exit code are kept), a function that outlives the timeout of the whole run, both timeouts set with either one shorter, a child process that keeps the output open, a function that finishes in time, and a zero timeout.TestContainerFn_Timeout: the same for the container runtime, with a small script in place of the container CLI, so it runs without Docker.TestRenderer_Execute_Timeout:Renderer.Executestops a pipeline whose function outlivesRunnerOptions.Timeout.TestNewRunner_RunTimeoutandTestDefaultFnFilterProvider_execTimeoutcheck that the timeout reaches the runtime in the render path and in the eval path.TestCmd_timeoutFlagfor render, and new cases inTestRunFnCommand_preRunEfor eval, cover flag parsing and the negative value.go testforpkg/fn/runtime,pkg/lib/runneroptions,commands/fn/render,thirdparty/kyaml/runfnandthirdparty/cmdconfig/commands/cmdeval, andTestRenderer_Execute_Timeoutinpkg/lib/kptops, all pass. I also ran a built binary against a package with a quick and a stuck exec function for both commands. I could not run the tests that need Docker on my machine.AI usage
I have used AI in the creation of this PR.
I have used the following AI tools: