Do not fail pipelinerun if a taskrun is skipped due to failed conditi…

…on checks Currently, a PipelineRun is marked failed if any TaskRun fails i.e its `ConditionSuceeded` is false. This commit changes the behavior to not fail the pipeline ONLY for those taskruns which are marked failed due to a condition check failure (i.e. the taskrun was skipped). Also, fixes a bug where pipelineruns with only one task with a failing condition would be running forever (until the timeout)
tektoncd · Jul 1, 2019 · b939af1 · b939af1
1 parent dc750ff
commit b939af1
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 53 deletions.
diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go
@@ -64,9 +64,6 @@ const (
 	// ReasonInvalidGraph indicates that the reason for the failure status is that the
 	// associated Pipeline is an invalid graph (a.k.a wrong order, cycle, …)
 	ReasonInvalidGraph = "PipelineInvalidGraph"
-	// ReasonConditionCheckFailed indicates that the reason for the failure status is that the
-	// condition check associated to the pipeline task evaluated to false
-	ReasonConditionCheckFailed = "ConditionCheckFailed"
 	// pipelineRunAgentName defines logging agent name for PipelineRun Controller
 	pipelineRunAgentName = "pipeline-controller"
 	// pipelineRunControllerName defines name for PipelineRun Controller
@@ -408,18 +405,10 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
 					c.Recorder.Eventf(pr, corev1.EventTypeWarning, "TaskRunCreationFailed", "Failed to create TaskRun %q: %v", rprt.TaskRunName, err)
 					return xerrors.Errorf("error creating TaskRun called %s for PipelineTask %s from PipelineRun %s: %w", rprt.TaskRunName, rprt.PipelineTask.Name, pr.Name, err)
 				}
-			} else {
-				hasStarted := true
-				for _, j := range rprt.ResolvedConditionChecks {
-					if j.ConditionCheck == nil {
-						hasStarted = false
-					}
-				}
-				if !hasStarted {
+			} else if !rprt.ResolvedConditionChecks.HasStarted(){
 					for _, rcc := range rprt.ResolvedConditionChecks {
 						rcc.ConditionCheck, err = c.makeConditionCheckContainer(c.Logger, rprt,  rcc, pr)
 					}
-				}
 			}
 		}
 	}
@@ -463,15 +452,15 @@ func updateTaskRunsStatus(pr *v1alpha1.PipelineRun, pipelineState []*resources.R
 				}
 			}
 			prtrs.ConditionChecks = cStatus
-			if !rprt.ResolvedConditionChecks.IsInProgress() && !rprt.ResolvedConditionChecks.IsSuccess() {
+			if rprt.ResolvedConditionChecks.IsComplete() && !rprt.ResolvedConditionChecks.IsSuccess() {
 				if prtrs.Status == nil {
 					prtrs.Status = &v1alpha1.TaskRunStatus{}
 					prtrs.Status.InitializeConditions()
 				}
 				prtrs.Status.SetCondition(&apis.Condition{
 						Type:               apis.ConditionSucceeded,
 						Status:             corev1.ConditionFalse,
-						Reason:             ReasonConditionCheckFailed,
+						Reason:             resources.ReasonConditionCheckFailed,
 						Message:            fmt.Sprintf("ConditionChecks failed for Task %s in PipelineRun %s", rprt.TaskRunName, pr.Name),
 				})
 			}

diff --git a/pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go b/pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go
@@ -17,39 +17,23 @@ type ResolvedConditionCheck struct {
 
 type TaskConditionCheckState []*ResolvedConditionCheck
 
-func (state TaskConditionCheckState) IsInProgress() bool {
-	if len(state) <= 0 {
-		return false
-	}
-	for _, rcc := range state {
-		if rcc == nil {
-			// Error?
-		}
-		if rcc.ConditionCheck == nil {
-			return false
-		}
-		status := rcc.ConditionCheck.Status.GetCondition(apis.ConditionSucceeded)
-		//logger.Warnf("STATUS BLOB: %+v", rcc.ConditionCheck.Status)
-		//logger.Warnf("STATUS IS %v", status)
-		if status == nil { // object exists but status.Conditions does not exist. So, in progress
-			return true
+func (state TaskConditionCheckState) HasStarted() bool {
+	hasStarted := true
+	for _, j := range state {
+		if j.ConditionCheck == nil {
+			hasStarted = false
 		}
 	}
-	return false
+	return hasStarted
 }
 
 func (state TaskConditionCheckState) IsComplete() bool{
-	if len(state) <= 0 {
-		return true
+	if !state.HasStarted() {
+		return false
 	}
 	isDone := true
 	for _, rcc := range state {
-		if rcc == nil || rcc.ConditionCheck == nil {
-			return false // TODO: Probably want to return some error message as well
-		}
-		// v0: Assume all condition containers need to succeed for the task to be successful
-		status := rcc.ConditionCheck.Status.GetCondition(apis.ConditionSucceeded)
-		isDone = isDone && status.IsTrue()
+		isDone = isDone && !rcc.ConditionCheck.Status.GetCondition(apis.ConditionSucceeded).IsUnknown()
 	}
 	return isDone
 }
@@ -60,14 +44,7 @@ func (state TaskConditionCheckState) IsSuccess() bool {
 	}
 	isSuccess := true
 	for _, rcc := range state {
-		if rcc == nil || rcc.ConditionCheck == nil {
-			// TODO: error
-			return false
-		}
-		// TODO: Err/Nil checking here
-		// Assumption right now is that there is one step for a conditional
-		exitCode := rcc.ConditionCheck.Status.Steps[0].Terminated.ExitCode
-		isSuccess = isSuccess && (exitCode == 0)
+		isSuccess = isSuccess && rcc.ConditionCheck.Status.GetCondition(apis.ConditionSucceeded).IsTrue()
 	}
 	return isSuccess
 }

diff --git a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go
@@ -48,6 +48,10 @@ const (
 	// ReasonTimedOut indicates that the PipelineRun has taken longer than its configured
 	// timeout
 	ReasonTimedOut = "PipelineRunTimeout"
+
+	// ReasonConditionCheckFailed indicates that the reason for the failure status is that the
+	// condition check associated to the pipeline task evaluated to false
+	ReasonConditionCheckFailed = "ConditionCheckFailed"
 )
 
 // ResolvedPipelineRunTask contains a Task and its associated TaskRun, if it
@@ -102,7 +106,7 @@ func (state PipelineRunState) GetNextTasks(candidateTasks map[string]v1alpha1.Pi
 		if _, ok := candidateTasks[t.PipelineTask.Name]; ok && t.TaskRun != nil {
 			status := t.TaskRun.Status.GetCondition(apis.ConditionSucceeded)
 			if status != nil && status.IsFalse() {
-				if !(t.TaskRun.IsCancelled() || status.Reason == "TaskRunCancelled") {
+				if !(t.TaskRun.IsCancelled() || status.Reason == "TaskRunCancelled" || status.Reason == ReasonConditionCheckFailed) {
 					if len(t.TaskRun.Status.RetriesStatus) < t.PipelineTask.Retries {
 						tasks = append(tasks, t)
 					}
@@ -276,7 +280,6 @@ func ResolvePipelineRun(
 					return nil, xerrors.Errorf("error retrieving Condition %s: %w", cName, err)
 				}
 				conditionCheckName := getConditionCheckName(pipelineRun.Status.TaskRuns, rprt.TaskRunName, cName)
-				logger.Warnf("!!!!!!!1ABOUT TO GET CONDITIONCHECK with name %s", conditionCheckName)
 				conditionCheck, err := getTaskRun(conditionCheckName) // TODO: Wrap this onto something else?
 				if err != nil {
 					if !errors.IsNotFound(err) {
@@ -303,7 +306,6 @@ func getConditionCheckName(taskRunStatus map[string]*v1alpha1.PipelineRunTaskRun
 	trStatus, ok := taskRunStatus[trName]
 	if ok && trStatus.ConditionChecks != nil {
 		for k,v := range trStatus.ConditionChecks {
-			// TODO: Support multiple conditions. We need a name parameter for conditions or use ordering to signify positions
 			if conditionName == v.ConditionName {
 				return k
 			}
@@ -327,7 +329,6 @@ func getTaskRunName(taskRunsStatus map[string]*v1alpha1.PipelineRunTaskRunStatus
 // updated with, based on the status of the TaskRuns in state.
 func GetPipelineConditionStatus(prName string, state PipelineRunState, logger *zap.SugaredLogger, startTime *metav1.Time,
 	pipelineTimeout *metav1.Duration) *apis.Condition {
-	allFinished := true
 	if !startTime.IsZero() && pipelineTimeout != nil {
 		timeout := pipelineTimeout.Duration
 		runtime := time.Since(startTime.Time)
@@ -343,10 +344,27 @@ func GetPipelineConditionStatus(prName string, state PipelineRunState, logger *z
 			}
 		}
 	}
+	allFinished := true
 	for _, rprt := range state {
 		if rprt.TaskRun == nil {
-			logger.Infof("TaskRun %s doesn't have a Status, so PipelineRun %s isn't finished", rprt.TaskRunName, prName)
-			allFinished = false
+
+			if rprt.ResolvedConditionChecks == nil {
+				logger.Infof("TaskRun %s doesn't have a Status, so PipelineRun %s isn't finished", rprt.TaskRunName, prName)
+				allFinished = false
+				continue
+			}
+			if !rprt.ResolvedConditionChecks.IsComplete() {
+				logger.Infof("ConditionChecks for TaskRun %s in progress, so PipelineRun %s isn't finished", rprt.TaskRunName, prName)
+				allFinished = false
+				continue
+			}
+			if rprt.ResolvedConditionChecks.IsSuccess() {
+				logger.Infof("ConditionChecks for TaskRun %s successful but TaskRun doesn't have a Status, so PipelineRun %s isn't finished", rprt.TaskRunName, prName)
+				allFinished = false
+				continue
+			}
+
+			logger.Info("ConditionChecks for TaskRun %s failed, so PipelineRun %s might be finished unless other TaskRuns are still running", rprt.TaskRunName, prName)
 			continue
 		}
 		c := rprt.TaskRun.Status.GetCondition(apis.ConditionSucceeded)