Skip to content

Commit

Permalink
Account for PipelineRun elapsed time for timeouts
Browse files Browse the repository at this point in the history
Prior to this change, the `getTaskRunTimeout` function returned a timeout for a new TaskRun
with the assumption that the new TaskRun started at the same time that the PipelineRun started.
This led to `pipelinerun.timeouts.tasks` being ignored by TaskRun retries or sequential TaskRuns.
`pipelinerun.timeouts.finally` has the same problem for retries of finally tasks, but does not have the same
problem for multiple separate finally tasks because finally tasks run in parallel.

This commit subtracts time that has already been elapsed in the pipelineRun from `pipelinerun.timeouts.tasks`
when creating a new TaskRun. The relationship between finally timeout and retries will be addressed in a separate commit.

Co-authored-by: Jerop Kipruto [email protected]
  • Loading branch information
lbernick authored and tekton-robot committed Jan 24, 2022
1 parent 8a11ec1 commit f97df42
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 3 deletions.
7 changes: 4 additions & 3 deletions pkg/reconciler/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -1019,6 +1019,7 @@ func getFinallyTaskRunTimeout(ctx context.Context, pr *v1beta1.PipelineRun, rprt
finallyTimeout := pr.FinallyTimeout()
// Return the smaller of taskRunTimeout and finallyTimeout
// This works because all finally tasks run in parallel, so there is no need to consider time spent by other finally tasks
// TODO(#4071): Account for time spent since finally task was first started (i.e. retries)
if finallyTimeout == nil || finallyTimeout.Duration == apisconfig.NoTimeoutDuration {
return taskRunTimeout
}
Expand Down Expand Up @@ -1049,15 +1050,15 @@ func getTaskRunTimeout(ctx context.Context, pr *v1beta1.PipelineRun, rprt *resou
// If pipeline level timeouts have already been exceeded, returns 1 second.
func calculateTaskRunTimeout(timeout time.Duration, pr *v1beta1.PipelineRun, rprt *resources.ResolvedPipelineRunTask, c clock.Clock) *metav1.Duration {
if timeout != apisconfig.NoTimeoutDuration {
pTimeoutTime := pr.Status.StartTime.Add(timeout)
if c.Now().After(pTimeoutTime) {
pElapsedTime := c.Since(pr.Status.StartTime.Time)
if pElapsedTime > timeout {
return &metav1.Duration{Duration: 1 * time.Second}
}
// Return the smaller of timeout and rprt.pipelineTask.timeout
if rprt.PipelineTask.Timeout != nil && rprt.PipelineTask.Timeout.Duration < timeout {
return &metav1.Duration{Duration: rprt.PipelineTask.Timeout.Duration}
}
return &metav1.Duration{Duration: timeout}
return &metav1.Duration{Duration: (timeout - pElapsedTime)}
}

if timeout == apisconfig.NoTimeoutDuration && rprt.PipelineTask.Timeout != nil {
Expand Down
54 changes: 54 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4051,6 +4051,33 @@ func TestGetTaskRunTimeout(t *testing.T) {
},
},
expected: &metav1.Duration{Duration: 1 * time.Minute},
}, {
name: "taskrun with elapsed time",
pr: &v1beta1.PipelineRun{
ObjectMeta: baseObjectMeta(prName, ns),
Spec: v1beta1.PipelineRunSpec{
PipelineRef: &v1beta1.PipelineRef{Name: p},
Timeouts: &v1beta1.TimeoutFields{
Tasks: &metav1.Duration{Duration: 20 * time.Minute},
},
},
Status: v1beta1.PipelineRunStatus{
PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{
StartTime: &metav1.Time{Time: now.Add(-10 * time.Minute)},
},
},
},
rprt: &resources.ResolvedPipelineRunTask{
PipelineTask: &v1beta1.PipelineTask{},
TaskRun: &v1beta1.TaskRun{
Status: v1beta1.TaskRunStatus{
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: nil,
},
},
},
},
expected: &metav1.Duration{Duration: 10 * time.Minute},
},
}

Expand Down Expand Up @@ -4232,6 +4259,33 @@ func TestGetFinallyTaskRunTimeout(t *testing.T) {
},
},
expected: &metav1.Duration{Duration: 1 * time.Minute},
}, {
name: "finally taskrun with elapsed time",
pr: &v1beta1.PipelineRun{
ObjectMeta: baseObjectMeta(prName, ns),
Spec: v1beta1.PipelineRunSpec{
PipelineRef: &v1beta1.PipelineRef{Name: p},
Timeouts: &v1beta1.TimeoutFields{
Finally: &metav1.Duration{Duration: 20 * time.Minute},
},
},
Status: v1beta1.PipelineRunStatus{
PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{
StartTime: &metav1.Time{Time: now.Add(-10 * time.Minute)},
},
},
},
rprt: &resources.ResolvedPipelineRunTask{
PipelineTask: &v1beta1.PipelineTask{},
TaskRun: &v1beta1.TaskRun{
Status: v1beta1.TaskRunStatus{
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: nil,
},
},
},
},
expected: &metav1.Duration{Duration: 20 * time.Minute},
},
}

Expand Down

0 comments on commit f97df42

Please sign in to comment.