Repository: tektoncd/pipeline (language: go)

Issue:
Pipelineruns with failed task with result fails on `PipelineValidationFailed` reason, hiding the failure
Given the following `PipelineRun`:
```
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: test-results
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
```

# Expected Behavior

When a Task "with a results" in a PipelineRun fails, the PipelineRun result in a `PipelineValidationFailed` instead of saying that the `TaskRun` failed.

# Actual Behavior

When a Task "with a results" in a PipelineRun fails, the PipelineRun result in a the Pipeline failing *because* the Task failed.

# Steps to Reproduce the Problem

1. Apply the `PipelineRun` above
2. See the failure

# Additional Info

The result was written even 👼🏼 So *technically*, it has the result.

- Tekton Pipeline version: v0.45.0 (and below)

/kind bug
cc @pritidesai @jerop I think it "might be" by design, but it is confusing. 



Discussion / hints:
This is not by design, its a bug 😞 

The pipelineRun must not fail with `pipelineValidationFailed`. If the `taskRun` fails, the results are not initialized in taskRun status even if the termination message contains that particular result. We have a PR to support initializing results irrespective of the taskRun status - https://github.com/tektoncd/pipeline/pull/5750

@Yongxuanzhang has a fix for similar error in case of skipped tasks. Please help fix this issue if its not already fixed.
I ran into the same issue while creating a release for v0.46.0 when the `publish-images` failed and dependent tasks `report-bucket` and `publish-*` tasks were declared as `skipped`. These are not skipped tasks but they were not executed because of a failure of a parent task. These should be counted towards `incomplete` tasks.

```
"conditions": [
    {
      "lastTransitionTime": "2023-03-16T21:22:54Z",
      "message": "invalid pipelineresults [release-file release-file-no-tag], the referred results don't exist",
      "reason": "PipelineValidationFailed",
      "status": "False",
      "type": "Succeeded"
    }
  ],
```

```
 tkn --context dogfooding pr describe release-pipeline-v0-46-0-1
Name:           release-pipeline-v0-46-0-1
Namespace:      default
Pipeline Ref:   pipeline-release
Labels:
 tekton.dev/pipeline=pipeline-release
Annotations:
 chains.tekton.dev/signed=true

🌡️  Status

STARTED        DURATION   STATUS
18 hours ago   33m47s     Failed(PipelineValidationFailed)

💌 Message

invalid pipelineresults [release-file release-file-no-tag], the referred results don't exist
TaskRun(s) cancelled: release-pipeline-v0-46-0-1-publish-images

⏱  Timeouts
 Pipeline:   1h0m0s

⚓ Params

 NAME                   VALUE
 ∙ gitRevision          70a2d714dcbaefb952924edf6df0c217aff80548
 ∙ serviceAccountPath   release.json
 ∙ versionTag           v0.46.0
 ∙ releaseBucket        gs://tekton-releases/pipeline

📂 Workspaces

 NAME               SUB PATH   WORKSPACE BINDING
 ∙ release-secret   ---        Secret (secret=release-secret)
 ∙ workarea         ---        VolumeClaimTemplate

🗂  Taskruns

 NAME                                          TASK NAME        STARTED        DURATION   STATUS
 ∙ release-pipeline-v0-46-0-1-publish-images   publish-images   18 hours ago   24m25s     Failed
 ∙ release-pipeline-v0-46-0-1-unit-tests       unit-tests       18 hours ago   7m42s      Succeeded
 ∙ release-pipeline-v0-46-0-1-build            build            18 hours ago   5m15s      Succeeded
 ∙ release-pipeline-v0-46-0-1-precheck         precheck         18 hours ago   1m16s      Succeeded
 ∙ release-pipeline-v0-46-0-1-git-clone        git-clone        18 hours ago   23s        Succeeded

⏭️  Skipped Tasks

 NAME
 ∙ publish-to-bucket
 ∙ publish-to-bucket-latest
 ∙ report-bucket
```

https://github.com/tektoncd/pipeline/blob/086e76abb81d2d35e341553e501410790b0797d8/tekton/release-pipeline.yaml#L47-L52


This might help troubleshoot this issue:

```
  "skippedTasks": [
    {
      "name": "publish-to-bucket",
      "reason": "PipelineRun was stopping"
    },
    {
      "name": "publish-to-bucket-latest",
      "reason": "PipelineRun was stopping",
      "whenExpressions": [
        {
          "input": "true",
          "operator": "in",
          "values": [
            "true"
          ]
        }
      ]
    },
    {
      "name": "report-bucket",
      "reason": "PipelineRun was stopping"
    }
  ],
```
We are running 0.44 in dogfooding:

```
tkn --context dogfooding version
Client version: 0.29.1
Chains version: v0.14.0
Pipeline version: v0.44.0
Triggers version: v0.22.1
Dashboard version: v0.32.0
```
/assign


Fix patch:
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 50023b58bc2..fdb9b0aa2da 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


Test patch:
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)
+	}
+}