refactor GetPipelineConditionStatus

Adding an attribute to PipelineRunState to tell whether a PipelineTask
will not be run because (1) its condition checks failed or (2) one of
the parents conditions failed or (3) pipeline is in stopping state

Instead of explicitly checking (3) in GetPipelineConditionStatus, moved this
check to the IsSkipped function which is used by both the scheduler and
while determining pipeline status.

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

@@ -141,14 +141,12 @@ func (t ResolvedPipelineRunTask) IsStarted() bool {
 	return true
 }
 
-func inGraph(n string, d *dag.Graph) bool {
-	if _, ok := d.Nodes[n]; ok {
-		return true
-	}
-	return false
-}
-
-func (t ResolvedPipelineRunTask) IsSkipped(root string, state PipelineRunState, d *dag.Graph) bool {
+// IsSkipped returns true if a PipelineTask will not be run because
+// (1) its Condition Checks failed or
+// (2) one of the parent task's conditions failed or
+// (3) Pipeline is in stopping state (one of the PipelineTasks failed)
+// Note that this means IsSkipped returns false if a conditionCheck is in progress
+func (t ResolvedPipelineRunTask) IsSkipped(state PipelineRunState, d *dag.Graph) bool {
 	// it already has TaskRun associated with it - PipelineTask not skipped
 	if t.IsStarted() {
 		return false
@@ -161,12 +159,7 @@ func (t ResolvedPipelineRunTask) IsSkipped(root string, state PipelineRunState,
 		}
 	}
 
-	// Skip the task if at least one PipelineTask has failed (not same as the one specified)
-	if t.PipelineTask.Name != root && t.IsFailure() {
-		return true
-	}
-
-	// Skip rest of the PipelineTasks if pipeline is stopping
+	// Skip the PipelineTask if pipeline is in stopping state
 	if inGraph(t.PipelineTask.Name, d) && state.IsStopping(d) {
 		return true
 	}
@@ -177,7 +170,7 @@ func (t ResolvedPipelineRunTask) IsSkipped(root string, state PipelineRunState,
 	node := d.Nodes[t.PipelineTask.Name]
 	if inGraph(t.PipelineTask.Name, d) {
 		for _, p := range node.Prev {
-			if stateMap[p.Task.HashKey()].IsSkipped(root, state, d) {
+			if stateMap[p.Task.HashKey()].IsSkipped(state, d) {
 				return true
 			}
 		}
@@ -265,7 +258,7 @@ func (state PipelineRunState) SuccessfulOrSkippedDAGTasks(d *dag.Graph) []string
 	tasks := []string{}
 	for _, t := range state {
 		if _, ok := d.Nodes[t.PipelineTask.Name]; ok {
-			if t.IsSuccessful() || t.IsSkipped(t.PipelineTask.Name, state, d) {
+			if t.IsSuccessful() || t.IsSkipped(state, d) {
 				tasks = append(tasks, t.PipelineTask.Name)
 			}
 		}
@@ -283,7 +276,7 @@ func (state PipelineRunState) checkTasksDone(d *dag.Graph) (isDone bool) {
 				// this task might have skipped if taskRun is nil
 				// continue and ignore if this task was skipped
 				// skipped task is considered part of done
-				if t.IsSkipped(t.PipelineTask.Name, state, d) {
+				if t.IsSkipped(state, d) {
 					continue
 				}
 				return false
@@ -305,7 +298,6 @@ func (state PipelineRunState) GetFinalTasks(d *dag.Graph, dfinally *dag.Graph) [
 	finalCandidates := map[string]struct{}{}
 	// check either pipeline has finished executing all DAG pipelineTasks
 	// or any one of the DAG pipelineTask has failed
-	//if state.isDAGTasksStopped(d) || state.checkTasksDone(d, dfinally) {
 	if state.checkTasksDone(d) {
 		// return list of tasks with all final tasks
 		for _, t := range state {
@@ -318,6 +310,14 @@ func (state PipelineRunState) GetFinalTasks(d *dag.Graph, dfinally *dag.Graph) [
 	return tasks
 }
 
+// Check if a PipelineTask belongs to the specified Graph
+func inGraph(n string, d *dag.Graph) bool {
+	if _, ok := d.Nodes[n]; ok {
+		return true
+	}
+	return false
+}
+
 // GetTaskRun is a function that will retrieve the TaskRun name.
 type GetTaskRun func(name string) (*v1beta1.TaskRun, error)
 
@@ -546,7 +546,7 @@ func GetPipelineConditionStatus(pr *v1beta1.PipelineRun, state PipelineRunState,
 		switch {
 		case rprt.IsSuccessful():
 			withStatusTasks = append(withStatusTasks, rprt.PipelineTask.Name)
-		case rprt.IsSkipped(rprt.PipelineTask.Name, state, dag):
+		case rprt.IsSkipped(state, dag):
 			skipTasks++
 			withStatusTasks = append(withStatusTasks, rprt.PipelineTask.Name)
 			// At least one is skipped and no failure yet, mark as completed

pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go

@@ -1156,6 +1156,32 @@ func TestIsSkipped(t *testing.T) {
 			},
 		}},
 		expected: true,
+	}, {
+		name:     "task-failed-pipeline-stopping",
+		taskName: "mytask7",
+		state: PipelineRunState{{
+			PipelineTask: &pts[0],
+			TaskRunName:  "pipelinerun-mytask1",
+			TaskRun:      makeFailed(trs[0]),
+			ResolvedTaskResources: &resources.ResolvedTaskResources{
+				TaskSpec: &task.Spec,
+			},
+		}, {
+			PipelineTask: &pts[5],
+			TaskRunName:  "pipelinerun-mytask2",
+			TaskRun:      makeStarted(trs[1]),
+			ResolvedTaskResources: &resources.ResolvedTaskResources{
+				TaskSpec: &task.Spec,
+			},
+		}, {
+			PipelineTask: &pts[6], // mytask7 runAfter mytask6
+			TaskRunName:  "pipelinerun-mytask3",
+			TaskRun:      nil,
+			ResolvedTaskResources: &resources.ResolvedTaskResources{
+				TaskSpec: &task.Spec,
+			},
+		}},
+		expected: true,
 	}}
 
 	for _, tc := range tcs {
@@ -1169,7 +1195,7 @@ func TestIsSkipped(t *testing.T) {
 			if rprt == nil {
 				t.Fatalf("Could not get task %s from the state: %v", tc.taskName, tc.state)
 			}
-			isSkipped := rprt.IsSkipped(rprt.PipelineTask.Name, tc.state, dag)
+			isSkipped := rprt.IsSkipped(tc.state, dag)
 			if d := cmp.Diff(isSkipped, tc.expected); d != "" {
 				t.Errorf("Didn't get expected isSkipped %s", diff.PrintWantGot(d))
 			}
@@ -1301,6 +1327,9 @@ func TestGetPipelineConditionStatus(t *testing.T) {
 		TaskRun:      makeFailed(trs[0]),
 	}}
 
+	var taskMultipleFailuresOneCancel = taskMultipleFailuresSkipRunning
+	taskMultipleFailuresOneCancel = append(taskMultipleFailuresOneCancel, cancelledTask[0])
+
 	var taskNotRunningWithSuccesfulParentsOneFailed = PipelineRunState{{
 		TaskRunName:             "task0taskrun",
 		PipelineTask:            &pts[5],
@@ -1439,7 +1468,7 @@ func TestGetPipelineConditionStatus(t *testing.T) {
 		expectedStatus:    corev1.ConditionFalse,
 		expectedCancelled: 1,
 	}, {
-		name:               "task with multiple failures; one cancelled",
+		name:               "task with multiple failures",
 		state:              taskMultipleFailuresSkipRunning,
 		expectedReason:     v1beta1.PipelineRunReasonStopping.String(),
 		expectedStatus:     corev1.ConditionUnknown,
@@ -1448,6 +1477,16 @@ func TestGetPipelineConditionStatus(t *testing.T) {
 		expectedIncomplete: 1,
 		expectedCancelled:  0,
 		expectedSkipped:    0,
+	}, {
+		name:               "task with multiple failures; one cancelled",
+		state:              taskMultipleFailuresOneCancel,
+		expectedReason:     v1beta1.PipelineRunReasonStopping.String(),
+		expectedStatus:     corev1.ConditionUnknown,
+		expectedSucceeded:  1,
+		expectedFailed:     1,
+		expectedIncomplete: 1,
+		expectedCancelled:  1,
+		expectedSkipped:    0,
 	}, {
 		name:              "task not started with passed parent; one failed",
 		state:             taskNotRunningWithSuccesfulParentsOneFailed,

test/pipelinefinally_test.go

@@ -172,7 +172,7 @@ func TestPipelineLevelFinally_OneFinalTaskFailed_Failure(t *testing.T) {
 		switch n := taskrunItem.Name; {
 		case strings.HasPrefix(n, "pipelinerun-failed-final-tasks-dagtask1"):
 			if !isSuccessful(t, n, taskrunItem.Status.Conditions) {
-				t.Fatalf("TaskRun %s for dag task should have succedded", n)
+				t.Fatalf("TaskRun %s for dag task should have succeeded", n)
 			}
 		case strings.HasPrefix(n, "pipelinerun-failed-final-tasks-finaltask1"):
 			if !isFailed(t, n, taskrunItem.Status.Conditions) {