Skip to content

Commit

Permalink
Account for finally TaskRun retries in PR timeouts
Browse files Browse the repository at this point in the history 
Prior to this commit, the PipelineRun reconciler did not account for time elapsed during a finally
TaskRun's retries when setting its timeout. This resulted in `pipelinerun.timeouts.finally` not being
respected when a finally TaskRun was retried.

This commit updates the finally TaskRun's timeout to account for time elapsed during retries.

Co-authored-by: Jerop Kipruto jerop@google.com
  • Loading branch information
@lbernick
lbernick committed Jan 24, 2022
1 parent f97df42 commit 5964986
Show file tree
Hide file tree
Showing 4 changed files with 312 additions and 5 deletions.
17 changes: 12 additions & 5 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1017,14 +1017,21 @@ func combineTaskRunAndTaskSpecAnnotations(pr *v1beta1.PipelineRun, pipelineTask
func getFinallyTaskRunTimeout(ctx context.Context, pr *v1beta1.PipelineRun, rprt *resources.ResolvedPipelineRunTask, c clock.Clock) *metav1.Duration {
taskRunTimeout := calculateTaskRunTimeout(pr.PipelineTimeout(ctx), pr, rprt, c)
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
}
if finallyTimeout.Duration < taskRunTimeout.Duration {
return finallyTimeout
// Return the smaller of taskRunTimeout and finallyTimeRemaining.
// This takes into account the time elapsed during retries of the finally TaskRun, but does not take into account
// time spent by other finally TaskRuns since they are executed in parallel.
finallyTimeRemaining := finallyTimeout.Duration
firstAttemptStartTime := rprt.FirstAttemptStartTime(c)
if firstAttemptStartTime != nil {
elapsed := c.Since(firstAttemptStartTime.Time)
finallyTimeRemaining = finallyTimeRemaining - elapsed
}

if finallyTimeRemaining < taskRunTimeout.Duration {
return &metav1.Duration{Duration: finallyTimeRemaining}
}
return taskRunTimeout
}
Expand Down
66 changes: 66 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4078,6 +4078,39 @@ func TestGetTaskRunTimeout(t *testing.T) {
},
},
expected: &metav1.Duration{Duration: 10 * time.Minute},
}, {
name: "taskrun with retries",
pr: &v1beta1.PipelineRun{
ObjectMeta: baseObjectMeta(prName, ns),
Spec: v1beta1.PipelineRunSpec{
PipelineRef: &v1beta1.PipelineRef{Name: p},
Timeouts: &v1beta1.TimeoutFields{
Pipeline: &metav1.Duration{Duration: 40 * time.Minute},
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,
RetriesStatus: []v1beta1.TaskRunStatus{{
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: &metav1.Time{Time: now.Add(-5 * time.Minute)},
},
}},
},
},
},
},
expected: &metav1.Duration{Duration: 10 * time.Minute},
},
}

Expand Down Expand Up @@ -4286,6 +4319,39 @@ func TestGetFinallyTaskRunTimeout(t *testing.T) {
},
},
expected: &metav1.Duration{Duration: 20 * time.Minute},
}, {
name: "finally taskrun with retries",
pr: &v1beta1.PipelineRun{
ObjectMeta: baseObjectMeta(prName, ns),
Spec: v1beta1.PipelineRunSpec{
PipelineRef: &v1beta1.PipelineRef{Name: p},
Timeouts: &v1beta1.TimeoutFields{
Pipeline: &metav1.Duration{Duration: 40 * time.Minute},
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,
RetriesStatus: []v1beta1.TaskRunStatus{{
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: &metav1.Time{Time: now.Add(-5 * time.Minute)},
},
}},
},
},
},
},
expected: &metav1.Duration{Duration: 15 * time.Minute},
},
}

Expand Down
45 changes: 45 additions & 0 deletions pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@ import (
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
resourcev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1"
"github.com/tektoncd/pipeline/pkg/clock"
"github.com/tektoncd/pipeline/pkg/list"
"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
)

Expand Down Expand Up @@ -174,6 +176,49 @@ func (t ResolvedPipelineRunTask) IsStarted() bool {
return t.TaskRun != nil && t.TaskRun.Status.GetCondition(apis.ConditionSucceeded) != nil
}

// FirstAttemptStartTime returns the start time of the first time the ResolvedPipelineRunTask was attempted.
// Returns nil if no attempt has been started.
func (t *ResolvedPipelineRunTask) FirstAttemptStartTime(c clock.Clock) *metav1.Time {
var startTime *metav1.Time

if t.IsCustomTask() {
r := t.Run
if r == nil {
return nil
}
startTime = r.Status.StartTime
if startTime.IsZero() {
if len(r.Status.RetriesStatus) == 0 {
return startTime
}
startTime = &metav1.Time{Time: c.Now()}
}
for _, retry := range r.Status.RetriesStatus {
if retry.StartTime.Time.Before(startTime.Time) {
startTime = retry.StartTime
}
}
return startTime
}
tr := t.TaskRun
if tr == nil {
return nil
}
startTime = tr.Status.StartTime
if startTime.IsZero() {
if len(tr.Status.RetriesStatus) == 0 {
return startTime
}
startTime = &metav1.Time{Time: c.Now()}
}
for _, retry := range tr.Status.RetriesStatus {
if retry.StartTime.Time.Before(startTime.Time) {
startTime = retry.StartTime
}
}
return startTime
}

// IsConditionStatusFalse returns true when a task has succeeded condition with status set to false
// it includes task failed after retries are exhausted, cancelled tasks, and time outs
func (t ResolvedPipelineRunTask) IsConditionStatusFalse() bool {
Expand Down
189 changes: 189 additions & 0 deletions pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"errors"
"fmt"
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
Expand Down Expand Up @@ -3508,3 +3509,191 @@ func TestGetRunName(t *testing.T) {
})
}
}

func TestResolvedPipelineRunTask_FirstAttemptStartTime(t *testing.T) {
tcs := []struct {
name string
rprt *ResolvedPipelineRunTask
want *metav1.Time
}{{
name: "TaskRun not started",
rprt: &ResolvedPipelineRunTask{
TaskRun: &v1beta1.TaskRun{
Status: v1beta1.TaskRunStatus{
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: nil,
},
},
},
},
want: nil,
}, {
name: "TaskRun on first attempt",
rprt: &ResolvedPipelineRunTask{
TaskRun: &v1beta1.TaskRun{
Status: v1beta1.TaskRunStatus{
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: &metav1.Time{Time: now},
},
},
},
},
want: &metav1.Time{Time: now},
}, {
name: "TaskRun completed first attempt",
rprt: &ResolvedPipelineRunTask{
TaskRun: &v1beta1.TaskRun{
Status: v1beta1.TaskRunStatus{
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: nil,
RetriesStatus: []v1beta1.TaskRunStatus{{
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: &metav1.Time{Time: now.Add(-10 * time.Second)},
CompletionTime: &metav1.Time{Time: now},
},
}},
},
},
},
},
want: &metav1.Time{Time: now.Add(-10 * time.Second)},
}, {
name: "TaskRun on second attempt",
rprt: &ResolvedPipelineRunTask{
TaskRun: &v1beta1.TaskRun{
Status: v1beta1.TaskRunStatus{
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: &metav1.Time{Time: now},
RetriesStatus: []v1beta1.TaskRunStatus{{
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: &metav1.Time{Time: now.Add(-10 * time.Second)},
CompletionTime: &metav1.Time{Time: now.Add(-6 * time.Second)},
},
}},
},
},
},
},
want: &metav1.Time{Time: now.Add(-10 * time.Second)},
}, {
name: "TaskRun on third attempt (multiple retries)",
rprt: &ResolvedPipelineRunTask{
TaskRun: &v1beta1.TaskRun{
Status: v1beta1.TaskRunStatus{
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: &metav1.Time{Time: now},
RetriesStatus: []v1beta1.TaskRunStatus{{
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: &metav1.Time{Time: now.Add(-11 * time.Second)},
CompletionTime: &metav1.Time{Time: now.Add(-6 * time.Second)},
},
}, {
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: &metav1.Time{Time: now.Add(-20 * time.Second)},
CompletionTime: &metav1.Time{Time: now.Add(-12 * time.Second)},
},
}},
},
},
},
},
want: &metav1.Time{Time: now.Add(-20 * time.Second)},
}, {
name: "Run not started",
rprt: &ResolvedPipelineRunTask{
CustomTask: true,
Run: &v1alpha1.Run{
Status: v1alpha1.RunStatus{
RunStatusFields: v1alpha1.RunStatusFields{
StartTime: nil,
},
},
},
},
want: nil,
}, {
name: "Run on first attempt",
rprt: &ResolvedPipelineRunTask{
CustomTask: true,
Run: &v1alpha1.Run{
Status: v1alpha1.RunStatus{
RunStatusFields: v1alpha1.RunStatusFields{
StartTime: &metav1.Time{Time: now},
},
},
},
},
want: &metav1.Time{Time: now},
}, {
name: "Run completed first attempt",
rprt: &ResolvedPipelineRunTask{
CustomTask: true,
Run: &v1alpha1.Run{
Status: v1alpha1.RunStatus{
RunStatusFields: v1alpha1.RunStatusFields{
StartTime: nil,
RetriesStatus: []v1alpha1.RunStatus{{
RunStatusFields: v1alpha1.RunStatusFields{
StartTime: &metav1.Time{Time: now.Add(-10 * time.Second)},
CompletionTime: &metav1.Time{Time: now},
},
}},
},
},
},
},
want: &metav1.Time{Time: now.Add(-10 * time.Second)},
}, {
name: "Run on second attempt",
rprt: &ResolvedPipelineRunTask{
CustomTask: true,
Run: &v1alpha1.Run{
Status: v1alpha1.RunStatus{
RunStatusFields: v1alpha1.RunStatusFields{
StartTime: &metav1.Time{Time: now},
RetriesStatus: []v1alpha1.RunStatus{{
RunStatusFields: v1alpha1.RunStatusFields{
StartTime: &metav1.Time{Time: now.Add(-10 * time.Second)},
CompletionTime: &metav1.Time{Time: now.Add(-6 * time.Second)},
},
}},
},
},
},
},
want: &metav1.Time{Time: now.Add(-10 * time.Second)},
}, {
name: "Run on third attempt (multiple retries)",
rprt: &ResolvedPipelineRunTask{
CustomTask: true,
Run: &v1alpha1.Run{
Status: v1alpha1.RunStatus{
RunStatusFields: v1alpha1.RunStatusFields{
StartTime: &metav1.Time{Time: now},
RetriesStatus: []v1alpha1.RunStatus{{
RunStatusFields: v1alpha1.RunStatusFields{
StartTime: &metav1.Time{Time: now.Add(-11 * time.Second)},
CompletionTime: &metav1.Time{Time: now.Add(-6 * time.Second)},
},
}, {
RunStatusFields: v1alpha1.RunStatusFields{

StartTime: &metav1.Time{Time: now.Add(-20 * time.Second)},
CompletionTime: &metav1.Time{Time: now.Add(-12 * time.Second)},
},
}},
},
},
},
},
want: &metav1.Time{Time: now.Add(-20 * time.Second)},
}}
for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
got := tc.rprt.FirstAttemptStartTime(testClock{})
if cmp.Diff(tc.want, got) != "" {
t.Errorf("Unexpected value for FirstAttemptStartTime: want %v, got %v", tc.want, got)
}
})
}
}

0 comments on commit 5964986

Please sign in to comment.