From 61d05a7357f008aeb25cc58189259f412ae1714e Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Mon, 20 Mar 2023 15:28:59 +0000 Subject: [PATCH] don't return validation error when taskrun failed/skipped This commit aims to fix #6383, #6139 and supports #3749. This commits skip the validation if the taskrun is not successful (e.g. failed or skipped) and omit the pipelinerun coresponding result. This way the skipped taskrun won't get validation error for pipelinerun. And the pipelinerun error won't be overwritten by the validation error. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- .../v1beta1/pipelineruns/6139-regression.yaml | 45 ++++++++++ pkg/reconciler/pipelinerun/pipelinerun.go | 2 +- pkg/reconciler/pipelinerun/resources/apply.go | 26 +++--- .../pipelinerun/resources/apply_test.go | 42 ++++++++- test/pipelinerun_test.go | 86 ++++++++++++++++++- 5 files changed, 186 insertions(+), 15 deletions(-) create mode 100644 examples/v1beta1/pipelineruns/6139-regression.yaml diff --git a/examples/v1beta1/pipelineruns/6139-regression.yaml b/examples/v1beta1/pipelineruns/6139-regression.yaml new file mode 100644 index 00000000000..f2a73f03299 --- /dev/null +++ b/examples/v1beta1/pipelineruns/6139-regression.yaml @@ -0,0 +1,45 @@ +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + name: pipelinerun-test +spec: + pipelineSpec: + params: + - name: say-hello + default: 'false' + tasks: + - name: hello + taskSpec: + results: + - name: result-one + steps: + - image: alpine + script: | + #!/bin/sh + echo "Hello world!" + echo -n "RES1" > $(results.result-one.path) + + - name: goodbye + runAfter: + - hello + taskSpec: + results: + - name: result-two + steps: + - image: alpine + script: | + #!/bin/sh + echo "Goodbye world!" + echo -n "RES2" > $(results.result-two.path) + when: + - input: $(params.say-hello) + operator: in + values: ["true"] + + results: + - name: result-hello + description: Result one + value: '$(tasks.hello.results.result-one)' + - name: result-goodbye + description: Result two + value: '$(tasks.goodbye.results.result-two)' diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 80e550dd63b..14a0b2db9e1 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -687,7 +687,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get pr.Status.SkippedTasks = pipelineRunFacts.GetSkippedTasks() if after.Status == corev1.ConditionTrue || after.Status == corev1.ConditionFalse { pr.Status.PipelineResults, err = resources.ApplyTaskResultsToPipelineResults(ctx, pipelineSpec.Results, - pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pr.Status.SkippedTasks) + pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pipelineRunFacts.GetPipelineTaskStatus()) if err != nil { pr.Status.MarkFailed(ReasonFailedValidation, err.Error()) return err diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go index edcec080901..8dd02c04804 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -338,13 +338,9 @@ func ApplyTaskResultsToPipelineResults( results []v1beta1.PipelineResult, taskRunResults map[string][]v1beta1.TaskRunResult, customTaskResults map[string][]v1beta1.CustomRunResult, - skippedTasks []v1beta1.SkippedTask) ([]v1beta1.PipelineRunResult, error) { + taskstatus map[string]string) ([]v1beta1.PipelineRunResult, error) { var runResults []v1beta1.PipelineRunResult var invalidPipelineResults []string - skippedTaskNames := map[string]bool{} - for _, t := range skippedTasks { - skippedTaskNames[t.Name] = true - } stringReplacements := map[string]string{} arrayReplacements := map[string][]string{} @@ -366,11 +362,7 @@ func ApplyTaskResultsToPipelineResults( continue } variableParts := strings.Split(variable, ".") - // if the referenced task is skipped, we should also skip the results replacements - if _, ok := skippedTaskNames[variableParts[1]]; ok { - validPipelineResult = false - continue - } + if (variableParts[0] != v1beta1.ResultTaskPart && variableParts[0] != v1beta1.ResultFinallyPart) || variableParts[2] != v1beta1.ResultResultPart { validPipelineResult = false invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name) @@ -406,6 +398,13 @@ func ApplyTaskResultsToPipelineResults( } else if resultValue := runResultValue(taskName, resultName, customTaskResults); resultValue != nil { stringReplacements[variable] = *resultValue } else { + // if the task is not successful (e.g. skipped or failed) and the results is missing, don't return error + if status, ok := taskstatus[PipelineTaskStatusPrefix+taskName+PipelineTaskStatusSuffix]; ok { + if status != v1beta1.TaskRunReasonSuccessful.String() { + validPipelineResult = false + continue + } + } // referred result name is not existent invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name) validPipelineResult = false @@ -423,6 +422,13 @@ func ApplyTaskResultsToPipelineResults( validPipelineResult = false } } else { + // if the task is not successful (e.g. skipped or failed) and the results is missing, don't return error + if status, ok := taskstatus[PipelineTaskStatusPrefix+taskName+PipelineTaskStatusSuffix]; ok { + if status != v1beta1.TaskRunReasonSuccessful.String() { + validPipelineResult = false + continue + } + } // referred result name is not existent invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name) validPipelineResult = false diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go index 45521ec70fd..a350d98d18e 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/pkg/reconciler/pipelinerun/resources/apply_test.go @@ -3512,6 +3512,7 @@ func TestApplyTaskResultsToPipelineResults_Success(t *testing.T) { results []v1beta1.PipelineResult taskResults map[string][]v1beta1.TaskRunResult runResults map[string][]v1beta1.CustomRunResult + taskstatus map[string]string skippedTasks []v1beta1.SkippedTask expectedResults []v1beta1.PipelineRunResult }{{ @@ -3789,13 +3790,47 @@ func TestApplyTaskResultsToPipelineResults_Success(t *testing.T) { Value: *v1beta1.NewStructuredValues("rae"), }}, }, - skippedTasks: []v1beta1.SkippedTask{{ - Name: "skippedTask", + taskstatus: map[string]string{resources.PipelineTaskStatusPrefix + "skippedTask" + resources.PipelineTaskStatusSuffix: resources.PipelineTaskStateNone}, + expectedResults: nil, + }, { + description: "unsuccessful-taskrun-no-results", + results: []v1beta1.PipelineResult{{ + Name: "foo", + Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo)"), }}, + taskResults: map[string][]v1beta1.TaskRunResult{}, + taskstatus: map[string]string{resources.PipelineTaskStatusPrefix + "pt1" + resources.PipelineTaskStatusSuffix: v1beta1.TaskRunReasonFailed.String()}, expectedResults: nil, + }, { + description: "unsuccessful-taskrun-no-returned-result-object-ref", + results: []v1beta1.PipelineResult{{ + Name: "foo", + Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo.key1)"), + }}, + taskResults: map[string][]v1beta1.TaskRunResult{}, + taskstatus: map[string]string{resources.PipelineTaskStatusPrefix + "pt1" + resources.PipelineTaskStatusSuffix: v1beta1.TaskRunReasonFailed.String()}, + expectedResults: nil, + }, { + description: "unsuccessful-taskrun-with-results", + results: []v1beta1.PipelineResult{{ + Name: "foo", + Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo[*])"), + }}, + taskResults: map[string][]v1beta1.TaskRunResult{ + "pt1": { + { + Name: "foo", + Value: *v1beta1.NewStructuredValues("do", "rae", "mi"), + }, + }}, + taskstatus: map[string]string{resources.PipelineTaskStatusPrefix + "pt1" + resources.PipelineTaskStatusSuffix: v1beta1.TaskRunReasonFailed.String()}, + expectedResults: []v1beta1.PipelineRunResult{{ + Name: "foo", + Value: *v1beta1.NewStructuredValues("do", "rae", "mi"), + }}, }} { t.Run(tc.description, func(t *testing.T) { - received, err := resources.ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.skippedTasks) + received, err := resources.ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.taskstatus) if err != nil { t.Errorf("Got unecpected error:%v", err) } @@ -4014,6 +4049,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { received, err := resources.ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, nil /*skipped tasks*/) if err == nil { t.Errorf("Expect error but got nil") + return } if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" { diff --git a/test/pipelinerun_test.go b/test/pipelinerun_test.go index a83cd6b7b34..2875ce90a00 100644 --- a/test/pipelinerun_test.go +++ b/test/pipelinerun_test.go @@ -744,7 +744,7 @@ metadata: spec: params: - name: HELLO - value: "Hello World!" + value: "Hello World!" pipelineRef: name: %s `, namespace, pipelineName)) @@ -952,3 +952,87 @@ func getLimitRange(name, namespace, resourceCPU, resourceMemory, resourceEphemer }, } } + +func TestPipelineRunTaskFailed(t *testing.T) { + ctx := context.Background() + ctx, cancel := context.WithCancel(ctx) + defer cancel() + c, namespace := setup(ctx, t) + + knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf) + defer tearDown(ctx, t, c, namespace) + + taskName := helpers.ObjectNameForTest(t) + pipelineName := helpers.ObjectNameForTest(t) + prName := helpers.ObjectNameForTest(t) + + t.Logf("Creating Task, Pipeline, and Pending PipelineRun %s in namespace %s", prName, namespace) + + if _, err := c.V1beta1TaskClient.Create(ctx, parse.MustParseV1beta1Task(t, fmt.Sprintf(` +metadata: + name: %s + namespace: %s +spec: + steps: + - image: ubuntu + command: ['/bin/bash'] + args: ['-c', 'echo hello, world'] +`, taskName, namespace)), metav1.CreateOptions{}); err != nil { + t.Fatalf("Failed to create Task `%s`: %s", taskName, err) + } + + if _, err := c.V1beta1PipelineClient.Create(ctx, parse.MustParseV1beta1Pipeline(t, fmt.Sprintf(` +metadata: + name: %s + namespace: %s +spec: + tasks: + - name: task + taskRef: + name: %s +`, pipelineName, namespace, taskName)), metav1.CreateOptions{}); err != nil { + t.Fatalf("Failed to create Pipeline `%s`: %s", pipelineName, err) + } + + pipelineRun, err := c.V1beta1PipelineRunClient.Create(ctx, parse.MustParseV1beta1PipelineRun(t, fmt.Sprintf(` +metadata: + name: %s + namespace: %s +spec: + pipelineSpec: + results: + - name: abc + value: "$(tasks.xxx.results.abc)" + tasks: + - name: xxx + taskSpec: + results: + - name: abc + steps: + - name: update-sa + image: bash:latest + script: | + echo 'test' > $(results.abc.path) + exit 1 +`, prName, namespace)), metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create PipelineRun `%s`: %s", prName, err) + } + // Wait for the PipelineRun to fail. + if err := WaitForPipelineRunState(ctx, c, prName, timeout, PipelineRunFailed(prName), "PipelineRunFailed", v1beta1Version); err != nil { + t.Fatalf("Waiting for PipelineRun to fail: %v", err) + } + + pipelineRun, err = c.V1beta1PipelineRunClient.Get(ctx, prName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Error getting PipelineRun %s: %s", prName, err) + } + + if pipelineRun.Status.GetCondition(apis.ConditionSucceeded).IsTrue() { + t.Errorf("Expected PipelineRun to fail but found condition: %s", pipelineRun.Status.GetCondition(apis.ConditionSucceeded)) + } + expectedMessage := "Tasks Completed: 1 (Failed: 1, Cancelled 0), Skipped: 0" + if pipelineRun.Status.GetCondition(apis.ConditionSucceeded).Message != expectedMessage { + t.Errorf("Expected PipelineRun to fail with condition message: %s but got: %s", expectedMessage, pipelineRun.Status.GetCondition(apis.ConditionSucceeded).Message) + } +}