TEP-0121: Retries - Reconciler Implementation #5844

XinruZhang · 2022-12-07T16:04:23Z

Changes

This is the second peice of TEP-0121 implementation, targeting at merging it as part in release v0.43.0.

Prior to this PR, retries logic for TaskRun is handled by Tekton PipelineRun reconciler. This PR delegates the retries implementation for TaskRun to the TaskRun reconciler.

The major change is we stopped relying on len(retriesStatus) to decide if a target TaskRun failed or not. Instead, we use status ConditionSuceeded to gate the completion of a TaskRun. Even a TaskRun failed on one execution, as long as it has remaining retries, the TaskRun won't be stored in etcd with its status set as Failed. Instead, the status will be:

Type: Succeeded
Status: Unknown
Reason: ToBeRetried

It is worth noticing that, we still need to use len(retriesStatus) for Custom Tasks because currently we still support the alpha version, once we fully migrate custom task to the beta, we can safely remove that reliance.

Note that the integration test retry_test.go still works.

This is a demo video in an API WG meeting [18:05-21:00]

/close #5756

/kind feature

Submitter Checklist

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

Has Docs included if any changes are user facing
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)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

tekton-robot · 2022-12-07T16:09:51Z

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/apis/pipeline/v1/taskrun_types.go	76.9%	77.4%	0.4
pkg/apis/pipeline/v1beta1/taskrun_conversion.go	92.2%	92.3%	0.1
pkg/apis/pipeline/v1beta1/taskrun_types.go	78.2%	78.6%	0.4
pkg/pod/status.go	90.9%	91.1%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.2%	85.8%	-0.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.7%	94.9%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	97.9%	97.7%	-0.2
pkg/reconciler/taskrun/taskrun.go	82.1%	80.7%	-1.4

tekton-robot · 2022-12-07T16:25:29Z

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/apis/pipeline/v1/taskrun_types.go	76.9%	77.4%	0.4
pkg/apis/pipeline/v1beta1/taskrun_conversion.go	92.2%	92.3%	0.1
pkg/apis/pipeline/v1beta1/taskrun_types.go	78.2%	78.6%	0.4
pkg/pod/status.go	90.9%	91.1%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.2%	85.8%	-0.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.7%	94.9%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	97.9%	97.7%	-0.2
pkg/reconciler/taskrun/taskrun.go	82.1%	80.9%	-1.2

XinruZhang · 2022-12-07T17:05:33Z

/test pull-tekton-pipeline-integration-tests
flake: TestGitPipelineRun

XinruZhang · 2022-12-07T17:06:09Z

/assign @jerop

cc @tektoncd/core-maintainers

XinruZhang · 2022-12-08T16:32:24Z

/unhold

tekton-robot · 2022-12-08T16:40:57Z

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	90.9%	91.1%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.2%	85.8%	-0.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.7%	94.9%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	97.9%	97.7%	-0.2
pkg/reconciler/taskrun/taskrun.go	82.1%	80.9%	-1.2

tekton-robot · 2022-12-08T16:53: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	90.9%	91.1%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.2%	85.8%	-0.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.7%	94.9%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	97.9%	97.7%	-0.2
pkg/reconciler/taskrun/taskrun.go	82.1%	80.9%	-1.2

tekton-robot · 2022-12-08T17:12:55Z

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	90.9%	91.1%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.2%	85.8%	-0.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.7%	94.9%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	97.9%	97.7%	-0.2

XinruZhang · 2022-12-08T17:29:59Z

/hold until #5853 is fixed

tekton-robot · 2022-12-20T15:27:51Z

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	90.3%	90.5%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	85.7%	-0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.3%	94.9%	0.6
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.5%	96.4%	-0.1
pkg/reconciler/taskrun/taskrun.go	82.6%	83.0%	0.4

tekton-robot · 2022-12-20T16:03:46Z

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	90.3%	90.5%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	85.7%	-0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.3%	94.9%	0.6
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.5%	96.4%	-0.1
pkg/reconciler/taskrun/taskrun.go	82.6%	83.0%	0.4

XinruZhang · 2022-12-20T17:07:35Z

/test pull-tekton-pipeline-alpha-integration-tests
Flake: TestExamples/v1beta1/taskruns/authenticating-git-commands

afrittoli

Thank you @XinruZhang for the great work on this and sorry about the late review 🙏
I have a few comments, but mostly minor or not blocking, or related to test coverage.

For events, I think it would be good align the events to the conditions, to be consistent to how things are today. It should be a minor change to implement, let me know what you think.

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go

pkg/reconciler/taskrun/taskrun.go

pkg/reconciler/taskrun/taskrun_test.go

tekton-robot · 2022-12-21T13:49:30Z

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	90.4%	90.5%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.4%	86.1%	-0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.3%	94.9%	0.6
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.5%	96.4%	-0.1
pkg/reconciler/taskrun/taskrun.go	83.1%	83.6%	0.4

tekton-robot · 2022-12-21T13:51:33Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	90.4%	90.5%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.4%	86.1%	-0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.3%	94.9%	0.6
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.5%	96.4%	-0.1
pkg/reconciler/taskrun/taskrun.go	83.1%	83.6%	0.4

tekton-robot · 2022-12-21T14:07:33Z

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	90.4%	90.5%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.4%	86.1%	-0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.3%	94.9%	0.6
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.5%	96.4%	-0.1
pkg/reconciler/taskrun/taskrun.go	83.1%	83.6%	0.4

tekton-robot · 2022-12-21T14:09:14Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	90.4%	90.5%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.4%	86.1%	-0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.3%	94.9%	0.6
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.5%	96.4%	-0.1
pkg/reconciler/taskrun/taskrun.go	83.1%	83.6%	0.4

afrittoli · 2022-12-21T14:15:21Z

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

File Old Coverage New Coverage Delta
pkg/pod/status.go 90.4% 90.5% 0.2
pkg/reconciler/pipelinerun/pipelinerun.go 86.4% 86.1% -0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 94.3% 94.9% 0.6
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go 96.5% 96.4% -0.1
pkg/reconciler/taskrun/taskrun.go 83.1% 83.6% 0.4

@chitrangpatel It looks like finally the go coverage work you made has been deployed, and I see that the coverage report is reported twice. However it looks like the links are broken for one of the two implementations. Would you mind having a look?

tekton-robot · 2022-12-21T14:15: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	90.4%	90.5%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.4%	86.1%	-0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.3%	94.9%	0.6
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.5%	96.4%	-0.1
pkg/reconciler/taskrun/taskrun.go	83.1%	83.6%	0.4

tekton-robot · 2022-12-21T14:16:29Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	90.4%	90.5%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.4%	86.1%	-0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.3%	94.9%	0.6
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.5%	96.4%	-0.1
pkg/reconciler/taskrun/taskrun.go	83.1%	83.6%	0.4

afrittoli

/lgtm

Prior to this commit, `retries` logic for TaskRun is handled by Tekton `PipelineRun` reconciler. This commit delegates the retries implementation for `TaskRun` to the `TaskRun` reconciler. The major change is we stopped relying on `len(retriesStatus)` to decide if a target `TaskRun` failed or not. Instead, we use status `ConditionSuceeded` to gate the completion of a `TaskRun`. Even a `TaskRun` failed on one execution, as long as it has remaining retries, the TaskRun won't be stored in etcd with its status set as `Failed`. Instead, the status will be: ```yaml Type: Succeeded Status: Unknown Reason: ToBeRetried ```

tekton-robot · 2022-12-21T14:40:41Z

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	90.4%	90.5%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.4%	86.1%	-0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.3%	94.9%	0.6
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.5%	96.4%	-0.1
pkg/reconciler/taskrun/taskrun.go	83.1%	83.6%	0.4

tekton-robot · 2022-12-21T14:42:01Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	90.4%	90.5%	0.2
pkg/reconciler/pipelinerun/pipelinerun.go	86.4%	86.1%	-0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.3%	94.9%	0.6
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.5%	96.4%	-0.1
pkg/reconciler/taskrun/taskrun.go	83.1%	83.6%	0.4

afrittoli · 2022-12-21T14:44:50Z

/lgtm

tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. release-note-none Denotes a PR that doesnt merit a release note. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 7, 2022

tekton-robot requested review from afrittoli and imjasonh December 7, 2022 16:04

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 7, 2022

XinruZhang force-pushed the TEP-0121-Reconciler branch 2 times, most recently from 1da1ab8 to f5f814f Compare December 7, 2022 16:19

tekton-robot assigned jerop Dec 7, 2022

jerop added this to the Pipelines v0.43 milestone Dec 7, 2022

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2022

XinruZhang force-pushed the TEP-0121-Reconciler branch from f5f814f to f0a7451 Compare December 8, 2022 16:30

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2022

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 8, 2022

XinruZhang force-pushed the TEP-0121-Reconciler branch from f0a7451 to ac6d523 Compare December 8, 2022 16:35

jerop changed the title ~~TEP-0121: Reconciler Implementation~~ TEP-0121: Retries - Reconciler Implementation Dec 8, 2022

XinruZhang force-pushed the TEP-0121-Reconciler branch from ac6d523 to c73c568 Compare December 8, 2022 16:46

XinruZhang force-pushed the TEP-0121-Reconciler branch from c73c568 to 13aab95 Compare December 8, 2022 17:07

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 8, 2022

XinruZhang mentioned this pull request Dec 8, 2022

TEP-0121: Modify metrics recorder for TaskRun Retries #5853

Merged

7 tasks

XinruZhang force-pushed the TEP-0121-Reconciler branch from d1f83c6 to 2087371 Compare December 20, 2022 15:22

XinruZhang force-pushed the TEP-0121-Reconciler branch from 2087371 to 92569ed Compare December 20, 2022 15:57

afrittoli reviewed Dec 21, 2022

View reviewed changes

XinruZhang force-pushed the TEP-0121-Reconciler branch from 92569ed to 27b2630 Compare December 21, 2022 13:44

XinruZhang force-pushed the TEP-0121-Reconciler branch from 27b2630 to 944382f Compare December 21, 2022 14:02

XinruZhang force-pushed the TEP-0121-Reconciler branch from 944382f to 36cf2d8 Compare December 21, 2022 14:09

afrittoli reviewed Dec 21, 2022

View reviewed changes

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

XinruZhang force-pushed the TEP-0121-Reconciler branch from 36cf2d8 to 9be9e9a Compare December 21, 2022 14:35

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 21, 2022

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

tekton-robot merged commit 2ee148d into tektoncd:main Dec 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TEP-0121: Retries - Reconciler Implementation #5844

TEP-0121: Retries - Reconciler Implementation #5844

XinruZhang commented Dec 7, 2022 •

edited

Loading

tekton-robot commented Dec 7, 2022

tekton-robot commented Dec 7, 2022

XinruZhang commented Dec 7, 2022

XinruZhang commented Dec 7, 2022 •

edited

Loading

XinruZhang commented Dec 8, 2022

tekton-robot commented Dec 8, 2022

tekton-robot commented Dec 8, 2022

tekton-robot commented Dec 8, 2022

XinruZhang commented Dec 8, 2022

tekton-robot commented Dec 20, 2022

tekton-robot commented Dec 20, 2022

XinruZhang commented Dec 20, 2022

afrittoli left a comment

tekton-robot commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

afrittoli commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

afrittoli left a comment

tekton-robot commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

afrittoli commented Dec 21, 2022

TEP-0121: Retries - Reconciler Implementation #5844

TEP-0121: Retries - Reconciler Implementation #5844

Conversation

XinruZhang commented Dec 7, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Dec 7, 2022

tekton-robot commented Dec 7, 2022

XinruZhang commented Dec 7, 2022

XinruZhang commented Dec 7, 2022 • edited Loading

XinruZhang commented Dec 8, 2022

tekton-robot commented Dec 8, 2022

tekton-robot commented Dec 8, 2022

tekton-robot commented Dec 8, 2022

XinruZhang commented Dec 8, 2022

tekton-robot commented Dec 20, 2022

tekton-robot commented Dec 20, 2022

XinruZhang commented Dec 20, 2022

afrittoli left a comment

Choose a reason for hiding this comment

tekton-robot commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

afrittoli commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

afrittoli left a comment

Choose a reason for hiding this comment

tekton-robot commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

afrittoli commented Dec 21, 2022

XinruZhang commented Dec 7, 2022 •

edited

Loading

XinruZhang commented Dec 7, 2022 •

edited

Loading