From dd36119baf9247382675cece422157480ee8d404 Mon Sep 17 00:00:00 2001 From: Kewei Yang Date: Mon, 17 Jun 2024 20:08:52 +0800 Subject: [PATCH] fix: When finally timeout is set to 0s, it will continue to enter the 'reconcile' queue --- pkg/reconciler/pipelinerun/pipelinerun.go | 3 +- .../pipelinerun/pipelinerun_test.go | 131 ++++++++++++++---- 2 files changed, 104 insertions(+), 30 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 5e9162e04d9..b25b1ad75b0 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -289,7 +289,8 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1.PipelineRun) pkgr if taskTimeout.Duration == config.NoTimeoutDuration { waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute } - } else if pr.Status.FinallyStartTime != nil && pr.FinallyTimeout() != nil { + } else if pr.Status.FinallyStartTime != nil && pr.FinallyTimeout() != nil && + pr.FinallyTimeout().Duration != config.NoTimeoutDuration { finallyWaitTime := pr.FinallyTimeout().Duration - c.Clock.Since(pr.Status.FinallyStartTime.Time) if finallyWaitTime < waitTime { waitTime = finallyWaitTime diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 3327606b142..6c6dfc37b64 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -2506,24 +2506,7 @@ spec: } func TestReconcileWithTimeoutDisabled(t *testing.T) { - testCases := []struct { - name string - timeout time.Duration - }{ - { - name: "pipeline timeout is 24h", - timeout: 24 * time.Hour, - }, - { - name: "pipeline timeout is way longer than 24h", - timeout: 360 * time.Hour, - }, - } - - for _, tc := range testCases { - startTime := time.Date(2022, time.January, 1, 0, 0, 0, 0, time.UTC).Add(-3 * tc.timeout) - t.Run(tc.name, func(t *testing.T) { - ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, ` + ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, ` metadata: name: test-pipeline namespace: foo @@ -2535,8 +2518,22 @@ spec: - name: hello-world-2 taskRef: name: hello-world +`), parse.MustParseV1Pipeline(t, ` +metadata: + name: test-pipeline-with-finally + namespace: foo +spec: + tasks: + - name: hello-world-1 + taskRef: + name: hello-world + finally: + - name: hello-world-2 + taskRef: + name: hello-world `)} - prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, ` + + prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, ` metadata: name: test-pipeline-run-with-timeout-disabled namespace: foo @@ -2549,32 +2546,108 @@ spec: pipeline: 0h0m0s status: startTime: "2021-12-30T00:00:00Z" +`), parse.MustParseV1PipelineRun(t, ` +metadata: + name: test-pipeline-run-with-timeout-disabled + namespace: foo +spec: + pipelineRef: + name: test-pipeline-with-finally + taskRunTemplate: + serviceAccountName: test-sa + timeouts: + pipeline: 96h0m0s + tasks: 96h0m0s +status: + startTime: "2021-12-30T00:00:00Z" + finallyStartTime: "2021-12-30T23:44:59Z" `)} - ts := []*v1.Task{simpleHelloWorldTask} + ts := []*v1.Task{simpleHelloWorldTask} - trs := []*v1.TaskRun{mustParseTaskRunWithObjectMeta(t, taskRunObjectMeta("test-pipeline-run-with-timeout-hello-world-1", "foo", "test-pipeline-run-with-timeout-disabled", - "test-pipeline", "hello-world-1", false), ` + trs := []*v1.TaskRun{mustParseTaskRunWithObjectMeta(t, taskRunObjectMeta("test-pipeline-run-with-timeout-hello-world-1", "foo", "test-pipeline-run-with-timeout-disabled", + "test-pipeline", "hello-world-1", false), ` +spec: + serviceAccountName: test-sa + taskRef: + name: hello-world + kind: Task +`), mustParseTaskRunWithObjectMeta(t, taskRunObjectMeta("test-pipeline-run-with-timeout-with-finally-hello-world-1", "foo", "test-pipeline-run-with-timeout-disabled", + "test-pipeline-with-finally", "hello-world-1", false), ` +spec: + startTime: "2021-12-30T00:00:00Z" + serviceAccountName: test-sa + taskRef: + name: hello-world + kind: Task + conditions: + - lastTransitionTime: null + status: "True" + type: Succeeded +`), mustParseTaskRunWithObjectMeta(t, taskRunObjectMeta("test-pipeline-run-with-timeout-with-finally-hello-world-2", "foo", "test-pipeline-run-with-timeout-disabled", + "test-pipeline-with-finally", "hello-world-2", false), ` spec: serviceAccountName: test-sa taskRef: name: hello-world kind: Task `)} + + testCases := []struct { + name string + timeout time.Duration + trs []*v1.TaskRun + ts []*v1.Task + ps []*v1.Pipeline + prs []*v1.PipelineRun + }{ + { + name: "pipeline timeout is 24h", + timeout: 24 * time.Hour, + trs: []*v1.TaskRun{trs[0]}, + ts: []*v1.Task{ts[0]}, + prs: []*v1.PipelineRun{prs[0]}, + ps: []*v1.Pipeline{ps[0]}, + }, + { + name: "pipeline timeout is way longer than 24h", + timeout: 360 * time.Hour, + trs: []*v1.TaskRun{trs[0]}, + ts: []*v1.Task{ts[0]}, + prs: []*v1.PipelineRun{prs[0]}, + ps: []*v1.Pipeline{ps[0]}, + }, + { + name: "pipeline timeout is 24h, and the final task timeout is 0s", + timeout: 24 * time.Hour, + trs: []*v1.TaskRun{trs[1], trs[2]}, + ts: []*v1.Task{ts[0]}, + prs: []*v1.PipelineRun{prs[1]}, + ps: []*v1.Pipeline{ps[1]}, + }, + } + + for _, tc := range testCases { + startTime := time.Date(2022, time.January, 1, 0, 0, 0, 0, time.UTC).Add(-3 * tc.timeout) + notAdjustedCreationTimestamp := time.Date(2022, time.January, 1, 0, 0, 0, 0, time.UTC).Add(tc.timeout) + t.Run(tc.name, func(t *testing.T) { start := metav1.NewTime(startTime) - prs[0].Status.StartTime = &start + tc.prs[0].Status.StartTime = &start + for i := range tc.trs { + tc.trs[i].CreationTimestamp = metav1.Time{Time: notAdjustedCreationTimestamp} + } d := test.Data{ - PipelineRuns: prs, - Pipelines: ps, - Tasks: ts, - TaskRuns: trs, + PipelineRuns: tc.prs, + Pipelines: tc.ps, + Tasks: tc.ts, + TaskRuns: tc.trs, } prt := newPipelineRunTest(t, d) defer prt.Cancel() c := prt.TestAssets.Controller clients := prt.TestAssets.Clients - reconcileError := c.Reconciler.Reconcile(prt.TestAssets.Ctx, "foo/test-pipeline-run-with-timeout-disabled") + reconcileError := c.Reconciler.Reconcile(prt.TestAssets.Ctx, fmt.Sprintf("%s/%s", "foo", tc.prs[0].Name)) if reconcileError == nil { t.Errorf("expected error, but got nil") } @@ -2584,7 +2657,7 @@ spec: t.Errorf("Expected a positive requeue duration but got %s", requeueDuration.String()) } prt.Test.Logf("Getting reconciled run") - reconciledRun, err := clients.Pipeline.TektonV1().PipelineRuns("foo").Get(prt.TestAssets.Ctx, "test-pipeline-run-with-timeout-disabled", metav1.GetOptions{}) + reconciledRun, err := clients.Pipeline.TektonV1().PipelineRuns("foo").Get(prt.TestAssets.Ctx, tc.prs[0].Name, metav1.GetOptions{}) if err != nil { prt.Test.Errorf("Somehow had error getting reconciled run out of fake client: %s", err) }