fix: propagate PipelineRun tasks/finally timeout to child TaskRuns#9419
fix: propagate PipelineRun tasks/finally timeout to child TaskRuns#9419infernus01 wants to merge 1 commit intotektoncd:mainfrom
Conversation
tekton-robot
added
release-note
tekton-robot
added
the
size/L
| if facts.FinalTasksGraph != nil && rpt.IsFinalTask(facts) { | ||
| return pr.FinallyTimeout() | ||
| } | ||
| return pr.TasksTimeout() |
There was a problem hiding this comment.
PLR.TasksTimeout is overall timeout of all the tasks in a pipelines. Looks like repurpose of the field to use this as default timeout for each task in pipeline ?
@vdemeester do you agree with that ?
There was a problem hiding this comment.
@pramodbindal Yes each task would get this timeout, and even though each TaskRun has an individual timeout set, the total execution will stay within spec.timeouts.tasks .
There was a problem hiding this comment.
Does it make any difference to PLR ?
because if task is not completed within that time then Pipelinerun will be timed out. and if PLR is timed out then there is no significance of taskrun.
what if we use some kind of default taskTimeout and if task exceeds this duration then timeout the task instead of PLR. (this default task timeout should less than overall pipeline timeout)
There was a problem hiding this comment.
That makes sense, so when timeouts.tasks is smaller than the default, we shouldn't propagate it. We let the default apply.
For exampel, tasks: 20s, default: 1h --> TaskRun gets 1h. The PipelineRun's cumulative timer cancels it at 20s. because 20s < 1h, so the default never fires before the PipelineRun steps in.
|
/assign |
infernus01
force-pushed
the
timeout-fix
branch
from
aac11ca to
7f4f81f
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/rebase |
|
❌ Rebase failed: Cannot rebase PRs from forks. The PR branch must be in the same repository. |
infernus01
force-pushed
the
timeout-fix
branch
from
7f4f81f to
e4c6b69
Compare
infernus01
force-pushed
the
timeout-fix
branch
from
e4c6b69 to
ed70e9c
Compare
|
/retest |
1 similar comment
|
/retest |
|
/test all |
github-project-automation
bot
moved this from Done
to In Progress
in Tekton Community Roadmap
|
@infernus01 we need to update the documentation and highlight the change of behavior there as well (and in the release notes also). But I think otherwise, it looks good 👼🏼 |
|
We should also add that it fixes #8539 |
Signed-off-by: Shubham Bhardwaj <shubbhar@redhat.com>
infernus01
force-pushed
the
timeout-fix
branch
from
ed70e9c to
e1e3aa7
Compare
6 participants
Changes
When a PipelineRun specifies
spec.timeouts.tasks(orspec.timeouts.finally) but no per-task timeout is set viapipeline.spec.tasks[].timeoutorpipelineRun.spec.taskRunSpecs[].timeout, the created TaskRun/CustomRunreceives no explicit timeout. The webhook then applies the global default (
default-timeout-minutes, typically 1h), causing the TaskRun to be cancelled prematurely by the TaskRun reconciler, even when the PipelineRun allows a longer duration.This commit propagates the PipelineRun-level tasks/finally timeout to child TaskRuns and CustomRuns when no per-task timeout is explicitly configured and the PipelineRun timeout exceeds the global default. When the PipelineRun timeout is smaller than the default, the PipelineRun's cumulative enforcement handles cancellation.
Timeout precedence (highest to lowest)
pipelineRun.spec.taskRunSpecs[].timeoutpipeline.spec.tasks[].timeoutpipelineRun.spec.timeouts.tasks/pipelineRun.spec.timeouts.finally(when > global default)default-timeout-minutes)fixes: #8539
/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes