From b939af1707d450e108c97c39fa99257ccae3c209 Mon Sep 17 00:00:00 2001 From: Dibyo Mukherjee Date: Wed, 26 Jun 2019 17:14:52 -0400 Subject: [PATCH] Do not fail pipelinerun if a taskrun is skipped due to failed condition 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) --- .../v1alpha1/pipelinerun/pipelinerun.go | 17 ++------ .../resources/conditionresolution.go | 43 +++++-------------- .../resources/pipelinerunresolution.go | 30 ++++++++++--- 3 files changed, 37 insertions(+), 53 deletions(-) diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go index c1cee466337..d422f8d9643 100644 --- 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,7 +452,7 @@ 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() @@ -471,7 +460,7 @@ func updateTaskRunsStatus(pr *v1alpha1.PipelineRun, pipelineState []*resources.R 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 index bb443255b6c..3eaddc813ce 100644 --- 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 index c111f11d973..cddf33a6df6 100644 --- 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)