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 <[email protected]>

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)
+			}
+		})
+	}
+}

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))

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",