diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 4c7cafd8ac7..3b2553105d2 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -665,7 +665,7 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.Pip // when expressions, so reset the skipped cache pipelineRunFacts.ResetSkippedCache() - // GetFinalTasks only returns tasks when a DAG is complete + // GetFinalTasks only returns final tasks when a DAG is complete fnextRprts := pipelineRunFacts.GetFinalTasks() if len(fnextRprts) != 0 { // apply the runtime context just before creating taskRuns for final tasks in queue diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 95a718c134b..1fcabf8417a 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -209,6 +209,15 @@ func (t ResolvedPipelineRunTask) isCancelled() bool { } } +// isScheduled returns true when the PipelineRunTask itself has a TaskRun +// or Run associated. +func (t ResolvedPipelineRunTask) isScheduled() bool { + if t.IsCustomTask() { + return t.Run != nil + } + return t.TaskRun != nil +} + // isStarted returns true only if the PipelineRunTask itself has a TaskRun or // Run associated that has a Succeeded-type condition. func (t ResolvedPipelineRunTask) isStarted() bool { @@ -249,7 +258,7 @@ func (t *ResolvedPipelineRunTask) skip(facts *PipelineRunFacts) TaskSkipStatus { var skippingReason v1beta1.SkippingReason switch { - case facts.isFinalTask(t.PipelineTask.Name) || t.isStarted(): + case facts.isFinalTask(t.PipelineTask.Name) || t.isScheduled(): skippingReason = v1beta1.None case facts.IsStopping(): skippingReason = v1beta1.StoppingSkip @@ -349,7 +358,7 @@ func (t *ResolvedPipelineRunTask) IsFinallySkipped(facts *PipelineRunFacts) Task var skippingReason v1beta1.SkippingReason switch { - case t.isStarted(): + case t.isScheduled(): skippingReason = v1beta1.None case facts.checkDAGTasksDone() && facts.isFinalTask(t.PipelineTask.Name): switch { diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go index e4481a0f238..265c3ee410e 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go @@ -156,6 +156,12 @@ var trs = []v1beta1.TaskRun{{ Name: "pipelinerun-mytask2", }, Spec: v1beta1.TaskRunSpec{}, +}, { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace", + Name: "pipelinerun-mytask4", + }, + Spec: v1beta1.TaskRunSpec{}, }} var runs = []v1alpha1.Run{{ @@ -190,6 +196,12 @@ var matrixedPipelineTask = &v1beta1.PipelineTask{ }}, } +func makeScheduled(tr v1beta1.TaskRun) *v1beta1.TaskRun { + newTr := newTaskRun(tr) + newTr.Status = v1beta1.TaskRunStatus{ /* explicitly empty */ } + return newTr +} + func makeStarted(tr v1beta1.TaskRun) *v1beta1.TaskRun { newTr := newTaskRun(tr) newTr.Status.Conditions[0].Status = corev1.ConditionUnknown @@ -375,6 +387,38 @@ var oneFailedState = PipelineRunState{{ }, }} +var finalScheduledState = PipelineRunState{{ + PipelineTask: &pts[0], + TaskRunName: "pipelinerun-mytask1", + TaskRun: makeSucceeded(trs[0]), + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, +}, { + PipelineTask: &pts[1], + TaskRunName: "pipelinerun-mytask2", + TaskRun: makeScheduled(trs[1]), + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, +}} + +var retryableFinalState = PipelineRunState{{ + PipelineTask: &pts[0], + TaskRunName: "pipelinerun-mytask1", + TaskRun: makeSucceeded(trs[0]), + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, +}, { + PipelineTask: &pts[3], + TaskRunName: "pipelinerun-mytask4", + TaskRun: makeFailed(trs[2]), + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, +}} + var allFinishedState = PipelineRunState{{ PipelineTask: &pts[0], TaskRunName: "pipelinerun-mytask1", @@ -546,6 +590,13 @@ func TestIsSkipped(t *testing.T) { expected: map[string]bool{ "mytask5": false, }, + }, { + name: "tasks-scheduled", + state: finalScheduledState, + expected: map[string]bool{ + "mytask1": false, + "mytask2": false, + }, }, { name: "tasks-parent-failed", state: PipelineRunState{{ @@ -2656,6 +2707,132 @@ func TestResolvedPipelineRunTask_IsFinallySkipped(t *testing.T) { } } +func TestResolvedPipelineRunTask_IsFinallySkippedByCondition(t *testing.T) { + task := &ResolvedPipelineRunTask{ + TaskRunName: "dag-task", + TaskRun: &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dag-task", + }, + Status: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + }}, + }, + }, + }, + PipelineTask: &v1beta1.PipelineTask{ + Name: "dag-task", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + }, + } + for _, tc := range []struct { + name string + state PipelineRunState + want TaskSkipStatus + }{{ + name: "task started", + state: PipelineRunState{ + task, + { + TaskRun: &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "final-task", + }, + Status: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionUnknown, + }}, + }, + }, + }, + PipelineTask: &v1beta1.PipelineTask{ + Name: "final-task", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + WhenExpressions: []v1beta1.WhenExpression{ + { + Input: "$(tasks.dag-task.status)", + Operator: selection.In, + Values: []string{"Succeeded"}, + }, + }, + }, + }, + }, + want: TaskSkipStatus{ + IsSkipped: false, + SkippingReason: v1beta1.None, + }, + }, { + name: "task scheduled", + state: PipelineRunState{ + task, + { + TaskRunName: "final-task", + TaskRun: &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "final-task", + }, + Status: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{ /* explicitly empty */ }, + }, + }, + }, + PipelineTask: &v1beta1.PipelineTask{ + Name: "final-task", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + WhenExpressions: []v1beta1.WhenExpression{ + { + Input: "$(tasks.dag-task.status)", + Operator: selection.In, + Values: []string{"Succeeded"}, + }, + }, + }, + }}, + want: TaskSkipStatus{ + IsSkipped: false, + SkippingReason: v1beta1.None, + }, + }} { + t.Run(tc.name, func(t *testing.T) { + tasks := v1beta1.PipelineTaskList([]v1beta1.PipelineTask{*tc.state[0].PipelineTask}) + d, err := dag.Build(tasks, tasks.Deps()) + if err != nil { + t.Fatalf("Could not get a dag from the dag tasks %#v: %v", tc.state[0], err) + } + + // build graph with finally tasks + var pts v1beta1.PipelineTaskList + for _, state := range tc.state[1:] { + pts = append(pts, *state.PipelineTask) + } + dfinally, err := dag.Build(pts, map[string][]string{}) + if err != nil { + t.Fatalf("Could not get a dag from the finally tasks %#v: %v", pts, err) + } + + facts := &PipelineRunFacts{ + State: tc.state, + TasksGraph: d, + FinalTasksGraph: dfinally, + } + + for _, state := range tc.state[1:] { + got := state.IsFinallySkipped(facts) + if d := cmp.Diff(tc.want, got); d != "" { + t.Errorf("IsFinallySkipped: %s", diff.PrintWantGot(d)) + } + } + }) + } +} + func TestResolvedPipelineRunTask_IsFinalTask(t *testing.T) { tr := &v1beta1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go index 25d302fc72d..46cb05c3d1e 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go @@ -402,7 +402,7 @@ func (facts *PipelineRunFacts) DAGExecutionQueue() (PipelineRunState, error) { return tasks, nil } -// GetFinalTasks returns a list of final tasks without any taskRun associated with it +// GetFinalTasks returns a list of final tasks which needs to be executed next // GetFinalTasks returns final tasks only when all DAG tasks have finished executing or have been skipped func (facts *PipelineRunFacts) GetFinalTasks() PipelineRunState { tasks := PipelineRunState{} @@ -412,7 +412,7 @@ func (facts *PipelineRunFacts) GetFinalTasks() PipelineRunState { if facts.checkDAGTasksDone() { // return list of tasks with all final tasks for _, t := range facts.State { - if facts.isFinalTask(t.PipelineTask.Name) && !t.isSuccessful() { + if facts.isFinalTask(t.PipelineTask.Name) { finalCandidates.Insert(t.PipelineTask.Name) } } diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go index 44a1d6e4efb..ac0485687fd 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go @@ -267,6 +267,11 @@ func TestIsBeforeFirstTaskRun_WithStartedRun(t *testing.T) { t.Fatalf("Expected state to be after first taskrun (Run test)") } } +func TestIsBeforeFirstTaskRun_WithSucceededTask(t *testing.T) { + if finalScheduledState.IsBeforeFirstTaskRun() { + t.Fatalf("Expected state to be after first taskrun") + } +} func TestGetNextTasks(t *testing.T) { tcs := []struct { @@ -354,6 +359,26 @@ func TestGetNextTasks(t *testing.T) { state: oneFailedState, candidates: sets.NewString("mytask1", "mytask2"), expectedNext: []*ResolvedPipelineRunTask{oneFailedState[1]}, + }, { + name: "final-task-scheduled-no-candidates", + state: finalScheduledState, + candidates: sets.NewString(), + expectedNext: []*ResolvedPipelineRunTask{}, + }, { + name: "final-task-finished-one-candidate", + state: finalScheduledState, + candidates: sets.NewString("mytask1"), + expectedNext: []*ResolvedPipelineRunTask{}, + }, { + name: "final-task-finished-other-candidate", + state: finalScheduledState, + candidates: sets.NewString("mytask2"), + expectedNext: []*ResolvedPipelineRunTask{}, + }, { + name: "final-task-finished-both-candidate", + state: finalScheduledState, + candidates: sets.NewString("mytask1", "mytask2"), + expectedNext: []*ResolvedPipelineRunTask{}, }, { name: "all-finished-no-candidates", state: allFinishedState, @@ -1160,6 +1185,24 @@ func TestPipelineRunState_GetFinalTasks(t *testing.T) { DAGTasks: []v1beta1.PipelineTask{pts[0]}, finalTasks: []v1beta1.PipelineTask{pts[1]}, expectedFinalTasks: PipelineRunState{oneFinishedState[1]}, + }, { + // tasks: [ mytask1] + // finally: [mytask2] + name: "06 - DAG tasks succeeded, final tasks scheduled - no final tasks", + desc: "DAG task (mytask1) finished successfully - final task (mytask2) scheduled - no final tasks", + state: finalScheduledState, + DAGTasks: []v1beta1.PipelineTask{pts[0]}, + finalTasks: []v1beta1.PipelineTask{pts[1]}, + expectedFinalTasks: PipelineRunState{}, + }, { + // tasks: [ mytask1] + // finally: [mytask4] + name: "07 - DAG tasks succeeded, return retryable final tasks", + desc: "DAG task (mytask1) finished successfully - retry failed final tasks (mytask4)", + state: retryableFinalState, + DAGTasks: []v1beta1.PipelineTask{pts[0]}, + finalTasks: []v1beta1.PipelineTask{pts[3]}, + expectedFinalTasks: PipelineRunState{retryableFinalState[1]}, }} for _, tc := range tcs { dagGraph, err := dag.Build(v1beta1.PipelineTaskList(tc.DAGTasks), v1beta1.PipelineTaskList(tc.DAGTasks).Deps())