From 3d3f5f4c0454b30fa6407c481ab775ec89ed0104 Mon Sep 17 00:00:00 2001
From: Priti Desai <pdesai@us.ibm.com>
Date: Fri, 26 Jun 2020 12:00:52 -0700
Subject: [PATCH] refactor GetPipelineConditionStatus
Adding an attribute to PipelineRunState to tell whether a PipelineTask
will not be run because (1) its condition checks failed or (2) one of
the parents conditions failed or (3) pipeline is in stopping state
Instead of explicitly checking (3) in GetPipelineConditionStatus, moved this
check to the IsSkipped function which is used by both the scheduler and
while determining pipeline status.
---
.../resources/pipelinerunresolution.go | 38 +++++++++----------
.../resources/pipelinerunresolution_test.go | 28 +++++++++++++-
test/pipelinefinally_test.go | 2 +-
3 files changed, 47 insertions(+), 21 deletions(-)
diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
index 0887a6561bf..842ebb57fc1 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,12 +159,7 @@ 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
+ // Skip the PipelineTasks if pipeline is in stopping state
if inGraph(t.PipelineTask.Name, d) && state.IsStopping(d) {
return true
}
@@ -177,7 +170,7 @@ func (t ResolvedPipelineRunTask) IsSkipped(root string, state PipelineRunState,
node := d.Nodes[t.PipelineTask.Name]
if inGraph(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
}
}
@@ -265,7 +258,7 @@ 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 t.IsSuccessful() || t.IsSkipped(state, d) {
tasks = append(tasks, t.PipelineTask.Name)
}
}
@@ -283,7 +276,7 @@ func (state PipelineRunState) checkTasksDone(d *dag.Graph) (isDone bool) {
// 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,7 +298,6 @@ 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 {
@@ -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 inGraph(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..50b538f209f 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))
}
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) {