From f92ec563bcabedafcc5acd398032d047814e2ce6 Mon Sep 17 00:00:00 2001 From: gabemontero Date: Thu, 18 Jan 2024 16:45:31 -0500 Subject: [PATCH 1/4] do not allow negative requeue times Use of the value of 0 for the taskrun/pipeline timeout, which per https://github.com/tektoncd/pipeline/blob/main/docs/pipelineruns.md#configuring-a-failure-timeout for example means timeout is disabled, results in the waitTime passed to the Requeue event to be negative. This had the observed behavior of Requeue'ing immediately, and intense cycles of many reconcilations per second were observed if the TaskRun's/PipelineRun's state did not in fact change. This artificially constrained the peformance of the pipeline controller. This change makes sure the wait time passed to the Requeue is not negative. rh-pre-commit.version: 2.1.0 rh-pre-commit.check-secrets: ENABLED --- docs/pipelineruns.md | 2 + docs/taskruns.md | 2 + pkg/reconciler/pipelinerun/pipelinerun.go | 16 +++- .../pipelinerun/pipelinerun_test.go | 90 +++++++++++++++++++ pkg/reconciler/taskrun/taskrun.go | 7 +- pkg/reconciler/taskrun/taskrun_test.go | 85 ++++++++++++++++++ 6 files changed, 199 insertions(+), 3 deletions(-) diff --git a/docs/pipelineruns.md b/docs/pipelineruns.md index 1fd3a008537..735886af02b 100644 --- a/docs/pipelineruns.md +++ b/docs/pipelineruns.md @@ -1424,6 +1424,8 @@ If you set the timeout to 0, the `PipelineRun` fails immediately upon encounteri > :warning: ** `timeout` is deprecated and will be removed in future versions. Consider using `timeouts` instead. +> :note: An internal detail of the `PipelineRun` and `TaskRun` reconcilers in the Tekton controller is that it will requeue a `PipelineRun` or `TaskRun` for re-evaluation, versus waiting for the next update, under certain conditions. The wait time for that re-queueing is the elapsed time subtracted from the timeout; however, if the timeout is set to '0', that calculation produces a negative number, and the new reconciliation event will fire immediately, which can impact overall performance, which is counter to the intent of wait time calculation. So instead, the reconcilers will use the configured global timeout as the wait time when the associated timeout has been set to '0'. + ## `PipelineRun` status ### The `status` field diff --git a/docs/taskruns.md b/docs/taskruns.md index 6db9c375f1b..ebb9264bee6 100644 --- a/docs/taskruns.md +++ b/docs/taskruns.md @@ -771,6 +771,8 @@ a different global default timeout value using the `default-timeout-minutes` fie all `TaskRuns` that do not have a timeout set will have no timeout and will run until it completes successfully or fails from an error. +> :note: An internal detail of the `PipelineRun` and `TaskRun` reconcilers in the Tekton controller is that it will requeue a `PipelineRun` or `TaskRun` for re-evaluation, versus waiting for the next update, under certain conditions. The wait time for that re-queueing is the elapsed time subtracted from the timeout; however, if the timeout is set to '0', that calculation produces a negative number, and the new reconciliation event will fire immediately, which can impact overall performance, which is counter to the intent of wait time calculation. So instead, the reconcilers will use the configured global timeout as the wait time when the associated timeout has been set to '0'. + ### Specifying `ServiceAccount` credentials You can execute the `Task` in your `TaskRun` with a specific set of credentials by diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index b9a01f558e5..c163438ca81 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -25,6 +25,7 @@ import ( "reflect" "regexp" "strings" + "time" "github.com/hashicorp/go-multierror" "github.com/tektoncd/pipeline/pkg/apis/config" @@ -273,9 +274,20 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1.PipelineRun) pkgr // Compute the time since the task started. elapsed := c.Clock.Since(pr.Status.StartTime.Time) // Snooze this resource until the appropriate timeout has elapsed. - waitTime := pr.PipelineTimeout(ctx) - elapsed - if pr.Status.FinallyStartTime == nil && pr.TasksTimeout() != nil { + // but if the timeout has been disabled by setting timeout to 0, we + // do not want to subtract from 0, because a negative wait time will + // result in the requeue happening essentially immediately + timeout := pr.PipelineTimeout(ctx) + taskTimeout := pr.TasksTimeout() + waitTime := timeout - elapsed + if timeout == config.NoTimeoutDuration { + waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute + } + if pr.Status.FinallyStartTime == nil && taskTimeout != nil { waitTime = pr.TasksTimeout().Duration - elapsed + if taskTimeout.Duration == config.NoTimeoutDuration { + waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute + } } else if pr.Status.FinallyStartTime != nil && pr.FinallyTimeout() != nil { finallyWaitTime := pr.FinallyTimeout().Duration - c.Clock.Since(pr.Status.FinallyStartTime.Time) if finallyWaitTime < waitTime { diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 1b100cd1b71..6af32515743 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -2486,6 +2486,96 @@ 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, ` +metadata: + name: test-pipeline + namespace: foo +spec: + tasks: + - name: hello-world-1 + taskRef: + name: hello-world + - name: hello-world-2 + taskRef: + name: hello-world +`)} + prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, ` +metadata: + name: test-pipeline-run-with-timeout-disabled + namespace: foo +spec: + pipelineRef: + name: test-pipeline + taskRunTemplate: + serviceAccountName: test-sa + timeouts: + pipeline: 0h0m0s +status: + startTime: "2021-12-30T00:00:00Z" +`)} + 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), ` +spec: + serviceAccountName: test-sa + taskRef: + name: hello-world + kind: Task +`)} + start := metav1.NewTime(startTime) + prs[0].Status.StartTime = &start + + d := test.Data{ + PipelineRuns: prs, + Pipelines: ps, + Tasks: ts, + TaskRuns: 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") + if reconcileError == nil { + t.Errorf("expected error, but got nil") + } + if isRequeueError, requeueDuration := controller.IsRequeueKey(reconcileError); !isRequeueError { + t.Errorf("Expected requeue error, but got: %s", reconcileError.Error()) + } else if requeueDuration < 0 { + 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{}) + if err != nil { + prt.Test.Errorf("Somehow had error getting reconciled run out of fake client: %s", err) + } + if reconciledRun.Status.GetCondition(apis.ConditionSucceeded).Reason == "PipelineRunTimeout" { + t.Errorf("Expected PipelineRun to not be timed out, but it is timed out") + } + }) + } +} + func TestReconcileWithTimeoutForALongTimeAndEtcdLimit_Pipeline(t *testing.T) { timeout := 12 * time.Hour testCases := []struct { diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 56482cecd56..0b00799a821 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -211,7 +211,12 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1.TaskRun) pkgrecon // Compute the time since the task started. elapsed := c.Clock.Since(tr.Status.StartTime.Time) // Snooze this resource until the timeout has elapsed. - return controller.NewRequeueAfter(tr.GetTimeout(ctx) - elapsed) + timeout := tr.GetTimeout(ctx) + waitTime := timeout - elapsed + if timeout == config.NoTimeoutDuration { + waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute + } + return controller.NewRequeueAfter(waitTime) } return nil } diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index c681f8ae04b..7eb2e9eb39a 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -2777,6 +2777,91 @@ status: } } +func TestReconcileWithTimeoutDisabled(t *testing.T) { + type testCase struct { + name string + taskRun *v1.TaskRun + } + + testcases := []testCase{ + { + name: "taskrun with timeout", + taskRun: parse.MustParseV1TaskRun(t, ` +metadata: + name: test-taskrun-timeout + namespace: foo +spec: + taskRef: + name: test-task + timeout: 10m +status: + conditions: + - status: Unknown + type: Succeeded +`), + }, { + name: "taskrun with default timeout", + taskRun: parse.MustParseV1TaskRun(t, ` +metadata: + name: test-taskrun-default-timeout-60-minutes + namespace: foo +spec: + taskRef: + name: test-task +status: + conditions: + - status: Unknown + type: Succeeded +`), + }, { + name: "task run with timeout set to 0 to disable", + taskRun: parse.MustParseV1TaskRun(t, ` +metadata: + name: test-taskrun-timeout-disabled + namespace: foo +spec: + taskRef: + name: test-task + timeout: 0s +status: + conditions: + - status: Unknown + type: Succeeded +`), + }} + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + start := metav1.NewTime(time.Now()) + tc.taskRun.Status.StartTime = &start + pod, err := makePod(tc.taskRun, simpleTask) + d := test.Data{ + TaskRuns: []*v1.TaskRun{tc.taskRun}, + Tasks: []*v1.Task{simpleTask}, + Pods: []*corev1.Pod{pod}, + } + testAssets, cancel := getTaskRunController(t, d) + defer cancel() + c := testAssets.Controller + clients := testAssets.Clients + + err = c.Reconciler.Reconcile(testAssets.Ctx, getRunName(tc.taskRun)) + if err == nil { + t.Errorf("expected error when reconciling completed TaskRun : %v", err) + } + if isRequeueError, requeueDuration := controller.IsRequeueKey(err); !isRequeueError { + t.Errorf("Expected requeue error, but got: %s", err.Error()) + } else if requeueDuration < 0 { + t.Errorf("Expected a positive requeue duration but got %s", requeueDuration.String()) + } + _, err = clients.Pipeline.TektonV1().TaskRuns(tc.taskRun.Namespace).Get(testAssets.Ctx, tc.taskRun.Name, metav1.GetOptions{}) + if err != nil { + t.Errorf("Expected completed TaskRun %s to exist but instead got error when getting it: %v", tc.taskRun.Name, err) + } + }) + } +} + func TestReconcileTimeouts(t *testing.T) { type testCase struct { name string From a400338dee46d94ca245aeec53bf260abf37888d Mon Sep 17 00:00:00 2001 From: qingliu Date: Tue, 23 Jan 2024 17:20:58 +0800 Subject: [PATCH 2/4] fix: ensure clustertask annotations are synced to taskrun fix #7601 --- pkg/reconciler/taskrun/resources/taskref_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/reconciler/taskrun/resources/taskref_test.go b/pkg/reconciler/taskrun/resources/taskref_test.go index 62eba5f2653..27379a2f94b 100644 --- a/pkg/reconciler/taskrun/resources/taskref_test.go +++ b/pkg/reconciler/taskrun/resources/taskref_test.go @@ -228,6 +228,12 @@ func TestLocalTaskRef(t *testing.T) { &v1beta1.ClusterTask{ ObjectMeta: metav1.ObjectMeta{ Name: "cluster-task", + Annotations: map[string]string{ + "foo": "bar", + }, + Labels: map[string]string{ + "foo": "bar", + }, }, }, &v1beta1.ClusterTask{ @@ -247,6 +253,12 @@ func TestLocalTaskRef(t *testing.T) { }, ObjectMeta: metav1.ObjectMeta{ Name: "cluster-task", + Annotations: map[string]string{ + "foo": "bar", + }, + Labels: map[string]string{ + "foo": "bar", + }, }, }, wantErr: nil, From b375a19a336c0ff0f1062c59823cf9835cbc6243 Mon Sep 17 00:00:00 2001 From: Andrew Bayer Date: Tue, 6 Feb 2024 14:39:03 -0500 Subject: [PATCH 3/4] Exclude stopped injected sidecars from TaskRun status fixes #7640 In #5565, we started stopping injected sidecars with `nop` in the same way we stop explicitly defined sidecar containers. `MakeTaskRunStatus` was updated to only include explicitly defined sidecars in the `TaskRun` status, rather than just any container in the `TaskRun` pod that doesn't start with `step-`, so while the pod is running, the injected sidecar doesn't show up in the status. However, once the pod has completed and the sidecars are stopped, if the `TaskRun`'s spec contains a sidecar, `updateStoppedSidecarStatus` will be called, and that function's logic for what containers to include in `TaskRun.Status.Sidecars` is still including everything but `step-`-prefixed containers. That should be updated to behave in the same way as `MakeTaskRunStatus`. Signed-off-by: Andrew Bayer --- pkg/pod/entrypoint.go | 4 +-- pkg/pod/status.go | 2 +- pkg/reconciler/taskrun/taskrun.go | 2 +- pkg/reconciler/taskrun/taskrun_test.go | 45 +++++++++++++++++++++++--- 4 files changed, 45 insertions(+), 8 deletions(-) diff --git a/pkg/pod/entrypoint.go b/pkg/pod/entrypoint.go index f8c73d7491b..5997131e5bd 100644 --- a/pkg/pod/entrypoint.go +++ b/pkg/pod/entrypoint.go @@ -338,9 +338,9 @@ func IsSidecarStatusRunning(tr *v1.TaskRun) bool { // represents a step. func IsContainerStep(name string) bool { return strings.HasPrefix(name, stepPrefix) } -// isContainerSidecar returns true if the container name indicates that it +// IsContainerSidecar returns true if the container name indicates that it // represents a sidecar. -func isContainerSidecar(name string) bool { return strings.HasPrefix(name, sidecarPrefix) } +func IsContainerSidecar(name string) bool { return strings.HasPrefix(name, sidecarPrefix) } // trimStepPrefix returns the container name, stripped of its step prefix. func trimStepPrefix(name string) string { return strings.TrimPrefix(name, stepPrefix) } diff --git a/pkg/pod/status.go b/pkg/pod/status.go index 2a5351763ac..0a0f8fdacab 100644 --- a/pkg/pod/status.go +++ b/pkg/pod/status.go @@ -147,7 +147,7 @@ func MakeTaskRunStatus(ctx context.Context, logger *zap.SugaredLogger, tr v1.Tas for _, s := range pod.Status.ContainerStatuses { if IsContainerStep(s.Name) { stepStatuses = append(stepStatuses, s) - } else if isContainerSidecar(s.Name) { + } else if IsContainerSidecar(s.Name) { sidecarStatuses = append(sidecarStatuses, s) } } diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 0b00799a821..4fd6038a5f5 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -914,7 +914,7 @@ func isPodAdmissionFailed(err error) bool { func updateStoppedSidecarStatus(pod *corev1.Pod, tr *v1.TaskRun) error { tr.Status.Sidecars = []v1.SidecarState{} for _, s := range pod.Status.ContainerStatuses { - if !podconvert.IsContainerStep(s.Name) { + if podconvert.IsContainerSidecar(s.Name) { var sidecarState corev1.ContainerState if s.LastTerminationState.Terminated != nil { // Sidecar has successfully by terminated by nop image diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index 7eb2e9eb39a..6bb33ee49eb 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -5367,13 +5367,24 @@ status: } func TestStopSidecars_WithInjectedSidecarsNoTaskSpecSidecars(t *testing.T) { + sidecarTask := &v1.Task{ + ObjectMeta: objectMeta("test-task-injected-sidecar", "foo"), + Spec: v1.TaskSpec{ + Steps: []v1.Step{simpleStep}, + Sidecars: []v1.Sidecar{{ + Name: "sidecar1", + Image: "image-id", + }}, + }, + } + taskRun := parse.MustParseV1TaskRun(t, ` metadata: name: test-taskrun-injected-sidecars namespace: foo spec: taskRef: - name: test-task + name: test-task-injected-sidecar status: podName: test-taskrun-injected-sidecars-pod conditions: @@ -5381,6 +5392,11 @@ status: reason: Build succeeded status: "True" type: Succeeded + sidecars: + - name: sidecar1 + container: sidecar-sidecar1 + running: + startedAt: "2000-01-01T01:01:01Z" `) pod := &corev1.Pod{ @@ -5394,6 +5410,10 @@ status: Name: "step-do-something", Image: "my-step-image", }, + { + Name: "sidecar1", + Image: "image-id", + }, { Name: "injected-sidecar", Image: "some-image", @@ -5407,6 +5427,10 @@ status: Name: "step-do-something", State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{}}, }, + { + Name: "sidecar-sidecar1", + State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}, + }, { Name: "injected-sidecar", State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}, @@ -5418,7 +5442,7 @@ status: d := test.Data{ Pods: []*corev1.Pod{pod}, TaskRuns: []*v1.TaskRun{taskRun}, - Tasks: []*v1.Task{simpleTask}, + Tasks: []*v1.Task{sidecarTask}, } testAssets, cancel := getTaskRunController(t, d) @@ -5438,14 +5462,27 @@ status: t.Fatalf("error retrieving pod: %s", err) } - if len(retrievedPod.Spec.Containers) != 2 { + if len(retrievedPod.Spec.Containers) != 3 { t.Fatalf("expected pod with two containers") } // check that injected sidecar is replaced with nop image - if d := cmp.Diff(images.NopImage, retrievedPod.Spec.Containers[1].Image); d != "" { + if d := cmp.Diff(images.NopImage, retrievedPod.Spec.Containers[2].Image); d != "" { t.Errorf("expected injected sidecar image to be replaced with nop image %s", diff.PrintWantGot(d)) } + + // Get the updated TaskRun. + reconciledRun, err := clients.Pipeline.TektonV1().TaskRuns(taskRun.Namespace).Get(testAssets.Ctx, taskRun.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Error getting updated TaskRun after reconcile: %v", err) + } + + // Verify that the injected sidecar isn't present in the TaskRun's status. + for _, sc := range reconciledRun.Status.Sidecars { + if sc.Container == "injected-sidecar" { + t.Errorf("expected not to find injected-sidecar in TaskRun status, but found %v", sc) + } + } } func Test_validateTaskSpecRequestResources_ValidResources(t *testing.T) { From f8cba46d407d6702f9d3365b6c9e35d34c3b8ade Mon Sep 17 00:00:00 2001 From: Kewei Yang Date: Tue, 6 Feb 2024 16:19:11 +0800 Subject: [PATCH 4/4] Prior to this PR, when the `when.cel` field in the final task refers to ordinary task status, it cannot take effect. In fact, when cel is calculated, the expression is not replaced, and the original literal is used, such as: `"'$(tasks.a-task. status)' == 'Succeeded'"`. This commit will be ensured that the status of the referenced ordinary task is replaced before calculating the final task `when.cel`. --- pkg/reconciler/pipelinerun/pipelinerun.go | 8 + .../pipelinerun/pipelinerun_test.go | 168 ++++++++++++++++++ .../resources/pipelinerunresolution.go | 5 +- 3 files changed, 178 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index c163438ca81..8756c1282f4 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -860,6 +860,14 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1.Pipeline continue } resources.ApplyTaskResults(resources.PipelineRunState{rpt}, resolvedResultRefs) + + if err := rpt.EvaluateCEL(); err != nil { + logger.Errorf("Final task %q is not executed, due to error evaluating CEL %s: %v", rpt.PipelineTask.Name, pr.Name, err) + pr.Status.MarkFailed(string(v1.PipelineRunReasonCELEvaluationFailed), + "Error evaluating CEL %s: %w", pr.Name, pipelineErrors.WrapUserError(err)) + return controller.NewPermanentError(err) + } + nextRpts = append(nextRpts, rpt) } } diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 6af32515743..837b7982607 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -4643,6 +4643,174 @@ spec: } } +func TestReconcileWithFinalTasksCELWhenExpressions(t *testing.T) { + ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, ` +metadata: + name: test-pipeline + namespace: foo +spec: + params: + - name: run + type: string + tasks: + - name: a-task + taskRef: + name: a-task + - name: b-task + taskRef: + name: b-task + finally: + - name: f-c-task + taskRef: + name: f-c-task + when: + - cel: "'$(tasks.a-task.results.aResult)' == 'aResultValue'" + - cel: "'$(params.run)'=='yes'" + - cel: "'$(tasks.a-task.status)' == 'Succeeded'" + - name: f-d-task + taskRef: + name: f-d-task + when: + - cel: "'$(tasks.b-task.status)' == 'Succeeded'" +`)} + prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, ` +metadata: + name: test-pipeline-run-different-final-task-when + namespace: foo +spec: + params: + - name: run + value: "yes" + pipelineRef: + name: test-pipeline + taskRunTemplate: + serviceAccountName: test-sa-0 +`)} + ts := []*v1.Task{ + {ObjectMeta: baseObjectMeta("a-task", "foo")}, + {ObjectMeta: baseObjectMeta("b-task", "foo")}, + {ObjectMeta: baseObjectMeta("f-c-task", "foo")}, + {ObjectMeta: baseObjectMeta("f-d-task", "foo")}, + } + trs := []*v1.TaskRun{mustParseTaskRunWithObjectMeta(t, + taskRunObjectMeta("test-pipeline-run-different-final-task-when-a-task-xxyyy", "foo", "test-pipeline-run-different-final-task-when", + "test-pipeline", "a-task", true), + ` +spec: + serviceAccountName: test-sa + taskRef: + name: hello-world + timeout: 1h0m0s +status: + conditions: + - status: "True" + type: Succeeded + results: + - name: aResult + value: aResultValue +`), mustParseTaskRunWithObjectMeta(t, + taskRunObjectMeta("test-pipeline-run-different-final-task-when-b-task-xxyyy", "foo", "test-pipeline-run-different-final-task-when", + "test-pipeline", "b-task", true), + ` +spec: + serviceAccountName: test-sa + taskRef: + name: hello-world + timeout: 1h0m0s +status: + conditions: + - status: "False" + type: Succeeded +`)} + cms := []*corev1.ConfigMap{ + { + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()}, + Data: map[string]string{ + "enable-cel-in-whenexpression": "true", + }, + }, + } + d := test.Data{ + PipelineRuns: prs, + Pipelines: ps, + Tasks: ts, + TaskRuns: trs, + ConfigMaps: cms, + } + prt := newPipelineRunTest(t, d) + defer prt.Cancel() + + wantEvents := []string{ + "Normal Started", + "Normal Running Tasks Completed: 2 \\(Failed: 1, Cancelled 0\\), Incomplete: 1, Skipped: 1", + } + pipelineRun, clients := prt.reconcileRun("foo", "test-pipeline-run-different-final-task-when", wantEvents, false) + + expectedTaskRunName := "test-pipeline-run-different-final-task-when-f-c-task" + expectedTaskRun := mustParseTaskRunWithObjectMeta(t, + taskRunObjectMeta(expectedTaskRunName, "foo", "test-pipeline-run-different-final-task-when", "test-pipeline", "f-c-task", true), + ` +spec: + serviceAccountName: test-sa-0 + taskRef: + name: f-c-task + kind: Task +`) + expectedTaskRun.Labels[pipeline.MemberOfLabelKey] = v1.PipelineFinallyTasks + // Check that the expected TaskRun was created + actual, err := clients.Pipeline.TektonV1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{ + LabelSelector: "tekton.dev/pipelineTask=f-c-task,tekton.dev/pipelineRun=test-pipeline-run-different-final-task-when", + Limit: 1, + }) + + if err != nil { + t.Fatalf("Failure to list TaskRun's %s", err) + } + if len(actual.Items) != 1 { + t.Fatalf("Expected 1 TaskRuns got %d", len(actual.Items)) + } + actualTaskRun := actual.Items[0] + if d := cmp.Diff(expectedTaskRun, &actualTaskRun, ignoreResourceVersion, ignoreTypeMeta); d != "" { + t.Errorf("expected to see TaskRun %v created. Diff %s", expectedTaskRunName, diff.PrintWantGot(d)) + } + + expectedWhenExpressionsInTaskRun := []v1.WhenExpression{{ + CEL: "'aResultValue' == 'aResultValue'", + }, { + CEL: "'yes'=='yes'", + }, { + CEL: "'Succeeded' == 'Succeeded'", + }} + verifyTaskRunStatusesWhenExpressions(t, pipelineRun.Status, expectedTaskRunName, expectedWhenExpressionsInTaskRun) + + actualSkippedTasks := pipelineRun.Status.SkippedTasks + expectedSkippedTasks := []v1.SkippedTask{{ + Name: "f-d-task", + Reason: v1.WhenExpressionsSkip, + WhenExpressions: v1.WhenExpressions{{ + CEL: "'Failed' == 'Succeeded'", + }}, + }} + if d := cmp.Diff(expectedSkippedTasks, actualSkippedTasks); d != "" { + t.Errorf("expected to find Skipped Tasks %v. Diff %s", expectedSkippedTasks, diff.PrintWantGot(d)) + } + + skippedTasks := []string{"f-d-task"} + for _, skippedTask := range skippedTasks { + labelSelector := fmt.Sprintf("tekton.dev/pipelineTask=%s,tekton.dev/pipelineRun=test-pipeline-run-different-final-task-when", skippedTask) + actualSkippedTask, err := clients.Pipeline.TektonV1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{ + LabelSelector: labelSelector, + Limit: 1, + }) + if err != nil { + t.Fatalf("Failure to list TaskRun's %s", err) + } + if len(actualSkippedTask.Items) != 0 { + t.Fatalf("Expected 0 TaskRuns got %d", len(actualSkippedTask.Items)) + } + } +} + func TestReconcile_InvalidCELWhenExpressions(t *testing.T) { ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, ` metadata: diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index fa595f13df4..e4000ad2749 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -76,9 +76,8 @@ type ResolvedPipelineTask struct { // EvaluateCEL evaluate the CEL expressions, and store the evaluated results in EvaluatedCEL func (t *ResolvedPipelineTask) EvaluateCEL() error { if t.PipelineTask != nil { - if len(t.EvaluatedCEL) == 0 { - t.EvaluatedCEL = make(map[string]bool) - } + // Each call to this function will reset this field to prevent additional CELs. + t.EvaluatedCEL = make(map[string]bool) for _, we := range t.PipelineTask.When { if we.CEL == "" { continue