Update TaskRun status even if PipelineRun is done

Due to the DAG execution model, it's possible for the PipelineRun to
error while other TaskRuns are still executing. This change resolves a
bug where the status for the other TaskRuns would not propagate to the
PipelineRun if it has failed.

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

@@ -176,22 +176,25 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 
 	if pr.IsDone() {
 		c.timeoutHandler.Release(pr)
-		c.Recorder.Event(pr, corev1.EventTypeNormal, eventReasonSucceeded, "PipelineRun completed successfully.")
-		return nil
-	}
+		if err := c.updateTaskRunsStatusDirectly(pr); err != nil {
+			c.Logger.Errorf("Failed to update TaskRun status for PipelineRun %s: %v", pr.Name, err)
+			return err
+		}
+	} else {
+		if err := c.tracker.Track(pr.GetTaskRunRef(), pr); err != nil {
+			c.Logger.Errorf("Failed to create tracker for TaskRuns for PipelineRun %s: %v", pr.Name, err)
+			c.Recorder.Event(pr, corev1.EventTypeWarning, eventReasonFailed, "Failed to create tracker for TaskRuns for PipelineRun")
+			return err
+		}
 
-	if err := c.tracker.Track(pr.GetTaskRunRef(), pr); err != nil {
-		c.Logger.Errorf("Failed to create tracker for TaskRuns for PipelineRun %s: %v", pr.Name, err)
-		c.Recorder.Event(pr, corev1.EventTypeWarning, eventReasonFailed, "Failed to create tracker for TaskRuns for PipelineRun")
-		return err
+		// Reconcile this copy of the pipelinerun and then write back any status or label
+		// updates regardless of whether the reconciliation errored out.
+		if err = c.reconcile(ctx, pr); err != nil {
+			c.Logger.Errorf("Reconcile error: %v", err.Error())
+			return err
+		}
 	}
 
-	// Reconcile this copy of the pipelinerun and then write back any status or label
-	// updates regardless of whether the reconciliation errored out.
-	if err = c.reconcile(ctx, pr); err != nil {
-		c.Logger.Errorf("Reconcile error: %v", err.Error())
-		return err
-	}
 	if equality.Semantic.DeepEqual(original.Status, pr.Status) {
 		// If we didn't change anything then don't call updateStatus.
 		// This is important because the copy we loaded from the informer's
@@ -396,6 +399,23 @@ func updateTaskRunsStatus(pr *v1alpha1.PipelineRun, pipelineState []*resources.R
 	}
 }
 
+func (c *Reconciler) updateTaskRunsStatusDirectly(pr *v1alpha1.PipelineRun) error {
+	for taskRunName := range pr.Status.TaskRuns {
+		prtrs := pr.Status.TaskRuns[taskRunName]
+		tr, err := c.taskRunLister.TaskRuns(pr.Namespace).Get(taskRunName)
+		if err != nil {
+			// If the TaskRun isn't found, it just means it won't be run
+			if !errors.IsNotFound(err) {
+				return fmt.Errorf("error retrieving TaskRun %s: %s", taskRunName, err)
+			}
+		} else {
+			prtrs.Status = &tr.Status
+		}
+	}
+
+	return nil
+}
+
 func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, rprt *resources.ResolvedPipelineRunTask, pr *v1alpha1.PipelineRun, storageBasePath string) (*v1alpha1.TaskRun, error) {
 	var taskRunTimeout = &metav1.Duration{Duration: 0 * time.Second}
 	if pr.Spec.Timeout != nil {

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go

@@ -451,22 +451,45 @@ func TestUpdateTaskRunsState(t *testing.T) {
 }
 
 func TestReconcileOnCompletedPipelineRun(t *testing.T) {
+	prtrs := make(map[string]*v1alpha1.PipelineRunTaskRunStatus)
+	taskRunName := "test-pipeline-run-completed-hello-world"
+	prtrs[taskRunName] = &v1alpha1.PipelineRunTaskRunStatus{
+		PipelineTaskName: "hello-world-1",
+		Status:           &v1alpha1.TaskRunStatus{},
+	}
 	prs := []*v1alpha1.PipelineRun{tb.PipelineRun("test-pipeline-run-completed", "foo",
 		tb.PipelineRunSpec("test-pipeline", tb.PipelineRunServiceAccount("test-sa")),
-		tb.PipelineRunStatus(tb.PipelineRunStatusCondition(duckv1alpha1.Condition{
-			Type:    duckv1alpha1.ConditionSucceeded,
-			Status:  corev1.ConditionTrue,
-			Reason:  resources.ReasonSucceeded,
-			Message: "All Tasks have completed executing",
-		})),
+		tb.PipelineRunStatus(
+			tb.PipelineRunStatusCondition(duckv1alpha1.Condition{
+				Type:    duckv1alpha1.ConditionSucceeded,
+				Status:  corev1.ConditionTrue,
+				Reason:  resources.ReasonSucceeded,
+				Message: "All Tasks have completed executing",
+			}),
+			tb.PipelineRunTaskRunsStatus(prtrs),
+		),
 	)}
 	ps := []*v1alpha1.Pipeline{tb.Pipeline("test-pipeline", "foo", tb.PipelineSpec(
-		tb.PipelineTask("hello-world-1", "hellow-world")))}
+		tb.PipelineTask("hello-world-1", "hello-world")))}
 	ts := []*v1alpha1.Task{tb.Task("hello-world", "foo")}
+	trs := []*v1alpha1.TaskRun{
+		tb.TaskRun(taskRunName, "foo",
+			tb.TaskRunOwnerReference("kind", "name"),
+			tb.TaskRunLabel(pipeline.GroupName+pipeline.PipelineLabelKey, "test-pipeline-run-completed"),
+			tb.TaskRunLabel(pipeline.GroupName+pipeline.PipelineRunLabelKey, "test-pipeline"),
+			tb.TaskRunSpec(tb.TaskRunTaskRef("hello-world")),
+			tb.TaskRunStatus(
+				tb.Condition(duckv1alpha1.Condition{
+					Type: duckv1alpha1.ConditionSucceeded,
+				}),
+			),
+		),
+	}
 	d := test.Data{
 		PipelineRuns: prs,
 		Pipelines:    ps,
 		Tasks:        ts,
+		TaskRuns:     trs,
 	}
 
 	// create fake recorder for testing
@@ -479,10 +502,15 @@ func TestReconcileOnCompletedPipelineRun(t *testing.T) {
 	c.Reconciler.Reconcile(context.Background(), "foo/test-pipeline-run-completed")
 
 	// make sure there is no failed events
-	validateEvents(t, fr)
+	validateNoEvents(t, fr)
 
-	if len(clients.Pipeline.Actions()) != 0 {
-		t.Fatalf("Expected client to not have created a TaskRun for the completed PipelineRun, but it did")
+	if len(clients.Pipeline.Actions()) != 1 {
+		t.Fatalf("Expected client to have updated the TaskRun status for a completed PipelineRun, but it did not")
+	}
+
+	actual := clients.Pipeline.Actions()[0].(ktesting.UpdateAction).GetObject().(*v1alpha1.PipelineRun)
+	if actual == nil {
+		t.Errorf("Expected a PipelineRun to be updated, but it wasn't.")
 	}
 
 	// Check that the PipelineRun was reconciled correctly
@@ -495,6 +523,20 @@ func TestReconcileOnCompletedPipelineRun(t *testing.T) {
 	if reconciledRun.Status.GetCondition(duckv1alpha1.ConditionSucceeded).IsUnknown() {
 		t.Errorf("Expected PipelineRun status to be complete, but was %v", reconciledRun.Status.GetCondition(duckv1alpha1.ConditionSucceeded))
 	}
+
+	expectedTaskRunsStatus := make(map[string]*v1alpha1.PipelineRunTaskRunStatus)
+	expectedTaskRunsStatus[taskRunName] = &v1alpha1.PipelineRunTaskRunStatus{
+		PipelineTaskName: prtrs[taskRunName].PipelineTaskName,
+		Status: &v1alpha1.TaskRunStatus{
+			Conditions: []duckv1alpha1.Condition{{
+				Type: duckv1alpha1.ConditionSucceeded,
+			}},
+		},
+	}
+
+	if d := cmp.Diff(reconciledRun.Status.TaskRuns, expectedTaskRunsStatus); d != "" {
+		t.Fatalf("Expected PipelineRun status to match TaskRun(s) status, but got a mismatch: %s", d)
+	}
 }
 
 func validateNoEvents(t *testing.T, r *record.FakeRecorder) {

test/builder/pipeline.go

@@ -355,6 +355,13 @@ func PipelineRunStartTime(startTime time.Time) PipelineRunStatusOp {
 	}
 }
 
+// PipelineRunTaskRunsStatus sets the TaskRuns of the PipelineRunStatus.
+func PipelineRunTaskRunsStatus(taskRuns map[string]*v1alpha1.PipelineRunTaskRunStatus) PipelineRunStatusOp {
+	return func(s *v1alpha1.PipelineRunStatus) {
+		s.TaskRuns = taskRuns
+	}
+}
+
 // PipelineResource creates a PipelineResource with default values.
 // Any number of PipelineResource modifier can be passed to transform it.
 func PipelineResource(name, namespace string, ops ...PipelineResourceOp) *v1alpha1.PipelineResource {