From 6712c130568e1087006f62b82afd44b97506eb3c Mon Sep 17 00:00:00 2001 From: Andrea Frittoli Date: Sun, 7 Jun 2020 11:38:22 +0100 Subject: [PATCH] Set pipeline status when all tasks complete We used to set the pipeline status to failed as soon as the first task in the pipeline failed or was cancelled. As soon as the first task in the pipeline fails or is cancelled, we stop scheduling new tasks, as we did before, but we will report status Unknown until all Tasks are complete, with reason "stopping". This allows to: - the completion time at the same time that the status is set and avoid inconsistencies - wait until all tasks are complete before we cleanup the pipeline artifact storage, affinity assistant and record metrics - report the correct number of failed / cancelled tasks, as there may be more than one. Other tasks that were already running when the first failure happened may fail too - prepare the pipeline controller more complex workflows, where the controller may continue working scheduling after failures Add test coverage for isSkipped and extend the pipelineresolution module unit test coverage to capture more input pipeline state. The DAG / GetNextTasks functions have not been touched at all. The pipeline run reconciler, when the pipeline run is in stopping mode, stops asking for next tasks to run. Once all running tasks finish, the pipeline run finally gets in failed state. When the pipeline run is in stopping mode, tasks that have not been started yet are also counted as skipped in the status message reported to the user. --- docs/events.md | 2 + .../pipeline/v1beta1/pipelinerun_types.go | 3 + pkg/reconciler/pipelinerun/pipelinerun.go | 63 +- .../resources/pipelinerunresolution.go | 141 +++-- .../resources/pipelinerunresolution_test.go | 545 +++++++++++++++--- 5 files changed, 592 insertions(+), 162 deletions(-) diff --git a/docs/events.md b/docs/events.md index add381754cc..a0d9302d272 100644 --- a/docs/events.md +++ b/docs/events.md @@ -16,6 +16,7 @@ No events are emitted for `Conditions` today (https://github.com/tektoncd/pipeli ## TaskRuns `TaskRun` events are generated for the following `Reasons`: + - `Started`: this is triggered the first time the `TaskRun` is picked by the reconciler from its work queue, so it only happens if web-hook validation was successful. Note that this event does not imply that a step started executing, @@ -32,6 +33,7 @@ No events are emitted for `Conditions` today (https://github.com/tektoncd/pipeli ## PipelineRuns `PipelineRun` events are generated for the following `Reasons`: + - `Succeeded`: this is triggered once all `Tasks` reachable via the DAG are executed successfully. - `Failed`: this is triggered if the `PipelineRun` is completed, but not diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go index e8a102b9ca4..2b259e4f44f 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go @@ -231,6 +231,9 @@ const ( PipelineRunReasonCancelled PipelineRunReason = "Cancelled" // PipelineRunReasonTimedOut is the reason set when the PipelineRun has timed out PipelineRunReasonTimedOut PipelineRunReason = "PipelineRunTimeout" + // ReasonStopping indicates that no new Tasks will be scheduled by the controller, and the + // pipeline will stop once all running tasks complete their work + PipelineRunReasonStopping PipelineRunReason = "PipelineRunStopping" ) func (t PipelineRunReason) String() string { diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 6b5f44e06b4..352d759abf3 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -426,9 +426,51 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err } } + as, err := artifacts.InitializeArtifactStorage(c.Images, pr, pipelineSpec, c.KubeClientSet, logger) + if err != nil { + logger.Infof("PipelineRun failed to initialize artifact storage %s", pr.Name) + return err + } + + // When the pipeline run is stopping, we don't schedule any new task and only + // wait for all running tasks to complete and report their status + if !pipelineState.IsStopping() { + err = c.runNextSchedulableTask(ctx, pr, d, pipelineState, as) + if err != nil { + return err + } + } + + before := pr.Status.GetCondition(apis.ConditionSucceeded) + after := resources.GetPipelineConditionStatus(pr, pipelineState, logger, d) + switch after.Status { + case corev1.ConditionTrue: + pr.Status.MarkSucceeded(after.Reason, after.Message) + case corev1.ConditionFalse: + pr.Status.MarkFailed(after.Reason, after.Message) + case corev1.ConditionUnknown: + pr.Status.MarkRunning(after.Reason, after.Message) + } + // Read the condition the way it was set by the Mark* helpers + after = pr.Status.GetCondition(apis.ConditionSucceeded) + events.Emit(recorder, before, after, pr) + + pr.Status.TaskRuns = getTaskRunsStatus(pr, pipelineState) + c.Logger.Infof("PipelineRun %s status is being set to %s", pr.Name, after) + return nil +} + +// runNextSchedulableTask gets the next schedulable Tasks from the dag based on the current +// pipeline run state, and starts them +func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.PipelineRun, d *dag.Graph, pipelineState resources.PipelineRunState, as artifacts.ArtifactStorageInterface) error { + + logger := logging.FromContext(ctx) + recorder := controller.GetEventRecorder(ctx) + candidateTasks, err := dag.GetSchedulable(d, pipelineState.SuccessfulPipelineTaskNames()...) if err != nil { logger.Errorf("Error getting potential next tasks for valid pipelinerun %s: %v", pr.Name, err) + return nil } nextRprts := pipelineState.GetNextTasks(candidateTasks) @@ -440,13 +482,6 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err } resources.ApplyTaskResults(nextRprts, resolvedResultRefs) - var as artifacts.ArtifactStorageInterface - - if as, err = artifacts.InitializeArtifactStorage(c.Images, pr, pipelineSpec, c.KubeClientSet, logger); err != nil { - logger.Infof("PipelineRun failed to initialize artifact storage %s", pr.Name) - return err - } - for _, rprt := range nextRprts { if rprt == nil { continue @@ -467,20 +502,6 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err } } } - before := pr.Status.GetCondition(apis.ConditionSucceeded) - after := resources.GetPipelineConditionStatus(pr, pipelineState, logger, d) - switch after.Status { - case corev1.ConditionTrue: - pr.Status.MarkSucceeded(after.Reason, after.Message) - case corev1.ConditionFalse: - pr.Status.MarkFailed(after.Reason, after.Message) - case corev1.ConditionUnknown: - pr.Status.MarkRunning(after.Reason, after.Message) - } - events.Emit(recorder, before, after, pr) - - pr.Status.TaskRuns = getTaskRunsStatus(pr, pipelineState) - logger.Infof("PipelineRun %s status is being set to %s", pr.Name, after) return nil } diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 77c046f81dc..4a2b143f50b 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -127,6 +127,20 @@ func (t ResolvedPipelineRunTask) IsCancelled() bool { return c.IsFalse() && c.Reason == v1beta1.TaskRunReasonCancelled.String() } +// IsStarted returns true only if the PipelineRunTask itself has a TaskRun associated +func (t ResolvedPipelineRunTask) IsStarted() bool { + if t.TaskRun == nil { + return false + } + + c := t.TaskRun.Status.GetCondition(apis.ConditionSucceeded) + if c == nil { + return false + } + + return true +} + // ToMap returns a map that maps pipeline task name to the resolved pipeline run task func (state PipelineRunState) ToMap() map[string]*ResolvedPipelineRunTask { m := make(map[string]*ResolvedPipelineRunTask) @@ -160,6 +174,20 @@ func (state PipelineRunState) IsBeforeFirstTaskRun() bool { return true } +// IsStopping returns true if the PipelineRun won't be scheduling any new Task because +// at least one task already failed or was cancelled +func (state PipelineRunState) IsStopping() bool { + for _, t := range state { + if t.IsCancelled() { + return true + } + if t.IsFailure() { + return true + } + } + return false +} + // GetNextTasks will return the next ResolvedPipelineRunTasks to execute, which are the ones in the // list of candidateTasks which aren't yet indicated in state to be running. func (state PipelineRunState) GetNextTasks(candidateTasks map[string]struct{}) []*ResolvedPipelineRunTask { @@ -389,9 +417,9 @@ func GetTaskRunName(taskRunsStatus map[string]*v1beta1.PipelineRunTaskRunStatus, func GetPipelineConditionStatus(pr *v1beta1.PipelineRun, state PipelineRunState, logger *zap.SugaredLogger, dag *dag.Graph) *apis.Condition { // We have 4 different states here: // 1. Timed out -> Failed - // 2. Any one TaskRun has failed - >Failed. This should change with #1020 and #1023 + // 2. All tasks are done and at least one has failed or has been cancelled -> Failed // 3. All tasks are done or are skipped (i.e. condition check failed).-> Success - // 4. A Task or Condition is running right now or there are things left to run -> Running + // 4. A Task or Condition is running right now or there are things left to run -> Running if pr.IsTimedOut() { return &apis.Condition{ Type: apis.ConditionSucceeded, @@ -401,72 +429,91 @@ func GetPipelineConditionStatus(pr *v1beta1.PipelineRun, state PipelineRunState, } } - // A single failed task mean we fail the pipeline - for _, rprt := range state { - if rprt.IsCancelled() { - logger.Infof("TaskRun %s is cancelled, so PipelineRun %s is cancelled", rprt.TaskRunName, pr.Name) - return &apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionFalse, - Reason: v1beta1.PipelineRunReasonCancelled.String(), - Message: fmt.Sprintf("TaskRun %s has cancelled", rprt.TaskRun.Name), - } - } - - if rprt.IsFailure() { //IsDone ensures we have crossed the retry limit - logger.Infof("TaskRun %s has failed, so PipelineRun %s has failed, retries done: %b", rprt.TaskRunName, pr.Name, len(rprt.TaskRun.Status.RetriesStatus)) - return &apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionFalse, - Reason: v1beta1.PipelineRunReasonFailed.String(), - Message: fmt.Sprintf("TaskRun %s has failed", rprt.TaskRun.Name), - } - } - } - allTasks := []string{} - successOrSkipTasks := []string{} + withStatusTasks := []string{} skipTasks := int(0) + failedTasks := int(0) + cancelledTasks := int(0) + reason := v1beta1.PipelineRunReasonSuccessful.String() + stateAsMap := state.ToMap() + isStopping := state.IsStopping() // Check to see if all tasks are success or skipped + // + // The completion reason is also calculated here, but it will only be used + // if all tasks are completed. + // + // The pipeline run completion reason is set from the taskrun completion reason + // according to the following logic: + // + // - All successful: ReasonSucceeded + // - Some successful, some skipped: ReasonCompleted + // - Some cancelled, none failed: ReasonCancelled + // - At least one failed: ReasonFailed for _, rprt := range state { allTasks = append(allTasks, rprt.PipelineTask.Name) - if rprt.IsSuccessful() { - successOrSkipTasks = append(successOrSkipTasks, rprt.PipelineTask.Name) - } - if isSkipped(rprt, state.ToMap(), dag) { + switch { + case !rprt.IsStarted() && isStopping: + // If the pipeline is in stopping mode, all tasks that are not running + // already will be skipped. Otherwise these tasks end up in the + // incomplete count. + skipTasks++ + withStatusTasks = append(withStatusTasks, rprt.PipelineTask.Name) + case rprt.IsSuccessful(): + withStatusTasks = append(withStatusTasks, rprt.PipelineTask.Name) + case isSkipped(rprt, stateAsMap, dag): skipTasks++ - successOrSkipTasks = append(successOrSkipTasks, rprt.PipelineTask.Name) + withStatusTasks = append(withStatusTasks, rprt.PipelineTask.Name) + // At least one is skipped and no failure yet, mark as completed + if reason == v1beta1.PipelineRunReasonSuccessful.String() { + reason = v1beta1.PipelineRunReasonCompleted.String() + } + case rprt.IsCancelled(): + cancelledTasks++ + withStatusTasks = append(withStatusTasks, rprt.PipelineTask.Name) + if reason != v1beta1.PipelineRunReasonFailed.String() { + reason = v1beta1.PipelineRunReasonCancelled.String() + } + case rprt.IsFailure(): + withStatusTasks = append(withStatusTasks, rprt.PipelineTask.Name) + failedTasks++ + reason = v1beta1.PipelineRunReasonFailed.String() } } - if reflect.DeepEqual(allTasks, successOrSkipTasks) { - logger.Infof("All TaskRuns have finished for PipelineRun %s so it has finished", pr.Name) - reason := v1beta1.PipelineRunReasonSuccessful.String() - if skipTasks != 0 { - reason = v1beta1.PipelineRunReasonCompleted.String() + if reflect.DeepEqual(allTasks, withStatusTasks) { + status := corev1.ConditionTrue + if failedTasks > 0 || cancelledTasks > 0 { + status = corev1.ConditionFalse } - + logger.Infof("All TaskRuns have finished for PipelineRun %s so it has finished", pr.Name) return &apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionTrue, - Reason: reason, - Message: fmt.Sprintf("Tasks Completed: %d, Skipped: %d", len(successOrSkipTasks)-skipTasks, skipTasks), + Type: apis.ConditionSucceeded, + Status: status, + Reason: reason, + Message: fmt.Sprintf("Tasks Completed: %d (Failed: %d, Cancelled %d), Skipped: %d", + len(allTasks)-skipTasks, failedTasks, cancelledTasks, skipTasks), } } - // Hasn't timed out; no taskrun failed yet; and not all tasks have finished.... + // Hasn't timed out; not all tasks have finished.... // Must keep running then.... + if failedTasks > 0 || cancelledTasks > 0 { + reason = v1beta1.PipelineRunReasonStopping.String() + } else { + reason = v1beta1.PipelineRunReasonRunning.String() + } return &apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionUnknown, - Reason: v1beta1.PipelineRunReasonRunning.String(), - Message: fmt.Sprintf("Tasks Completed: %d, Incomplete: %d, Skipped: %d", len(successOrSkipTasks)-skipTasks, len(allTasks)-len(successOrSkipTasks), skipTasks), + Type: apis.ConditionSucceeded, + Status: corev1.ConditionUnknown, + Reason: reason, + Message: fmt.Sprintf("Tasks Completed: %d (Failed: %d, Cancelled %d), Incomplete: %d, Skipped: %d", + len(withStatusTasks)-skipTasks, failedTasks, cancelledTasks, len(allTasks)-len(withStatusTasks), skipTasks), } } // isSkipped returns true if a Task in a TaskRun will not be run either because -// its Condition Checks failed or because one of the parent tasks's conditions failed +// its Condition Checks failed or because one of the parent tasks' conditions failed // Note that this means isSkipped returns false if a conditionCheck is in progress func isSkipped(rprt *ResolvedPipelineRunTask, stateMap map[string]*ResolvedPipelineRunTask, d *dag.Graph) bool { // Taskrun not skipped if it already exists diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go index 294e3b285f5..686b9dd2437 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go @@ -298,7 +298,7 @@ var failedTaskConditionCheckState = TaskConditionCheckState{{ var conditionCheckSuccessNoTaskStartedState = PipelineRunState{{ PipelineTask: &pts[5], - TaskRunName: "pipeleinerun-conditionaltask", + TaskRunName: "pipelinerun-conditionaltask", TaskRun: nil, ResolvedTaskResources: &resources.ResolvedTaskResources{ TaskSpec: &task.Spec, @@ -308,7 +308,7 @@ var conditionCheckSuccessNoTaskStartedState = PipelineRunState{{ var conditionCheckStartedState = PipelineRunState{{ PipelineTask: &pts[5], - TaskRunName: "pipeleinerun-conditionaltask", + TaskRunName: "pipelinerun-conditionaltask", TaskRun: nil, ResolvedTaskResources: &resources.ResolvedTaskResources{ TaskSpec: &task.Spec, @@ -322,7 +322,7 @@ var conditionCheckStartedState = PipelineRunState{{ var conditionCheckFailedWithNoOtherTasksState = PipelineRunState{{ PipelineTask: &pts[5], - TaskRunName: "pipeleinerun-conditionaltask", + TaskRunName: "pipelinerun-conditionaltask", TaskRun: nil, ResolvedTaskResources: &resources.ResolvedTaskResources{ TaskSpec: &task.Spec, @@ -332,7 +332,7 @@ var conditionCheckFailedWithNoOtherTasksState = PipelineRunState{{ var conditionCheckFailedWithOthersPassedState = PipelineRunState{{ PipelineTask: &pts[5], - TaskRunName: "pipeleinerun-conditionaltask", + TaskRunName: "pipelinerun-conditionaltask", TaskRun: nil, ResolvedTaskResources: &resources.ResolvedTaskResources{ TaskSpec: &task.Spec, @@ -351,7 +351,7 @@ var conditionCheckFailedWithOthersPassedState = PipelineRunState{{ var conditionCheckFailedWithOthersFailedState = PipelineRunState{{ PipelineTask: &pts[5], - TaskRunName: "pipeleinerun-conditionaltask", + TaskRunName: "pipelinerun-conditionaltask", TaskRun: nil, ResolvedTaskResources: &resources.ResolvedTaskResources{ TaskSpec: &task.Spec, @@ -938,6 +938,244 @@ func TestIsDone(t *testing.T) { } } +func TestIsSkipped(t *testing.T) { + + tcs := []struct { + name string + taskName string + state PipelineRunState + expected bool + }{{ + name: "tasks-condition-passed", + taskName: "mytask1", + state: PipelineRunState{{ + PipelineTask: &pts[0], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: successTaskConditionCheckState, + }}, + expected: false, + }, { + name: "tasks-condition-failed", + taskName: "mytask1", + state: PipelineRunState{{ + PipelineTask: &pts[0], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: failedTaskConditionCheckState, + }}, + expected: true, + }, { + name: "tasks-multiple-conditions-passed-failed", + taskName: "mytask1", + state: PipelineRunState{{ + PipelineTask: &pts[0], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: TaskConditionCheckState{{ + ConditionCheckName: "myconditionCheck", + Condition: &condition, + ConditionCheck: v1beta1.NewConditionCheck(makeFailed(conditionChecks[0])), + }, { + ConditionCheckName: "myconditionCheck", + Condition: &condition, + ConditionCheck: v1beta1.NewConditionCheck(makeSucceeded(conditionChecks[0])), + }}, + }}, + expected: true, + }, { + name: "tasks-condition-running", + taskName: "mytask6", + state: conditionCheckStartedState, + expected: false, + }, { + name: "tasks-parent-condition-passed", + taskName: "mytask7", + state: PipelineRunState{{ + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: successTaskConditionCheckState, + }, { + PipelineTask: &pts[6], + }}, + expected: false, + }, { + name: "tasks-parent-condition-failed", + taskName: "mytask7", + state: PipelineRunState{{ + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: failedTaskConditionCheckState, + }, { + PipelineTask: &pts[6], + }}, + expected: true, + }, { + name: "tasks-parent-condition-running", + taskName: "mytask7", + state: PipelineRunState{{ + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: TaskConditionCheckState{{ + ConditionCheckName: "myconditionCheck", + Condition: &condition, + ConditionCheck: v1beta1.NewConditionCheck(makeStarted(conditionChecks[0])), + }}, + }, { + PipelineTask: &pts[6], + }}, + expected: false, + }, { + name: "tasks-failed", + taskName: "mytask1", + state: oneFailedState, + expected: false, + }, { + name: "tasks-passed", + taskName: "mytask1", + state: oneFinishedState, + expected: false, + }, { + name: "tasks-cancelled", + taskName: "mytask5", + state: taskCancelled, + expected: false, + }, { + name: "tasks-parent-failed", + taskName: "mytask7", + state: PipelineRunState{{ + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-mytask1", + TaskRun: makeFailed(trs[0]), + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + PipelineTask: &pts[6], // mytask7 runAfter mytask6 + TaskRunName: "pipelinerun-mytask2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + expected: false, + }, { + name: "tasks-parent-cancelled", + taskName: "mytask7", + state: PipelineRunState{{ + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-mytask1", + TaskRun: withCancelled(makeFailed(trs[0])), + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + PipelineTask: &pts[6], // mytask7 runAfter mytask6 + TaskRunName: "pipelinerun-mytask2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + expected: false, + }, { + name: "tasks-grandparent-failed", + taskName: "mytask10", + state: PipelineRunState{{ + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-mytask1", + TaskRun: makeFailed(trs[0]), + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + PipelineTask: &pts[6], // mytask7 runAfter mytask6 + TaskRunName: "pipelinerun-mytask2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask10", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask7"}, + }, // mytask10 runAfter mytask7 runAfter mytask6 + TaskRunName: "pipelinerun-mytask3", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + expected: false, + }, { + name: "tasks-parents-failed-passed", + taskName: "mytask8", + state: PipelineRunState{{ + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-mytask1", + TaskRun: makeSucceeded(trs[0]), + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + PipelineTask: &pts[0], + TaskRunName: "pipelinerun-mytask2", + TaskRun: makeFailed(trs[0]), + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + PipelineTask: &pts[7], // mytask8 runAfter mytask1, mytask6 + TaskRunName: "pipelinerun-mytask3", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + expected: false, + }} + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + dag, err := DagFromState(tc.state) + if err != nil { + t.Fatalf("Could not get a dag from the TC state %#v: %v", tc.state, err) + } + stateMap := tc.state.ToMap() + rprt := stateMap[tc.taskName] + if rprt == nil { + t.Fatalf("Could not get task %s from the state: %v", tc.taskName, tc.state) + } + isSkipped := isSkipped(rprt, stateMap, dag) + if d := cmp.Diff(isSkipped, tc.expected); d != "" { + t.Errorf("Didn't get expected isSkipped %s", diff.PrintWantGot(d)) + } + }) + } +} + func TestSuccessfulPipelineTaskNames(t *testing.T) { tcs := []struct { name string @@ -974,6 +1212,15 @@ func TestSuccessfulPipelineTaskNames(t *testing.T) { } } +func getExpectedMessage(status corev1.ConditionStatus, successful, incomplete, skipped, failed, cancelled int) string { + if status == corev1.ConditionFalse || status == corev1.ConditionTrue { + return fmt.Sprintf("Tasks Completed: %d (Failed: %d, Cancelled %d), Skipped: %d", + successful+failed+cancelled, failed, cancelled, skipped) + } + return fmt.Sprintf("Tasks Completed: %d (Failed: %d, Cancelled %d), Incomplete: %d, Skipped: %d", + successful+failed+cancelled, failed, cancelled, incomplete, skipped) +} + func TestGetPipelineConditionStatus(t *testing.T) { var taskRetriedState = PipelineRunState{{ @@ -985,98 +1232,201 @@ func TestGetPipelineConditionStatus(t *testing.T) { }, }} - tcs := []struct { - name string - state []*ResolvedPipelineRunTask - expectedStatus corev1.ConditionStatus - }{{ - name: "no-tasks-started", - state: noneStartedState, - expectedStatus: corev1.ConditionUnknown, - }, { - name: "one-task-started", - state: oneStartedState, - expectedStatus: corev1.ConditionUnknown, - }, { - name: "one-task-finished", - state: oneFinishedState, - expectedStatus: corev1.ConditionUnknown, - }, { - name: "one-task-failed", - state: oneFailedState, - expectedStatus: corev1.ConditionFalse, - }, { - name: "all-finished", - state: allFinishedState, - expectedStatus: corev1.ConditionTrue, - }, { - name: "one-retry-needed", - state: taskRetriedState, - expectedStatus: corev1.ConditionUnknown, - }, { - name: "condition-success-no-task started", - state: conditionCheckSuccessNoTaskStartedState, - expectedStatus: corev1.ConditionUnknown, - }, { - name: "condition-check-in-progress", - state: conditionCheckStartedState, - expectedStatus: corev1.ConditionUnknown, - }, { - name: "condition-failed-no-other-tasks", // 1 task pipeline with a condition that fails - state: conditionCheckFailedWithNoOtherTasksState, - expectedStatus: corev1.ConditionTrue, - }, { - name: "condition-failed-another-task-succeeded", // 1 task skipped due to condition, but others pass - state: conditionCheckFailedWithOthersPassedState, - expectedStatus: corev1.ConditionTrue, - }, { - name: "condition-failed-another-task-failed", // 1 task skipped due to condition, but others failed - state: conditionCheckFailedWithOthersFailedState, - expectedStatus: corev1.ConditionFalse, - }, { - name: "no-tasks-started", - state: noneStartedState, - expectedStatus: corev1.ConditionUnknown, - }, { - name: "one-task-started", - state: oneStartedState, - expectedStatus: corev1.ConditionUnknown, - }, { - name: "one-task-finished", - state: oneFinishedState, - expectedStatus: corev1.ConditionUnknown, - }, { - name: "one-task-failed", - state: oneFailedState, - expectedStatus: corev1.ConditionFalse, - }, { - name: "all-finished", - state: allFinishedState, - expectedStatus: corev1.ConditionTrue, - }, { - name: "one-retry-needed", - state: taskRetriedState, - expectedStatus: corev1.ConditionUnknown, + var taskCancelledFailed = PipelineRunState{{ + PipelineTask: &pts[4], + TaskRunName: "pipelinerun-mytask1", + TaskRun: withCancelled(makeFailed(trs[0])), + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }} + + // 6 Tasks, 4 that run in parallel in the beginning + // Of the 4, 1 passed, 1 cancelled, 2 failed + // 1 runAfter the passed one, currently running + // 1 runAfter the failed one, which is marked as incomplete + var taskMultipleFailuresSkipRunning = PipelineRunState{{ + TaskRunName: "task0taskrun", + PipelineTask: &pts[5], + TaskRun: makeSucceeded(trs[0]), + ResolvedConditionChecks: successTaskConditionCheckState, + }, { + TaskRunName: "runningTaskRun", // this is running + PipelineTask: &pts[6], + TaskRun: makeStarted(trs[1]), + }, { + TaskRunName: "failedTaskRun", // this failed + PipelineTask: &pts[0], + TaskRun: makeFailed(trs[0]), }, { - name: "task skipped due to condition failure in parent", - state: taskWithParentSkippedState, - expectedStatus: corev1.ConditionTrue, + TaskRunName: "skippedTaskRun", // skipped because parent failed + PipelineTask: &pts[7], // mytask8 runAfter mytask1 (failed), mytask6 (succeeded) + TaskRun: nil, }, { - name: "task with multiple parent tasks -> one of which is skipped", - state: taskWithMultipleParentsSkippedState, - expectedStatus: corev1.ConditionTrue, + TaskRunName: "anothertaskrun", // this failed + PipelineTask: &pts[1], + TaskRun: makeFailed(trs[1]), }, { - name: "task with grand parent task skipped", - state: taskWithGrandParentSkippedState, - expectedStatus: corev1.ConditionTrue, + TaskRunName: "taskrun", // this was cancelled + PipelineTask: &pts[4], + TaskRun: withCancelled(makeFailed(trs[0])), + }} + + var taskNotRunningWithSuccesfulParentsOneFailed = PipelineRunState{{ + TaskRunName: "task0taskrun", + PipelineTask: &pts[5], + TaskRun: makeSucceeded(trs[0]), + ResolvedConditionChecks: successTaskConditionCheckState, }, { - name: "task with grand parents; one parent failed", - state: taskWithGrandParentsOneFailedState, - expectedStatus: corev1.ConditionFalse, + TaskRunName: "notRunningTaskRun", // runAfter pts[5], not started yet + PipelineTask: &pts[6], + TaskRun: nil, }, { - name: "task with grand parents; one not run yet", - state: taskWithGrandParentsOneNotRunState, - expectedStatus: corev1.ConditionUnknown, + TaskRunName: "failedTaskRun", // this failed + PipelineTask: &pts[0], + TaskRun: makeFailed(trs[0]), + }} + + tcs := []struct { + name string + state []*ResolvedPipelineRunTask + expectedStatus corev1.ConditionStatus + expectedReason string + expectedSucceeded int + expectedIncomplete int + expectedSkipped int + expectedFailed int + expectedCancelled int + }{{ + name: "no-tasks-started", + state: noneStartedState, + expectedStatus: corev1.ConditionUnknown, + expectedReason: v1beta1.PipelineRunReasonRunning.String(), + expectedIncomplete: 2, + }, { + name: "one-task-started", + state: oneStartedState, + expectedStatus: corev1.ConditionUnknown, + expectedReason: v1beta1.PipelineRunReasonRunning.String(), + expectedIncomplete: 2, + }, { + name: "one-task-finished", + state: oneFinishedState, + expectedStatus: corev1.ConditionUnknown, + expectedReason: v1beta1.PipelineRunReasonRunning.String(), + expectedSucceeded: 1, + expectedIncomplete: 1, + }, { + name: "one-task-failed", + state: oneFailedState, + expectedStatus: corev1.ConditionFalse, + expectedReason: v1beta1.PipelineRunReasonFailed.String(), + expectedFailed: 1, + expectedSkipped: 1, + }, { + name: "all-finished", + state: allFinishedState, + expectedStatus: corev1.ConditionTrue, + expectedReason: v1beta1.PipelineRunReasonSuccessful.String(), + expectedSucceeded: 2, + }, { + name: "one-retry-needed", + state: taskRetriedState, + expectedStatus: corev1.ConditionUnknown, + expectedReason: v1beta1.PipelineRunReasonRunning.String(), + expectedIncomplete: 1, + }, { + name: "condition-success-no-task started", + state: conditionCheckSuccessNoTaskStartedState, + expectedStatus: corev1.ConditionUnknown, + expectedReason: v1beta1.PipelineRunReasonRunning.String(), + expectedIncomplete: 1, + }, { + name: "condition-check-in-progress", + state: conditionCheckStartedState, + expectedStatus: corev1.ConditionUnknown, + expectedReason: v1beta1.PipelineRunReasonRunning.String(), + expectedIncomplete: 1, + }, { + name: "condition-failed-no-other-tasks", // 1 task pipeline with a condition that fails + state: conditionCheckFailedWithNoOtherTasksState, + expectedStatus: corev1.ConditionTrue, + expectedReason: v1beta1.PipelineRunReasonCompleted.String(), + expectedSkipped: 1, + expectedIncomplete: 1, + }, { + name: "condition-failed-another-task-succeeded", // 1 task skipped due to condition, but others pass + state: conditionCheckFailedWithOthersPassedState, + expectedStatus: corev1.ConditionTrue, + expectedReason: v1beta1.PipelineRunReasonCompleted.String(), + expectedSucceeded: 1, + expectedSkipped: 1, + }, { + name: "condition-failed-another-task-failed", // 1 task skipped due to condition, but others failed + state: conditionCheckFailedWithOthersFailedState, + expectedStatus: corev1.ConditionFalse, + expectedReason: v1beta1.PipelineRunReasonFailed.String(), + expectedFailed: 1, + expectedSkipped: 1, + }, { + name: "task skipped due to condition failure in parent", + state: taskWithParentSkippedState, + expectedStatus: corev1.ConditionTrue, + expectedReason: v1beta1.PipelineRunReasonCompleted.String(), + expectedSkipped: 2, + }, { + name: "task with multiple parent tasks -> one of which is skipped", + state: taskWithMultipleParentsSkippedState, + expectedStatus: corev1.ConditionTrue, + expectedReason: v1beta1.PipelineRunReasonCompleted.String(), + expectedSkipped: 2, + expectedSucceeded: 1, + }, { + name: "task with grand parent task skipped", + state: taskWithGrandParentSkippedState, + expectedStatus: corev1.ConditionTrue, + expectedReason: v1beta1.PipelineRunReasonCompleted.String(), + expectedSkipped: 3, + expectedSucceeded: 1, + }, { + name: "task with grand parents; one parent failed", + state: taskWithGrandParentsOneFailedState, + expectedStatus: corev1.ConditionFalse, + expectedReason: v1beta1.PipelineRunReasonFailed.String(), + expectedSucceeded: 1, + expectedSkipped: 2, + expectedFailed: 1, + }, { + name: "task with grand parents; one not run yet", + state: taskWithGrandParentsOneNotRunState, + expectedStatus: corev1.ConditionUnknown, + expectedReason: v1beta1.PipelineRunReasonRunning.String(), + expectedSucceeded: 1, + expectedIncomplete: 3, + }, { + name: "task that was cancelled", + state: taskCancelledFailed, + expectedReason: v1beta1.PipelineRunReasonCancelled.String(), + expectedStatus: corev1.ConditionFalse, + expectedCancelled: 1, + }, { + name: "task with multiple failures; one cancelled", + state: taskMultipleFailuresSkipRunning, + expectedReason: v1beta1.PipelineRunReasonStopping.String(), + expectedStatus: corev1.ConditionUnknown, + expectedSucceeded: 1, + expectedFailed: 2, + expectedIncomplete: 1, + expectedCancelled: 1, + expectedSkipped: 1, + }, { + name: "task not started with passed parent; one failed", + state: taskNotRunningWithSuccesfulParentsOneFailed, + expectedReason: v1beta1.PipelineRunReasonFailed.String(), + expectedStatus: corev1.ConditionFalse, + expectedSucceeded: 1, + expectedFailed: 1, + expectedSkipped: 1, }} for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { @@ -1086,8 +1436,15 @@ func TestGetPipelineConditionStatus(t *testing.T) { t.Fatalf("Unexpected error while buildig DAG for state %v: %v", tc.state, err) } c := GetPipelineConditionStatus(pr, tc.state, zap.NewNop().Sugar(), dag) - if c.Status != tc.expectedStatus { - t.Fatalf("Expected to get status %s but got %s for state %v", tc.expectedStatus, c.Status, tc.state) + wantCondition := &apis.Condition{ + Type: apis.ConditionSucceeded, + Status: tc.expectedStatus, + Reason: tc.expectedReason, + Message: getExpectedMessage(tc.expectedStatus, tc.expectedSucceeded, + tc.expectedIncomplete, tc.expectedSkipped, tc.expectedFailed, tc.expectedCancelled), + } + if d := cmp.Diff(wantCondition, c); d != "" { + t.Fatalf("Mismatch in condition %s", diff.PrintWantGot(d)) } }) }