From f3d0f7ee0d04fba85078a64e69b223fb612cae48 Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Thu, 23 Nov 2023 00:52:16 +0000 Subject: [PATCH] don't return validation error when final tasks failed/skipped This commit closes #6139, in previous fix PR: https://github.com/tektoncd/pipeline/pull/6395, only dagTasks statuses are considered and final tasks are missing. This PR fixes this. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- examples/v1/pipelineruns/6139-regression.yaml | 21 ++- pkg/reconciler/pipelinerun/pipelinerun.go | 7 +- .../pipelinerun/resources/pipelinerunstate.go | 24 +++ .../resources/pipelinerunstate_test.go | 143 ++++++++++++++++++ 4 files changed, 193 insertions(+), 2 deletions(-) diff --git a/examples/v1/pipelineruns/6139-regression.yaml b/examples/v1/pipelineruns/6139-regression.yaml index 0d9baf321f1..4fc16c0ed1a 100644 --- a/examples/v1/pipelineruns/6139-regression.yaml +++ b/examples/v1/pipelineruns/6139-regression.yaml @@ -7,6 +7,8 @@ spec: params: - name: say-hello default: 'false' + - name: do-shutdown + default: 'false' tasks: - name: hello taskSpec: @@ -35,7 +37,21 @@ spec: - input: $(params.say-hello) operator: in values: ["true"] - + finally: + - name: shutdown + taskSpec: + results: + - name: result-three + steps: + - image: alpine + script: | + #!/bin/sh + echo "Shutdown world!" + echo -n "RES3" > $(results.result-three.path) + when: + - input: $(params.do-shutdown) + operator: in + values: ["true"] results: - name: result-hello description: Result one @@ -43,3 +59,6 @@ spec: - name: result-goodbye description: Result two value: '$(tasks.goodbye.results.result-two)' + - name: result-shutdown + description: Result shutdown + value: '$(finally.shutdown.results.result-three)' diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 1f8d60d4c97..936f018c554 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -752,9 +752,14 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel pr.Status.ChildReferences = pipelineRunFacts.GetChildReferences() pr.Status.SkippedTasks = pipelineRunFacts.GetSkippedTasks() + + taskStatus := pipelineRunFacts.GetPipelineTaskStatus() + finalTaskStatus := pipelineRunFacts.GetPipelineFinalTaskStatus() + taskStatus = kmap.Union(taskStatus, finalTaskStatus) + if after.Status == corev1.ConditionTrue || after.Status == corev1.ConditionFalse { pr.Status.Results, err = resources.ApplyTaskResultsToPipelineResults(ctx, pipelineSpec.Results, - pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pipelineRunFacts.GetPipelineTaskStatus()) + pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), taskStatus) if err != nil { pr.Status.MarkFailed(v1.PipelineRunReasonFailedValidation.String(), err.Error()) return err diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go index 088d757f136..487666f161f 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go @@ -542,6 +542,30 @@ func (facts *PipelineRunFacts) GetPipelineTaskStatus() map[string]string { return tStatus } +// GetPipelineTaskStatus returns the status of a PipelineFinalTask depending on its taskRun +func (facts *PipelineRunFacts) GetPipelineFinalTaskStatus() map[string]string { + // construct a map of tasks..status and its state + tStatus := make(map[string]string) + for _, t := range facts.State { + if facts.isFinalTask(t.PipelineTask.Name) { + var s string + switch { + // execution status is Succeeded when a task has succeeded condition with status set to true + case t.isSuccessful(): + s = v1.TaskRunReasonSuccessful.String() + // execution status is Failed when a task has succeeded condition with status set to false + case t.haveAnyRunsFailed(): + s = v1.TaskRunReasonFailed.String() + default: + // None includes skipped as well + s = PipelineTaskStateNone + } + tStatus[PipelineTaskStatusPrefix+t.PipelineTask.Name+PipelineTaskStatusSuffix] = s + } + } + return tStatus +} + // completedOrSkippedTasks returns a list of the names of all of the PipelineTasks in state // which have completed or skipped func (facts *PipelineRunFacts) completedOrSkippedDAGTasks() []string { diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go index 6f20dc46415..a87bee3ea4a 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go @@ -2377,6 +2377,149 @@ func TestPipelineRunFacts_GetPipelineTaskStatus(t *testing.T) { } } +func TestPipelineRunFacts_GetPipelineFinalTaskStatus(t *testing.T) { + tcs := []struct { + name string + state PipelineRunState + finalTasks []v1.PipelineTask + expectedStatus map[string]string + }{{ + name: "no-tasks-started", + state: noneStartedState, + finalTasks: []v1.PipelineTask{pts[0], pts[1]}, + expectedStatus: map[string]string{ + PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone, + PipelineTaskStatusPrefix + pts[1].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone, + }, + }, { + name: "one-task-started", + state: oneStartedState, + finalTasks: []v1.PipelineTask{pts[0], pts[1]}, + expectedStatus: map[string]string{ + PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone, + PipelineTaskStatusPrefix + pts[1].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone, + }, + }, { + name: "one-task-finished", + state: oneFinishedState, + finalTasks: []v1.PipelineTask{pts[0], pts[1]}, + expectedStatus: map[string]string{ + PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix: v1.TaskRunReasonSuccessful.String(), + PipelineTaskStatusPrefix + pts[1].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone, + }, + }, { + name: "one-task-failed", + state: oneFailedState, + finalTasks: []v1.PipelineTask{pts[0], pts[1]}, + expectedStatus: map[string]string{ + PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix: v1.TaskRunReasonFailed.String(), + PipelineTaskStatusPrefix + pts[1].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone, + }, + }, { + name: "all-finished", + state: allFinishedState, + finalTasks: []v1.PipelineTask{pts[0], pts[1]}, + expectedStatus: map[string]string{ + PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix: v1.TaskRunReasonSuccessful.String(), + PipelineTaskStatusPrefix + pts[1].Name + PipelineTaskStatusSuffix: v1.TaskRunReasonSuccessful.String(), + }, + }, { + name: "task-with-when-expressions-passed", + state: PipelineRunState{{ + PipelineTask: &pts[9], + TaskRunNames: []string{"pr-guard-succeeded-task-not-started"}, + TaskRuns: nil, + ResolvedTask: &resources.ResolvedTask{ + TaskSpec: &task.Spec, + }, + }}, + finalTasks: []v1.PipelineTask{pts[9]}, + expectedStatus: map[string]string{ + PipelineTaskStatusPrefix + pts[9].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone, + }, + }, { + name: "tasks-when-expression-failed-and-task-skipped", + state: PipelineRunState{{ + PipelineTask: &pts[10], + TaskRunNames: []string{"pr-guardedtask-skipped"}, + ResolvedTask: &resources.ResolvedTask{ + TaskSpec: &task.Spec, + }, + }}, + finalTasks: []v1.PipelineTask{pts[10]}, + expectedStatus: map[string]string{ + PipelineTaskStatusPrefix + pts[10].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone, + }, + }, { + name: "when-expression-task-with-parent-started", + state: PipelineRunState{{ + PipelineTask: &pts[0], + TaskRuns: []*v1.TaskRun{makeStarted(trs[0])}, + ResolvedTask: &resources.ResolvedTask{ + TaskSpec: &task.Spec, + }, + }, { + PipelineTask: &pts[11], + TaskRuns: nil, + ResolvedTask: &resources.ResolvedTask{ + TaskSpec: &task.Spec, + }, + }}, + finalTasks: []v1.PipelineTask{pts[0], pts[11]}, + expectedStatus: map[string]string{ + PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone, + PipelineTaskStatusPrefix + pts[11].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone, + }, + }, { + name: "task-cancelled", + state: taskCancelled, + finalTasks: []v1.PipelineTask{pts[4]}, + expectedStatus: map[string]string{ + PipelineTaskStatusPrefix + pts[4].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone, + }, + }, { + name: "one-skipped-one-failed-aggregate-status-must-be-failed", + state: PipelineRunState{{ + PipelineTask: &pts[10], + TaskRunNames: []string{"pr-guardedtask-skipped"}, + ResolvedTask: &resources.ResolvedTask{ + TaskSpec: &task.Spec, + }, + }, { + PipelineTask: &pts[0], + TaskRunNames: []string{"pipelinerun-mytask1"}, + TaskRuns: []*v1.TaskRun{makeFailed(trs[0])}, + ResolvedTask: &resources.ResolvedTask{ + TaskSpec: &task.Spec, + }, + }}, + finalTasks: []v1.PipelineTask{pts[0], pts[10]}, + expectedStatus: map[string]string{ + PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix: v1.PipelineRunReasonFailed.String(), + PipelineTaskStatusPrefix + pts[10].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone, + }, + }} + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + d, err := dag.Build(v1.PipelineTaskList(tc.finalTasks), v1.PipelineTaskList(tc.finalTasks).Deps()) + if err != nil { + t.Fatalf("Unexpected error while building graph for DAG tasks %v: %v", tc.finalTasks, err) + } + facts := PipelineRunFacts{ + State: tc.state, + FinalTasksGraph: d, + TimeoutsState: PipelineRunTimeoutsState{ + Clock: testClock, + }, + } + s := facts.GetPipelineFinalTaskStatus() + if d := cmp.Diff(tc.expectedStatus, s); d != "" { + t.Fatalf("Test failed: %s Mismatch in pipelineFinalTask execution state %s", tc.name, diff.PrintWantGot(d)) + } + }) + } +} + func TestPipelineRunFacts_GetSkippedTasks(t *testing.T) { for _, tc := range []struct { name string