diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 0887a6561bf..8908bd30277 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/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,13 +159,8 @@ 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 - if inGraph(t.PipelineTask.Name, d) && state.IsStopping(d) { + // Skip the PipelineTask if pipeline is in stopping state + if isTaskInGraph(t.PipelineTask.Name, d) && state.IsStopping(d) { return true } @@ -175,9 +168,9 @@ func (t ResolvedPipelineRunTask) IsSkipped(root string, state PipelineRunState, // Recursively look at parent tasks to see if they have been skipped, // if any of the parents have been skipped, skip as well node := d.Nodes[t.PipelineTask.Name] - if inGraph(t.PipelineTask.Name, d) { + if isTaskInGraph(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 } } @@ -224,7 +217,7 @@ func (state PipelineRunState) IsBeforeFirstTaskRun() bool { // at least one task already failed or was cancelled in the specified dag func (state PipelineRunState) IsStopping(d *dag.Graph) bool { for _, t := range state { - if _, ok := d.Nodes[t.PipelineTask.Name]; ok { + if isTaskInGraph(t.PipelineTask.Name, d) { if t.IsCancelled() { return true } @@ -264,8 +257,8 @@ func (state PipelineRunState) GetNextTasks(candidateTasks map[string]struct{}) [ 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 isTaskInGraph(t.PipelineTask.Name, d) { + if t.IsSuccessful() || t.IsSkipped(state, d) { tasks = append(tasks, t.PipelineTask.Name) } } @@ -278,12 +271,12 @@ func (state PipelineRunState) SuccessfulOrSkippedDAGTasks(d *dag.Graph) []string func (state PipelineRunState) checkTasksDone(d *dag.Graph) (isDone bool) { isDone = true for _, t := range state { - if _, ok := d.Nodes[t.PipelineTask.Name]; ok { + if isTaskInGraph(t.PipelineTask.Name, d) { if t.TaskRun == nil { // 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,11 +298,10 @@ 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 { - if _, ok := dfinally.Nodes[t.PipelineTask.Name]; ok && !t.IsSuccessful() { + if isTaskInGraph(t.PipelineTask.Name, dfinally) && !t.IsSuccessful() { finalCandidates[t.PipelineTask.Name] = struct{}{} } } @@ -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 isTaskInGraph(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 diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go index 1577a0e3e74..89ba8185902 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go +++ b/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, diff --git a/test/pipelinefinally_test.go b/test/pipelinefinally_test.go index a9ae68ea07c..c6cda756d33 100644 --- a/test/pipelinefinally_test.go +++ b/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) {