TEP-0090: Failure Strategies - Remove Fail Fast

In #4951, we implemented
`isFailure` for matrixed `TaskRuns` where we applied fail-fast
failure strategy. We discussed failure strategies further in this
PR - tektoncd/community#724 - and API WG
on 13 June 2022. We agreed to leave early termination upon failure
out of scope for the initial release of Matrix. We plan to explore
failure strategies, including fail-fast, in future work. And these
failure strategies may apply more broadly beyond Matrix.

In this change, we update `isFailure` to evaluate to `true` only when
there's a failure and there are no running `TaskRuns` in the `rprt`.

Author: jerop

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

@@ -108,7 +108,8 @@ func (t ResolvedPipelineRunTask) isSuccessful() bool {
 }
 
 // isFailure returns true only if the run has failed and will not be retried.
-// If the PipelineTask has a Matrix, isFailure returns true if any run has failed and will not be retried.
+// If the PipelineTask has a Matrix, isFailure returns true if any run has failed (no remaining retries)
+// and all other runs are done.
 func (t ResolvedPipelineRunTask) isFailure() bool {
 	if t.isCancelled() {
 		return true
@@ -130,14 +131,15 @@ func (t ResolvedPipelineRunTask) isFailure() bool {
 		if len(t.TaskRuns) == 0 {
 			return false
 		}
+		var atLeastOneFailed bool
+		var atLeastOneRunning bool
 		for _, taskRun := range t.TaskRuns {
 			c = taskRun.Status.GetCondition(apis.ConditionSucceeded)
-			isDone = taskRun.IsDone()
-			if isDone && c.IsFalse() && !t.hasRemainingRetries() {
-				return true
-			}
+			taskRunFailed := taskRun.IsDone() && c.IsFalse() && !t.hasRemainingRetries()
+			atLeastOneFailed = atLeastOneFailed || taskRunFailed
+			atLeastOneRunning = atLeastOneRunning || !taskRun.IsDone()
 		}
-		return false
+		return atLeastOneFailed && !atLeastOneRunning
 	default:
 		if t.TaskRun == nil {
 			return false
@@ -192,14 +194,14 @@ func (t ResolvedPipelineRunTask) isCancelled() bool {
 		if len(t.TaskRuns) == 0 {
 			return false
 		}
-		// is cancelled when any TaskRun is cancelled to fail fast
+		var atLeastOneCancelled bool
+		var atLeastOneRunning bool
 		for _, taskRun := range t.TaskRuns {
 			c := taskRun.Status.GetCondition(apis.ConditionSucceeded)
-			if c != nil && c.IsFalse() && c.Reason == v1beta1.TaskRunReasonCancelled.String() {
-				return true
-			}
+			atLeastOneCancelled = atLeastOneCancelled || c != nil && c.IsFalse() && c.Reason == v1beta1.TaskRunReasonCancelled.String()
+			atLeastOneRunning = atLeastOneRunning || !taskRun.IsDone()
 		}
-		return false
+		return atLeastOneCancelled && !atLeastOneRunning
 	default:
 		if t.TaskRun == nil {
 			return false

pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go

@@ -1120,12 +1120,12 @@ func TestIsFailure(t *testing.T) {
 		},
 		want: true,
 	}, {
-		name: "one matrixed taskruns failed",
+		name: "one matrixed taskrun failed, one matrixed taskrun running",
 		rprt: ResolvedPipelineRunTask{
 			PipelineTask: matrixedPipelineTask,
 			TaskRuns:     []*v1beta1.TaskRun{makeFailed(trs[0]), makeStarted(trs[1])},
 		},
-		want: true,
+		want: false,
 	}, {
 		name: "matrixed taskruns failed: retries remaining",
 		rprt: ResolvedPipelineRunTask{
@@ -1155,12 +1155,12 @@ func TestIsFailure(t *testing.T) {
 		},
 		want: true,
 	}, {
-		name: "one matrixed taskrun cancelled",
+		name: "one matrixed taskrun cancelled, one matrixed taskrun running",
 		rprt: ResolvedPipelineRunTask{
 			PipelineTask: matrixedPipelineTask,
 			TaskRuns:     []*v1beta1.TaskRun{withCancelled(makeFailed(trs[0])), makeStarted(trs[1])},
 		},
-		want: true,
+		want: false,
 	}, {
 		name: "matrixed taskruns cancelled but not failed",
 		rprt: ResolvedPipelineRunTask{
@@ -1183,12 +1183,12 @@ func TestIsFailure(t *testing.T) {
 		},
 		want: true,
 	}, {
-		name: "one matrixed taskrun cancelled: retries remaining",
+		name: "one matrixed taskrun cancelled: retries remaining, one matrixed taskrun running",
 		rprt: ResolvedPipelineRunTask{
 			PipelineTask: withPipelineTaskRetries(*matrixedPipelineTask, 1),
 			TaskRuns:     []*v1beta1.TaskRun{withCancelled(makeFailed(trs[0])), makeStarted(trs[1])},
 		},
-		want: true,
+		want: false,
 	}, {
 		name: "matrixed taskruns cancelled: no retries remaining",
 		rprt: ResolvedPipelineRunTask{
@@ -1197,12 +1197,12 @@ func TestIsFailure(t *testing.T) {
 		},
 		want: true,
 	}, {
-		name: "one matrixed taskrun cancelled: no retries remaining",
+		name: "one matrixed taskrun cancelled: no retries remaining, one matrixed taskrun running",
 		rprt: ResolvedPipelineRunTask{
 			PipelineTask: withPipelineTaskRetries(*matrixedPipelineTask, 1),
 			TaskRuns:     []*v1beta1.TaskRun{withCancelled(withRetries(makeFailed(trs[0]))), makeStarted(trs[1])},
 		},
-		want: true,
+		want: false,
 	}} {
 		t.Run(tc.name, func(t *testing.T) {
 			if got := tc.rprt.isFailure(); got != tc.want {