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.

Sign up for GitHub

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

fix: panic may occur when calculating the final task timeout waiting time #7188

Merged
merged 1 commit into from
Oct 27, 2023
Merged

fix: panic may occur when calculating the final task timeout waiting time #7188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1.PipelineRun) pkgr
waitTime := pr.PipelineTimeout(ctx) - elapsed
if pr.Status.FinallyStartTime == nil && pr.TasksTimeout() != nil {
waitTime = pr.TasksTimeout().Duration - elapsed
} else if pr.FinallyTimeout() != nil {
} else if pr.Status.FinallyStartTime != nil && pr.FinallyTimeout() != nil {
finallyWaitTime := pr.FinallyTimeout().Duration - c.Clock.Since(pr.Status.FinallyStartTime.Time)
if finallyWaitTime < waitTime {
waitTime = finallyWaitTime
Expand Down Expand Up @@ -700,7 +700,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
errs := timeoutPipelineTasksForTaskNames(ctx, logger, pr, c.PipelineClientSet, tasksToTimeOut)
if len(errs) > 0 {
errString := strings.Join(errs, "\n")
logger.Errorf("Failed to timeout tasks for PipelineRun %s/%s: %s", pr.Name, pr.Name, errString)
logger.Errorf("Failed to timeout tasks for PipelineRun %s/%s: %s", pr.Namespace, pr.Name, errString)
return fmt.Errorf("error(s) from cancelling TaskRun(s) from PipelineRun %s: %s", pr.Name, errString)
}
}
Expand All @@ -717,7 +717,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
errs := timeoutPipelineTasksForTaskNames(ctx, logger, pr, c.PipelineClientSet, tasksToTimeOut)
if len(errs) > 0 {
errString := strings.Join(errs, "\n")
logger.Errorf("Failed to timeout finally tasks for PipelineRun %s/%s: %s", pr.Name, pr.Name, errString)
logger.Errorf("Failed to timeout finally tasks for PipelineRun %s/%s: %s", pr.Namespace, pr.Name, errString)
return fmt.Errorf("error(s) from cancelling TaskRun(s) from PipelineRun %s: %s", pr.Name, errString)
}
}
Expand Down
85 changes: 68 additions & 17 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2694,7 +2694,7 @@ spec:
`)}
prName := "test-pipeline-run-with-timeout"
ts := []*v1.Task{simpleHelloWorldTask}
trs := []*v1.TaskRun{
oneFinishedTRs := []*v1.TaskRun{
getTaskRun(
t,
"test-pipeline-run-with-timeout-hello-world",
Expand All @@ -2711,12 +2711,26 @@ spec:
name: hello-world
kind: Task
`)}
oneStartedTRs := []*v1.TaskRun{
getTaskRun(
t,
"test-pipeline-run-with-timeout-hello-world",
prName,
ps[0].Name,
"hello-world",
corev1.ConditionUnknown,
),
}

tcs := []struct {
name string
pr *v1.PipelineRun
name string
trs []*v1.TaskRun
pr *v1.PipelineRun
wantFinallyTimeout bool
wantOneFinalTaskSkipped bool
}{{
name: "finally timeout specified",
trs: oneFinishedTRs,
pr: parse.MustParseV1PipelineRun(t, fmt.Sprintf(`
metadata:
name: %s
Expand Down Expand Up @@ -2749,8 +2763,11 @@ status:
status: "Unknown"
type: Succeeded
`, prName)),
wantFinallyTimeout: true,
wantOneFinalTaskSkipped: true,
}, {
name: "finally timeout calculated",
trs: oneFinishedTRs,
pr: parse.MustParseV1PipelineRun(t, fmt.Sprintf(`
metadata:
name: %s
Expand Down Expand Up @@ -2784,13 +2801,43 @@ status:
status: "Unknown"
type: Succeeded
`, prName)),
wantFinallyTimeout: true,
wantOneFinalTaskSkipped: true,
}, {
name: "finally timeout specified and pipeline timeout set to 0",
trs: oneStartedTRs,
pr: parse.MustParseV1PipelineRun(t, fmt.Sprintf(`
metadata:
name: %s
namespace: foo
spec:
pipelineRef:
name: test-pipeline-with-finally
timeouts:
finally: 15m
pipeline: 0s
status:
startTime: "2021-12-31T23:40:00Z"
childReferences:
- name: test-pipeline-run-with-timeout-hello-world
apiVersion: tekton.dev/v1
kind: TaskRun
pipelineTaskName: task1
status:
conditions:
- lastTransitionTime: null
status: Unknown
type: Succeeded
`, prName)),
wantFinallyTimeout: false,
wantOneFinalTaskSkipped: false,
}}
for _, tc := range tcs {
d := test.Data{
PipelineRuns: []*v1.PipelineRun{tc.pr},
Pipelines: ps,
Tasks: ts,
TaskRuns: trs,
TaskRuns: tc.trs,
}
prt := newPipelineRunTest(t, d)
defer prt.Cancel()
Expand All @@ -2810,22 +2857,26 @@ status:
}

// Check that there is a skipped task for the expected reason
if len(reconciledRun.Status.SkippedTasks) != 1 {
t.Errorf("expected one skipped task, found %d", len(reconciledRun.Status.SkippedTasks))
} else if reconciledRun.Status.SkippedTasks[0].Reason != v1.FinallyTimedOutSkip {
t.Errorf("expected skipped reason to be '%s', but was '%s", v1.FinallyTimedOutSkip, reconciledRun.Status.SkippedTasks[0].Reason)
if tc.wantOneFinalTaskSkipped {
if len(reconciledRun.Status.SkippedTasks) != 1 {
t.Errorf("expected one skipped task, found %d", len(reconciledRun.Status.SkippedTasks))
} else if reconciledRun.Status.SkippedTasks[0].Reason != v1.FinallyTimedOutSkip {
t.Errorf("expected skipped reason to be '%s', but was '%s", v1.FinallyTimedOutSkip, reconciledRun.Status.SkippedTasks[0].Reason)
}
}

updatedTaskRun, err := clients.Pipeline.TektonV1().TaskRuns("foo").Get(context.Background(), trs[1].Name, metav1.GetOptions{})
if err != nil {
t.Fatalf("error getting updated TaskRun: %#v", err)
}
if tc.wantFinallyTimeout {
updatedTaskRun, err := clients.Pipeline.TektonV1().TaskRuns("foo").Get(context.Background(), tc.trs[1].Name, metav1.GetOptions{})
if err != nil {
t.Fatalf("error getting updated TaskRun: %#v", err)
}

if updatedTaskRun.Spec.Status != v1.TaskRunSpecStatusCancelled {
t.Errorf("expected existing TaskRun Spec.Status to be set to %s, but was %s", v1.TaskRunSpecStatusCancelled, updatedTaskRun.Spec.Status)
}
if updatedTaskRun.Spec.StatusMessage != v1.TaskRunCancelledByPipelineTimeoutMsg {
t.Errorf("expected existing TaskRun Spec.StatusMessage to be set to %s, but was %s", v1.TaskRunCancelledByPipelineTimeoutMsg, updatedTaskRun.Spec.StatusMessage)
if updatedTaskRun.Spec.Status != v1.TaskRunSpecStatusCancelled {
t.Errorf("expected existing TaskRun Spec.Status to be set to %s, but was %s", v1.TaskRunSpecStatusCancelled, updatedTaskRun.Spec.Status)
}
if updatedTaskRun.Spec.StatusMessage != v1.TaskRunCancelledByPipelineTimeoutMsg {
t.Errorf("expected existing TaskRun Spec.StatusMessage to be set to %s, but was %s", v1.TaskRunCancelledByPipelineTimeoutMsg, updatedTaskRun.Spec.StatusMessage)
}
}
}
}
Expand Down
Loading