fix: When finally timeout is set to 0s, it will continue to enter the…

… 'reconcile' queue
tektoncd · Jul 31, 2024 · dd36119 · dd36119
1 parent f89f789
commit dd36119
Show file tree

Hide file tree

Showing 2 changed files with 104 additions and 30 deletions.
diff --git 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
@@ -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)
 			}