-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TEP-0121: Implement Retries
in TaskRun
#5807
Conversation
The following is the coverage report on the affected files.
|
2e4a97b
to
1d1beaf
Compare
The following is the coverage report on the affected files.
|
1d1beaf
to
819e26e
Compare
The following is the coverage report on the affected files.
|
This is the implementation PR of TEP-0121, see the demo video on a API WG meeting [18:05-21:00]. /assign @pritidesai cc @tektoncd/core-maintainers |
Thanks @XinruZhang and thanks for making the comprehensive changes for the respective conversions. And this PR lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The api changes LGTM
819e26e
to
741c534
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closes #5248
@XinruZhang could you test the behavior of your PR against #4071 (comment)?
The following is the coverage report on the affected files.
|
741c534
to
57ef9ee
Compare
The following is the coverage report on the affected files.
|
Yes it works as expected:
|
57ef9ee
to
5def3f7
Compare
The following is the coverage report on the affected files.
|
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @XinruZhang!
5def3f7
to
415cccc
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/hold One thing we haven't fully investigated is |
/unhold TL;DR: We do NOT and should NOT mark the resource as The current behaviorThe transient errors in taskrun implementation: once pipeline/pkg/reconciler/taskrun/taskrun.go Line 127 in 8c59d00
And if the TaskRun failed to retrieve the remote Task, the taskRun marks the resource as pipeline/pkg/reconciler/taskrun/taskrun.go Line 336 in 8c59d00
For reference, in pipeline/pkg/reconciler/pipelinerun/pipelinerun.go Lines 526 to 535 in 8c59d00
That's said, we won't mark the resource as Done ( |
9826aa2
to
448265d
Compare
The following is the coverage report on the affected files.
|
448265d
to
9826aa2
Compare
The following is the coverage report on the affected files.
|
9826aa2
to
e5d7c5d
Compare
The following is the coverage report on the affected files.
|
e5d7c5d
to
8b295db
Compare
The following is the coverage report on the affected files.
|
) | ||
|
||
// EmitK8sEvents emits kubernetes events for object | ||
// WhenConditionChange emits kubernetes events for object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// WhenConditionChange emits kubernetes events for object | |
// EmitK8sEventsWhenConditionChange emits kubernetes events for object |
// IsRetriable returns true if the TaskRun's Retries is not exhausted. | ||
func (tr *TaskRun) IsRetriable() bool { | ||
return len(tr.Status.RetriesStatus) < tr.Spec.Retries | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add unit tests for this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, should this function also be added to taskrun_types.go in v1?
@@ -85,6 +85,7 @@ func (trs *TaskRunSpec) ConvertTo(ctx context.Context, sink *v1.TaskRunSpec) err | |||
} | |||
sink.Status = v1.TaskRunSpecStatus(trs.Status) | |||
sink.StatusMessage = v1.TaskRunSpecStatusMessage(trs.StatusMessage) | |||
sink.Retries = trs.Retries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add retries
field to the ConvertTo
tests in https://github.com/tektoncd/pipeline/blob/main/pkg/apis/pipeline/v1beta1/taskrun_conversion_test.go
@@ -164,6 +165,7 @@ func (trs *TaskRunSpec) ConvertFrom(ctx context.Context, source *v1.TaskRunSpec) | |||
} | |||
trs.Status = TaskRunSpecStatus(source.Status) | |||
trs.StatusMessage = TaskRunSpecStatusMessage(source.StatusMessage) | |||
trs.Retries = source.Retries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add retries
field to the ConvertFrom
tests in https://github.com/tektoncd/pipeline/blob/main/pkg/apis/pipeline/v1beta1/taskrun_conversion_test.go
Thanks @jerop! We've discussed offline, will break this PR into smaller pieces, and targeting to get all sub PRs merged before v0.43.0. Thanks! Keeping this PR open as a reference. |
This commit implements `Retries` in `TaskRun`, and removes the logic that PipelineRun controller relies on `RetriesStatus` to determine the termination of a TaskRun or CustomRun/Run. Key Changes: - New `Retries` field in both `v1beta1.TaskRun` and `v1.TaskRun` - Archive retry attempt history in `RetriesStatus` for a failed `TaskRun`, before sending kubernetes and cloud events before a reconcile loop ends. - Unit Tests to test the `TaskRun` object changes, especially the changes on `status.conditions` and `status.retriesStatus` after being reconciled once (one reconcile loop).
8b295db
to
146213e
Compare
The following is the coverage report on the affected files.
|
Thank you for breaking it down @XinruZhang! /hold |
@XinruZhang: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Changes
This PR implements
Retries
inTaskRun
, and removes the logic that PipelineRun controller relies onRetriesStatus
to determine the termination of a TaskRun or CustomRun/Run.Key Changes:
Retries
field in bothv1beta1.TaskRun
andv1.TaskRun
RetriesStatus
for a failedTaskRun
, before sending kubernetes and cloud events before a reconcile loop ends.TaskRun
object changes, especially the changes onstatus.conditions
andstatus.retriesStatus
after being reconciled once (one reconcile loop).fix #5755
fix #5248
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes