Skip to content

Commit

Permalink
feat: support to extract failed task results
Browse files Browse the repository at this point in the history
If the task fails or succeeds, as long as the task result is initialized,
it can be successfully parsed and can be referenced by the final task.

This commit enables the failed task to produce the task results, and
the final task can reference it.

Please see detailed description in issue #5749

Co-authored-by: yang kewei <[email protected]>

Signed-off-by: pritidesai <[email protected]>
Signed-off-by: Priti Desai <[email protected]>
  • Loading branch information
cugykw authored and pritidesai committed May 18, 2023
1 parent cec2422 commit 660e742
Show file tree
Hide file tree
Showing 7 changed files with 233 additions and 6 deletions.
4 changes: 2 additions & 2 deletions docs/pipelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -1257,8 +1257,8 @@ spec:
`finally` tasks after all non-`finally` tasks are done.

The controller resolves task results before executing the `finally` task `discover-git-commit`. If the task
`clone-app-repo` failed or skipped with [when expression](#guard-task-execution-using-when-expressions) resulting in
uninitialized task result `commit`, the `finally` Task `discover-git-commit` will be included in the list of
`clone-app-repo` failed before initializing `commit` or skipped with [when expression](#guard-task-execution-using-when-expressions)
resulting in uninitialized task result `commit`, the `finally` Task `discover-git-commit` will be included in the list of
`skippedTasks` and continues executing rest of the `finally` tasks. The pipeline exits with `completion` instead of
`success` if a `finally` task is added to the list of `skippedTasks`.

Expand Down
4 changes: 2 additions & 2 deletions pkg/pod/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL

// populate task run CRD with results from sidecar logs
taskResults, _ := filterResults(sidecarLogResults, specResults)
if tr.IsSuccessful() {
if tr.IsDone() {
trs.TaskRunResults = append(trs.TaskRunResults, taskResults...)
}
}
Expand All @@ -209,7 +209,7 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL
}

taskResults, filteredResults := filterResults(results, specResults)
if tr.IsSuccessful() {
if tr.IsDone() {
trs.TaskRunResults = append(trs.TaskRunResults, taskResults...)
}
msg, err = createMessageFromResults(filteredResults)
Expand Down
34 changes: 34 additions & 0 deletions pkg/pod/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,40 @@ func TestMakeTaskRunStatus(t *testing.T) {
CompletionTime: &metav1.Time{Time: time.Now()},
},
},
}, {
desc: "the failed task show task results",
podStatus: corev1.PodStatus{
Phase: corev1.PodFailed,
ContainerStatuses: []corev1.ContainerStatus{{
Name: "step-task-result",
State: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{
Message: `[{"key":"resultName","value":"resultValue", "type":1}]`,
},
},
}},
},
want: v1beta1.TaskRunStatus{
Status: statusFailure(v1beta1.TaskRunReasonFailed.String(), "build failed for unspecified reasons."),
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
Steps: []v1beta1.StepState{{
ContainerState: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{
Message: `[{"key":"resultName","value":"resultValue","type":1}]`,
},
},
Name: "task-result",
ContainerName: "step-task-result",
}},
Sidecars: []v1beta1.SidecarState{},
CompletionTime: &metav1.Time{Time: time.Now()},
TaskRunResults: []v1beta1.TaskRunResult{{
Name: "resultName",
Type: v1beta1.ResultsTypeString,
Value: *v1beta1.NewStructuredValues("resultValue"),
}},
},
},
}, {
desc: "taskrun status set to failed if task fails",
podStatus: corev1.PodStatus{
Expand Down
26 changes: 26 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6228,13 +6228,22 @@ spec:
operator: in
values:
- aResultValue
- name: final-task-7
params:
- name: finalParam
value: $(tasks.dag-task-3.results.aResult)
taskRef:
name: final-task
tasks:
- name: dag-task-1
taskRef:
name: dag-task
- name: dag-task-2
taskRef:
name: dag-task
- name: dag-task-3
taskRef:
name: dag-task
`)}

prs := []*v1beta1.PipelineRun{parse.MustParseV1beta1PipelineRun(t, `
Expand Down Expand Up @@ -6295,6 +6304,23 @@ status:
- lastTransitionTime: null
status: "False"
type: Succeeded
`),
mustParseTaskRunWithObjectMeta(t,
taskRunObjectMeta("test-pipeline-run-final-task-results-dag-task-3-xxyyy", "foo",
"test-pipeline-run-final-task-results", "test-pipeline", "dag-task-3", false),
`
spec:
serviceAccountName: test-sa
taskRef:
name: hello-world
status:
conditions:
- lastTransitionTime: null
status: "False"
type: Succeeded
taskResults:
- name: aResult
value: aResultValue
`),
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/pipelinerun/resources/resultrefresolution.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ func resolveResultRef(pipelineState PipelineRunState, resultRef *v1beta1.ResultR
if referencedPipelineTask == nil {
return nil, resultRef.PipelineTask, fmt.Errorf("could not find task %q referenced by result", resultRef.PipelineTask)
}
if !referencedPipelineTask.isSuccessful() {
return nil, resultRef.PipelineTask, fmt.Errorf("task %q referenced by result was not successful", referencedPipelineTask.PipelineTask.Name)
if !referencedPipelineTask.isSuccessful() && !referencedPipelineTask.isFailure() {
return nil, resultRef.PipelineTask, fmt.Errorf("task %q referenced by result was not finished", referencedPipelineTask.PipelineTask.Name)
}

var runName, runValue, taskRunName string
Expand Down
47 changes: 47 additions & 0 deletions pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ var (
Type: apis.ConditionSucceeded,
Status: corev1.ConditionTrue,
}
failedCondition = apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
}
)

var pipelineRunState = PipelineRunState{{
Expand Down Expand Up @@ -180,6 +184,35 @@ var pipelineRunState = PipelineRunState{{
Value: *v1beta1.NewStructuredValues("$(tasks.dTask.results.dResult[3])"),
}},
},
}, {
TaskRunName: "eTaskRun",
TaskRun: &v1beta1.TaskRun{
ObjectMeta: metav1.ObjectMeta{Name: "eTaskRun"},
Status: v1beta1.TaskRunStatus{
Status: duckv1.Status{
Conditions: duckv1.Conditions{failedCondition},
},
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
TaskRunResults: []v1beta1.TaskRunResult{{
Name: "eResult",
Value: *v1beta1.NewStructuredValues("eResultValue"),
}},
},
},
},
PipelineTask: &v1beta1.PipelineTask{
Name: "eTask",
TaskRef: &v1beta1.TaskRef{Name: "eTask"},
},
}, {
PipelineTask: &v1beta1.PipelineTask{
Name: "fTask",
TaskRef: &v1beta1.TaskRef{Name: "fTask"},
Params: v1beta1.Params{{
Name: "fParam",
Value: *v1beta1.NewStructuredValues("$(tasks.eTask.results.eResult)"),
}},
},
}}

func TestResolveResultRefs(t *testing.T) {
Expand Down Expand Up @@ -285,6 +318,20 @@ func TestResolveResultRefs(t *testing.T) {
FromRun: "aRun",
}},
wantErr: false,
}, {
name: "Test successful result references resolution - params - failed taskrun",
pipelineRunState: pipelineRunState,
targets: PipelineRunState{
pipelineRunState[10],
},
want: ResolvedResultRefs{{
Value: *v1beta1.NewStructuredValues("eResultValue"),
ResultReference: v1beta1.ResultRef{
PipelineTask: "eTask",
Result: "eResult",
},
FromTaskRun: "eTaskRun",
}},
}} {
t.Run(tt.name, func(t *testing.T) {
got, pt, err := ResolveResultRefs(tt.pipelineRunState, tt.targets)
Expand Down
120 changes: 120 additions & 0 deletions test/task_results_from_failed_tasks_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
//go:build e2e
// +build e2e

/*
Copyright 2023 The Tekton Authors
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package test

import (
"context"
"fmt"
"testing"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/test/parse"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
knativetest "knative.dev/pkg/test"
"knative.dev/pkg/test/helpers"
)

func TestTaskResultsFromFailedTasks(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)

pipelineRun := parse.MustParseV1beta1PipelineRun(t, fmt.Sprintf(`
metadata:
name: %s
spec:
pipelineSpec:
tasks:
- name: task1
taskSpec:
results:
- name: result1
- name: result2
steps:
- name: failing-step
image: busybox
script: 'echo -n 123 | tee $(results.result1.path); exit 1; echo -n 456 | tee $(results.result2.path)'
finally:
- name: finaltask1
params:
- name: param1
value: $(tasks.task1.results.result1)
taskSpec:
params:
- name: param1
steps:
- image: busybox
script: 'exit 0'
- name: finaltask2
params:
- name: param1
value: $(tasks.task1.results.result2)
taskSpec:
params:
- name: param1
steps:
- image: busybox
script: exit 0`, helpers.ObjectNameForTest(t)))

if _, err := c.V1beta1PipelineRunClient.Create(ctx, pipelineRun, metav1.CreateOptions{}); err != nil {
t.Fatalf("Failed to create PipelineRun `%s`: %s", pipelineRun.Name, err)
}

t.Logf("Waiting for PipelineRun in namespace %s to fail", namespace)
if err := WaitForPipelineRunState(ctx, c, pipelineRun.Name, timeout, FailedWithReason(v1beta1.PipelineRunReasonFailed.String(), pipelineRun.Name), "InvalidTaskResultReference", v1beta1Version); err != nil {
t.Errorf("Error waiting for PipelineRun to fail: %s", err)
}

taskrunList, err := c.V1beta1TaskRunClient.List(ctx, metav1.ListOptions{LabelSelector: "tekton.dev/pipelineRun=" + pipelineRun.Name})
if err != nil {
t.Fatalf("Error listing TaskRuns for PipelineRun %s: %s", pipelineRun.Name, err)
}

if len(taskrunList.Items) != 2 {
t.Fatalf("The pipelineRun \"%s\" should have exactly 2 taskRuns, one for the task \"task1\""+
"and one more for the final task \"finaltask1\" instead it has \"%d\" taskRuns", pipelineRun.Name, len(taskrunList.Items))
}

for _, taskrunItem := range taskrunList.Items {
switch n := taskrunItem.Labels["tekton.dev/pipelineTask"]; n {
case "task1":
if !isFailed(t, "", taskrunItem.Status.Conditions) {
t.Fatalf("task1 should have been a failure")
}
if len(taskrunItem.Status.TaskRunResults) != 1 {
t.Fatalf("task1 should have produced a result even with the failing step")
}
for _, r := range taskrunItem.Status.TaskRunResults {
if r.Name == "result1" && r.Value.StringVal != "123" {
t.Fatalf("task1 should have initialized a result \"result1\" to \"123\"")
}
}
case "finaltask1":
if !isSuccessful(t, "", taskrunItem.Status.Conditions) {
t.Fatalf("finaltask1 should have been successful")
}
default:
t.Fatalf("TaskRuns were not found for both final and dag tasks")
}
}
}

0 comments on commit 660e742

Please sign in to comment.