diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 47979d1d80c..fbb63040d45 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -140,6 +140,43 @@ func (t ResolvedPipelineRunTask) IsStarted() bool { return true } +// 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 + } + + // Check if conditionChecks have failed, if so task is skipped + if len(t.ResolvedConditionChecks) > 0 { + if t.ResolvedConditionChecks.IsDone() && !t.ResolvedConditionChecks.IsSuccess() { + return true + } + } + + // Skip the PipelineTask if pipeline is in stopping state + if isTaskInGraph(t.PipelineTask.Name, d) && state.IsStopping(d) { + return true + } + + stateMap := state.ToMap() + // 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 isTaskInGraph(t.PipelineTask.Name, d) { + for _, p := range node.Prev { + if stateMap[p.Task.HashKey()].IsSkipped(state, d) { + return true + } + } + } + return false +} + // ToMap returns a map that maps pipeline task name to the resolved pipeline run task func (state PipelineRunState) ToMap() map[string]*ResolvedPipelineRunTask { m := make(map[string]*ResolvedPipelineRunTask) @@ -174,7 +211,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 } @@ -213,10 +250,9 @@ func (state PipelineRunState) GetNextTasks(candidateTasks map[string]struct{}) [ // which have successfully completed or skipped func (state PipelineRunState) SuccessfulOrSkippedDAGTasks(d *dag.Graph) []string { tasks := []string{} - stateMap := state.ToMap() for _, t := range state { - if _, ok := d.Nodes[t.PipelineTask.Name]; ok { - if t.IsSuccessful() || isSkipped(t, stateMap, d) { + if isTaskInGraph(t.PipelineTask.Name, d) { + if t.IsSuccessful() || t.IsSkipped(state, d) { tasks = append(tasks, t.PipelineTask.Name) } } @@ -224,34 +260,16 @@ func (state PipelineRunState) SuccessfulOrSkippedDAGTasks(d *dag.Graph) []string return tasks } -// isDAGTasksStopped returns true if any of the pipeline task has failed -// and none of the pipeline task are still running -func (state PipelineRunState) isDAGTasksStopped(d *dag.Graph) bool { - failed := false - for _, t := range state { - if t.IsFailure() { - failed = true - continue - } - if t.IsStarted() && !t.IsDone() { - failed = false - break - } - } - return failed -} - // checkTasksDone returns true if all tasks from the specified graph are finished executing // a task is considered done if it has failed/succeeded/skipped func (state PipelineRunState) checkTasksDone(d *dag.Graph) bool { - stateMap := state.ToMap() 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 isSkipped(t, stateMap, d) { + if t.IsSkipped(state, d) { continue } return false @@ -272,10 +290,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) { + 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{}{} } } @@ -284,6 +302,14 @@ func (state PipelineRunState) GetFinalTasks(d *dag.Graph, dfinally *dag.Graph) [ return tasks } +// Check if a PipelineTask belongs to the specified Graph +func isTaskInGraph(pipelineTaskName string, d *dag.Graph) bool { + if _, ok := d.Nodes[pipelineTaskName]; ok { + return true + } + return false +} + // GetTaskRun is a function that will retrieve the TaskRun name. type GetTaskRun func(name string) (*v1beta1.TaskRun, error) @@ -494,8 +520,6 @@ func GetPipelineConditionStatus(pr *v1beta1.PipelineRun, state PipelineRunState, failedTasks := int(0) cancelledTasks := int(0) reason := v1beta1.PipelineRunReasonSuccessful.String() - stateAsMap := state.ToMap() - isStopping := state.IsStopping(dag) // Check to see if all tasks are success or skipped // @@ -512,18 +536,9 @@ func GetPipelineConditionStatus(pr *v1beta1.PipelineRun, state PipelineRunState, for _, rprt := range state { allTasks = append(allTasks, rprt.PipelineTask.Name) switch { - case !rprt.IsStarted() && isStopping: - // If the pipeline is in stopping mode, all tasks that are not running - // already will be skipped. Otherwise these tasks end up in the - // incomplete count. - // this should never be the case for final task - if _, ok := dag.Nodes[rprt.PipelineTask.Name]; ok { - skipTasks++ - withStatusTasks = append(withStatusTasks, rprt.PipelineTask.Name) - } case rprt.IsSuccessful(): withStatusTasks = append(withStatusTasks, rprt.PipelineTask.Name) - case isSkipped(rprt, stateAsMap, 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 @@ -576,37 +591,6 @@ func GetPipelineConditionStatus(pr *v1beta1.PipelineRun, state PipelineRunState, } } -// isSkipped returns true if a Task in a TaskRun will not be run either because -// its Condition Checks failed or because one of the parent tasks' conditions failed -// Note that this means isSkipped returns false if a conditionCheck is in progress -func isSkipped(rprt *ResolvedPipelineRunTask, stateMap map[string]*ResolvedPipelineRunTask, d *dag.Graph) bool { - // Taskrun not skipped if it already exists - if rprt.TaskRun != nil { - return false - } - - // Check if conditionChecks have failed, if so task is skipped - if len(rprt.ResolvedConditionChecks) > 0 { - // isSkipped is only true iof - if rprt.ResolvedConditionChecks.IsDone() && !rprt.ResolvedConditionChecks.IsSuccess() { - return true - } - } - - // Recursively look at parent tasks to see if they have been skipped, - // if any of the parents have been skipped, skip as well - // continue if the task does not belong to the specified Graph - if node, ok := d.Nodes[rprt.PipelineTask.Name]; ok { - for _, p := range node.Prev { - skip := isSkipped(stateMap[p.Task.HashKey()], stateMap, d) - if skip { - return true - } - } - } - return false -} - func resolveConditionChecks(pt *v1beta1.PipelineTask, taskRunStatus map[string]*v1beta1.PipelineRunTaskRunStatus, taskRunName string, getTaskRun resources.GetTaskRun, getCondition GetCondition, providedResources map[string]*resourcev1alpha1.PipelineResource) ([]*ResolvedConditionCheck, error) { rccs := []*ResolvedConditionCheck{} for i := range pt.Conditions { diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go index 34089f45786..89ba8185902 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go @@ -1080,7 +1080,7 @@ func TestIsSkipped(t *testing.T) { TaskSpec: &task.Spec, }, }}, - expected: false, + expected: true, }, { name: "tasks-parent-cancelled", taskName: "mytask7", @@ -1099,7 +1099,7 @@ func TestIsSkipped(t *testing.T) { TaskSpec: &task.Spec, }, }}, - expected: false, + expected: true, }, { name: "tasks-grandparent-failed", taskName: "mytask10", @@ -1129,7 +1129,7 @@ func TestIsSkipped(t *testing.T) { TaskSpec: &task.Spec, }, }}, - expected: false, + expected: true, }, { name: "tasks-parents-failed-passed", taskName: "mytask8", @@ -1155,7 +1155,33 @@ func TestIsSkipped(t *testing.T) { TaskSpec: &task.Spec, }, }}, - expected: false, + 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 := isSkipped(rprt, stateMap, 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)) } @@ -1197,7 +1223,7 @@ func TestPipelineRunState_SuccessfulOrSkippedDAGTasks(t *testing.T) { }, { name: "one-task-failed", state: oneFailedState, - expectedNames: []string{}, + expectedNames: []string{pts[1].Name}, }, { name: "all-finished", state: allFinishedState, @@ -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,