From 5a448f24cfd67908dbac3ef5ba4f57bd7b150fe2 Mon Sep 17 00:00:00 2001 From: Andrew Bayer Date: Tue, 9 Aug 2022 15:10:48 -0400 Subject: [PATCH] Unscheduled Runs shouldn't be included in full status Fixes #5282 When removing conditions, I accidentally made `GetRunsStatus` not skip including custom task `ResolvedPipelineTask`s without `Run`s in the `PipelineRun.Status.Runs` map. This resulted in cancellation failing, because the cancellation logic tries to patch each `Run` or `TaskRun` in the `PipelineRun` status (either in the `Runs` and `TaskRuns` maps for full embedded status, or `ChildReferences` for minimal or both embedded status). Unscheduled `PipelineTask`s for `Task`s were already properly excluded from the status map, and `ChildReferences` was fine either way, but due to the oversight I mentioned above, unscheduled `PipelineTask`s for custom tasks _were_ included in the full embedded status map, and cancellation would error due to that. cherry-picked from #5287, with some additional modifications to child reference population for custom tasks without a `Run` which have been fixed in v0.38.x. Signed-off-by: Andrew Bayer --- .../pipelinerun/pipelinerun_test.go | 91 +++++++++++++++++++ .../pipelinerun/resources/pipelinerunstate.go | 14 ++- .../resources/pipelinerunstate_test.go | 41 +++++++-- 3 files changed, 130 insertions(+), 16 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 09ed2d49394..fd08b227783 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -7749,3 +7749,94 @@ spec: }) } } + +func TestReconcile_CancelUnscheduled(t *testing.T) { + testCases := []struct { + name string + embeddedStatusVal string + }{ + { + name: "default embedded status", + embeddedStatusVal: config.DefaultEmbeddedStatus, + }, + { + name: "full embedded status", + embeddedStatusVal: config.FullEmbeddedStatus, + }, + { + name: "both embedded status", + embeddedStatusVal: config.BothEmbeddedStatus, + }, + { + name: "minimal embedded status", + embeddedStatusVal: config.MinimalEmbeddedStatus, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + pipelineRunName := "cancel-test-run" + prs := []*v1beta1.PipelineRun{parse.MustParsePipelineRun(t, `metadata: + name: cancel-test-run + namespace: foo +spec: + pipelineSpec: + tasks: + - name: wait-1 + taskSpec: + apiVersion: example.dev/v0 + kind: Wait + params: + - name: duration + value: 1h + - name: wait-2 + runAfter: + - wait-1 + taskSpec: + apiVersion: example.dev/v0 + kind: Wait + params: + - name: duration + value: 10s + - name: wait-3 + runAfter: + - wait-1 + taskRef: + name: hello-world +`)} + + ts := []*v1beta1.Task{simpleHelloWorldTask} + + cms := []*corev1.ConfigMap{withEmbeddedStatus(withCustomTasks(newFeatureFlagsConfigMap()), tc.embeddedStatusVal)} + + d := test.Data{ + PipelineRuns: prs, + Tasks: ts, + ConfigMaps: cms, + } + prt := newPipelineRunTest(d, t) + defer prt.Cancel() + + pr, clients := prt.reconcileRun("foo", pipelineRunName, []string{}, false) + + if tc.embeddedStatusVal != config.MinimalEmbeddedStatus { + if len(pr.Status.Runs) > 1 { + t.Errorf("Expected one Run in status, but found %d", len(pr.Status.Runs)) + } + if len(pr.Status.TaskRuns) > 0 { + t.Errorf("Expected no TaskRuns in status, but found %d", len(pr.Status.TaskRuns)) + } + } + if tc.embeddedStatusVal != config.FullEmbeddedStatus { + if len(pr.Status.ChildReferences) > 1 { + t.Errorf("Expected one Run or TaskRun in child references, but found %d", len(pr.Status.ChildReferences)) + } + } + + err := cancelPipelineRun(prt.TestAssets.Ctx, logtesting.TestLogger(t), pr, clients.Pipeline) + if err != nil { + t.Fatalf("Error found: %v", err) + } + }) + } +} diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go index 0181c6f5bfc..9e5c2e84c1a 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go @@ -198,21 +198,19 @@ func (state PipelineRunState) GetRunsStatus(pr *v1beta1.PipelineRun) map[string] continue } - var prrs *v1beta1.PipelineRunRunStatus - if rprt.Run != nil { - prrs = pr.Status.Runs[rprt.RunName] + if rprt.Run == nil { + continue } + prrs := pr.Status.Runs[rprt.RunName] + if prrs == nil { prrs = &v1beta1.PipelineRunRunStatus{ PipelineTaskName: rprt.PipelineTask.Name, WhenExpressions: rprt.PipelineTask.WhenExpressions, } } - - if rprt.Run != nil { - prrs.Status = &rprt.Run.Status - } + prrs.Status = &rprt.Run.Status status[rprt.RunName] = prrs } @@ -242,7 +240,7 @@ func (state PipelineRunState) GetChildReferences(taskRunVersion string, runVersi var childRefs []v1beta1.ChildStatusReference for _, rprt := range state { - if rprt.CustomTask { + if rprt.CustomTask && rprt.Run != nil { childRefs = append(childRefs, rprt.getChildRefForRun(runVersion)) } else if rprt.TaskRun != nil { childRefs = append(childRefs, rprt.getChildRefForTaskRun(taskRunVersion)) diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go index ac0485687fd..39114c70650 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go @@ -2067,6 +2067,15 @@ spec: }}, TaskRef: &v1beta1.TaskRef{Name: "unit-test-task"}, } + unscheduledPipelineTask := v1beta1.PipelineTask{ + Name: "unit-test-2", + WhenExpressions: []v1beta1.WhenExpression{{ + Input: "foo", + Operator: selection.In, + Values: []string{"foo", "bar"}, + }}, + TaskRef: &v1beta1.TaskRef{Name: "unit-test-task"}, + } task := parse.MustParseTask(t, fmt.Sprintf(` metadata: @@ -2104,6 +2113,12 @@ status: ResolvedTaskResources: &resources.ResolvedTaskResources{ TaskSpec: &task.Spec, }, + }, { + PipelineTask: &unscheduledPipelineTask, + TaskRunName: "test-pipeline-run-success-unit-test-2", + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, }} pr.Status.InitializeConditions(testClock) status := state.GetTaskRunsStatus(pr) @@ -2159,6 +2174,19 @@ spec: Name: "unit-test-run", }, } + unscheduledPipelineTask := v1beta1.PipelineTask{ + Name: "unit-test-2", + WhenExpressions: []v1beta1.WhenExpression{{ + Input: "foo", + Operator: selection.In, + Values: []string{"foo", "bar"}, + }}, + TaskRef: &v1beta1.TaskRef{ + APIVersion: "example.dev/v0", + Kind: "Example", + Name: "unit-test-run", + }, + } run := parse.MustParseRun(t, fmt.Sprintf(` metadata: @@ -2175,6 +2203,10 @@ status: CustomTask: true, RunName: "test-pipeline-run-success-unit-test-1", Run: run, + }, { + PipelineTask: &unscheduledPipelineTask, + CustomTask: true, + RunName: "test-pipeline-run-success-unit-test-2", }} pr.Status.InitializeConditions(testClock) status := state.GetRunsStatus(pr) @@ -2442,14 +2474,7 @@ func TestPipelineRunState_GetChildReferences(t *testing.T) { }, }, }}, - childRefs: []v1beta1.ChildStatusReference{{ - TypeMeta: runtime.TypeMeta{ - APIVersion: "tekton.dev/v1alpha1", - Kind: "Run", - }, - Name: "unresolved-custom-task-run", - PipelineTaskName: "unresolved-custom-task-1", - }}, + childRefs: nil, }, { name: "single-task",