Skip to content

Commit

Permalink
Assume task not skipped if the run is associated
Browse files Browse the repository at this point in the history 
#4582

Currently, the status of PipelineRun with Finally is flakey. For
example, PipelineRun's completionTime is earlier than the last
TaskRun's completionTime, and also Running Finally tasks are marked as
Skipped.

This issue occurs when TaskRun has no conditions. Because the task
state context is not applied to the running Finally tasks and
IsFinallySkipped assumes TaskRun with no conditions as not started.

This commit resolves this issue by assuming the Finally task is not
skipped if the run is associated instead of checking the `Succeeded`
condition.

Signed-off-by: Sunghoon Kang <[email protected]>
  • Loading branch information
Sunghoon Kang committed Jun 9, 2022
1 parent 4621a66 commit cd69baf
Show file tree
Hide file tree
Showing 5 changed files with 202 additions and 4 deletions.
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 11 additions & 2 deletions pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,15 @@ func (t ResolvedPipelineRunTask) isCancelled() bool {
return c != nil && c.IsFalse() && c.Reason == v1beta1.TaskRunReasonCancelled.String()
}

// 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 {
Expand Down Expand Up @@ -205,7 +214,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
Expand Down Expand Up @@ -305,7 +314,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 {
Expand Down
155 changes: 155 additions & 0 deletions pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,12 @@ var gitSweetResourceBinding = v1beta1.PipelineResourceBinding{
ResourceRef: &v1beta1.PipelineResourceRef{Name: "sweet-resource"},
}

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
Expand Down Expand Up @@ -361,6 +367,22 @@ 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 allFinishedState = PipelineRunState{{
PipelineTask: &pts[0],
TaskRunName: "pipelinerun-mytask1",
Expand Down Expand Up @@ -532,6 +554,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{{
Expand Down Expand Up @@ -2517,6 +2546,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{
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/resources/pipelinerunstate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) && !t.isScheduled() {
finalCandidates.Insert(t.PipelineTask.Name)
}
}
Expand Down
34 changes: 34 additions & 0 deletions pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -1160,6 +1185,15 @@ 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{},
}}
for _, tc := range tcs {
dagGraph, err := dag.Build(v1beta1.PipelineTaskList(tc.DAGTasks), v1beta1.PipelineTaskList(tc.DAGTasks).Deps())
Expand Down

0 comments on commit cd69baf

Please sign in to comment.