[TEP-0050] Implement PipelineTask OnError #7422

QuanZhang-William · 2023-11-27T15:49:18Z

Changes

Part of #7165. In TEP-0050, we proposed to add an OnError API field under PipelineTask to configure error handling strategy.

This commits leverages PipelineTask.OnError API field introduced in the previous PR, implement the error handling strategy, update related docs and tests.

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Implement "Ignore Task Failure" with new "PipelineTask.OnError" API field (TEP-0050). User can now set `pipelineTask.onError: continue` to ignore failure

tekton-robot · 2023-11-27T15:49:21Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

QuanZhang-William · 2023-11-27T15:49:31Z

/test all

tekton-robot · 2023-11-27T15:56:06Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	92.9%	93.0%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	92.3%	92.2%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-11-27T15:58:24Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	92.9%	93.0%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	92.3%	92.2%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

QuanZhang-William · 2023-11-27T17:36:29Z

/retest

tekton-robot · 2023-11-27T17:40:44Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	92.9%	93.0%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	92.3%	92.2%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-11-27T17:44:36Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	92.9%	93.0%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	92.3%	92.2%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-11-27T18:32:38Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	92.9%	93.0%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	92.3%	92.2%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

JeromeJu · 2023-11-28T15:29:05Z

/assign

pkg/apis/pipeline/v1/pipelinerun_types.go

tekton-robot · 2023-11-29T15:43:58Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	92.9%	93.0%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	92.3%	92.2%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-11-29T15:44:42Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	92.9%	93.0%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	92.3%	92.2%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

JeromeJu · 2023-12-01T15:50:40Z

test/ignore_task_error_test.go

+        steps:
+        - name: failing-step
+          image: busybox
+          script: exit 1; echo -n 123 | tee $(results.result1.path)'


Looks like we are missing one quote ' here.
I'm not sure why this doesn't affect the test being run.
Curious how it went through in CI, coz when I create this PipelineRun locally this'd fail runtime validation.

missing quote added. Interestingly, I tried the same yamlfile and test locally. I verified the resources and test are created with no problem 🤔

test/ignore_task_error_test.go

JeromeJu · 2023-12-01T16:32:24Z

pkg/pod/status.go

-		updateCompletedTaskRunStatus(logger, trs, pod)
+		onError, ok := tr.Annotations[v1.PipelineTaskOnErrorAnnotation]
+		if ok {
+			updateCompletedTaskRunStatus(logger, trs, pod, v1.PipelineTaskOnErrorType(onError))


Could you help check my understanding here:
I saw this func called at taskrun reconciler, but what if the TaskRun fails at earlier stage i.e. pvc creation

Hi @JeromeJu. This is a good questions.

TEP-0050 is designed with requirement that the task would be considered "successful" for the purposes of determining the status of the pipeline (i.e. the TaskRun itself should still be failed, but with reason FailureIgnored). Looking at the usecases in TEP-0050, it is mainly used to ignore pod error itself.

I think your question is that for the validation/reconciler errors (like the pvc creation), should we also mark the reason as FailureIgnored?

I think it might not be a good idea make the validation/reconciler errors with reason FailureIgnored:

The validation/reconciler errors is likely from the Pipeline/Task Author, it shouldn't be ignored, but should be raised and fixed, FailureIgnored error reason could be confusing.

the error reason FailureIgnored becomes too generic if we use it across the TaskRun reconciler code. Instead, I'd prefer to keep the scope of FailureIgnored reason to the pod execution time.

WDYT?

Thanks @QuanZhang-William for the offline conversation and completing the 2nd half of my question!

And yes, I do agree that in actual use cases, only the container(Step) failures in the pod, which indicates users' own test failures, shall be ignored but not the "preparation" of those steps i.e. pvc creation and validations. Thanks for the explanation here.

So I think the current implementation is the way to go. One small ask is maybe we would can add this explanation/conversation somewhere in the docString or maybe link to the TEP050 for this design choice.

This makes sense as long as we document that FailedIgnored is ignoring a failure when the actual task execution fails but not otherwise (validation failure, etc.)

SG. I have added the docstring. PTAL

QuanZhang-William · 2023-12-01T17:20:58Z

cc @tektoncd/core-collaborators @tektoncd/core-maintainers @jerop @pritidesai . Please take a look if you are interested!

dibyom · 2023-12-01T18:51:46Z

Overall, this looks good to me! Thanks @QuanZhang-William Once you address the remaining open comments, I can approve.

QuanZhang-William · 2023-12-04T15:23:09Z

Overall, this looks good to me! Thanks @QuanZhang-William Once you address the remaining open comments, I can approve.

Thanks @dibyom. I've addressed the comments. PTAL!

tekton-robot · 2023-12-04T15:27:31Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	92.9%	93.0%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	92.3%	92.2%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-12-04T15:29:11Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	92.9%	93.0%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	92.3%	92.2%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

Part of [tektoncd#7165][tektoncd#7165]. In [TEP-0050][tep-0050], we proposed to add an `OnError` API field under `PipelineTask` to configure error handling strategy. This commits leverages `PipelineTask.OnError` API field introduced in the previous PR, implement the error handling strategy, update related docs and tests. /kind feature [tep-0050]: https://github.com/tektoncd/community/blob/main/teps/0050-ignore-task-failures.md [tektoncd#7165]: tektoncd#7165

tekton-robot · 2023-12-04T15:47:47Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	92.9%	93.0%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	92.3%	92.2%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-12-04T15:49:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	92.9%	93.0%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	92.3%	92.2%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

JeromeJu

Thanks @QuanZhang-William
One nit on release note, might be helpful to also include how users could use with the OnError feature i.e. "users can now set OnError to 'continue' to ignore failure from previous task".

JeromeJu · 2023-12-04T17:32:15Z

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

@@ -278,11 +280,11 @@ func (state PipelineRunState) getNextTasks(candidateTasks sets.String) []*Resolv
 }

 // IsStopping returns true if the PipelineRun won't be scheduling any new Task because
-// at least one task already failed or was cancelled in the specified dag
+// at least one task already failed (with onError: stopAndFail) or was cancelled in the specified dag


nit: "stopAndFail" is the default here this sounds like users have to set OnError?
Will leave this up to your decision.

dibyom

Any other feedback on this before merge? cc @vdemeester @jerop @pritidesai @afrittoli

tekton-robot · 2023-12-05T16:19:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, JeromeJu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JeromeJu,dibyom]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

QuanZhang-William · 2023-12-06T15:46:21Z

Any other feedback on this before merge? cc @vdemeester @jerop @pritidesai @afrittoli

Hi @dibyom. This PR has been opened for more than a week. Was wondering if we can merge? 🙏

Yongxuanzhang

/lgtm

tekton-robot requested review from afrittoli and pritidesai November 27, 2023 15:49

QuanZhang-William mentioned this pull request Nov 27, 2023

TEP-0050: Ignore Task Failure Implementation #7165

Closed

4 tasks

QuanZhang-William force-pushed the tep-0050-implementation branch from 0a34d79 to 98fd1ec Compare November 27, 2023 17:32

QuanZhang-William marked this pull request as ready for review November 27, 2023 18:25

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 27, 2023

tekton-robot requested review from dibyom and jerop November 27, 2023 18:25

tekton-robot assigned JeromeJu Nov 28, 2023

dibyom reviewed Nov 28, 2023

View reviewed changes

pkg/apis/pipeline/v1/pipelinerun_types.go Outdated Show resolved Hide resolved

QuanZhang-William force-pushed the tep-0050-implementation branch from 98fd1ec to df36337 Compare November 29, 2023 15:35

QuanZhang-William requested a review from dibyom November 30, 2023 18:33

JeromeJu reviewed Dec 1, 2023

View reviewed changes

QuanZhang-William force-pushed the tep-0050-implementation branch from df36337 to 34ef7fd Compare December 4, 2023 15:21

QuanZhang-William requested a review from JeromeJu December 4, 2023 15:22

QuanZhang-William force-pushed the tep-0050-implementation branch from 34ef7fd to a4f7be7 Compare December 4, 2023 15:40

JeromeJu approved these changes Dec 4, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2023

dibyom approved these changes Dec 5, 2023

View reviewed changes

JeromeJu added this to the Pipelines v0.55 milestone Dec 6, 2023

Yongxuanzhang reviewed Dec 6, 2023

View reviewed changes

tekton-robot assigned Yongxuanzhang Dec 6, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2023

tekton-robot merged commit b6d27a8 into tektoncd:main Dec 6, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP-0050] Implement PipelineTask OnError #7422

[TEP-0050] Implement PipelineTask OnError #7422

QuanZhang-William commented Nov 27, 2023 •

edited

Loading

tekton-robot commented Nov 27, 2023

QuanZhang-William commented Nov 27, 2023

tekton-robot commented Nov 27, 2023

tekton-robot commented Nov 27, 2023

QuanZhang-William commented Nov 27, 2023

tekton-robot commented Nov 27, 2023

tekton-robot commented Nov 27, 2023

tekton-robot commented Nov 27, 2023

JeromeJu commented Nov 28, 2023

tekton-robot commented Nov 29, 2023

tekton-robot commented Nov 29, 2023

JeromeJu Dec 1, 2023

QuanZhang-William Dec 4, 2023

JeromeJu Dec 1, 2023

QuanZhang-William Dec 1, 2023

JeromeJu Dec 1, 2023

dibyom Dec 1, 2023

QuanZhang-William Dec 4, 2023

QuanZhang-William commented Dec 1, 2023

dibyom commented Dec 1, 2023

QuanZhang-William commented Dec 4, 2023

tekton-robot commented Dec 4, 2023

tekton-robot commented Dec 4, 2023

tekton-robot commented Dec 4, 2023

tekton-robot commented Dec 4, 2023

JeromeJu left a comment

JeromeJu Dec 4, 2023

dibyom left a comment

tekton-robot commented Dec 5, 2023

QuanZhang-William commented Dec 6, 2023

Yongxuanzhang left a comment

[TEP-0050] Implement PipelineTask OnError #7422

[TEP-0050] Implement PipelineTask OnError #7422

Conversation

QuanZhang-William commented Nov 27, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Nov 27, 2023

QuanZhang-William commented Nov 27, 2023

tekton-robot commented Nov 27, 2023

tekton-robot commented Nov 27, 2023

QuanZhang-William commented Nov 27, 2023

tekton-robot commented Nov 27, 2023

tekton-robot commented Nov 27, 2023

tekton-robot commented Nov 27, 2023

JeromeJu commented Nov 28, 2023

tekton-robot commented Nov 29, 2023

tekton-robot commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuanZhang-William commented Dec 1, 2023

dibyom commented Dec 1, 2023

QuanZhang-William commented Dec 4, 2023

tekton-robot commented Dec 4, 2023

tekton-robot commented Dec 4, 2023

tekton-robot commented Dec 4, 2023

tekton-robot commented Dec 4, 2023

JeromeJu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom left a comment

Choose a reason for hiding this comment

tekton-robot commented Dec 5, 2023

QuanZhang-William commented Dec 6, 2023

Yongxuanzhang left a comment

Choose a reason for hiding this comment

QuanZhang-William commented Nov 27, 2023 •

edited

Loading