TEP-0114: Clarify the Behavior of Custom Task Retries

[XinruZhang]

Changes

Prior to this PR, it is not clear enough how retries works when
the custom task run is timeout. This PR refines the developer
guide for custom controllers supporting retries to clarify it.

/kind doc

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]

@XinruZhang: The label(s) kind/doc cannot be applied, because the repository doesn't have them.

In response to this:

Changes

Prior to this PR, it is not clear enough how retries works when
the custom task run is timeout. This PR refines the developer
guide for custom controllers supporting retries to clarify it.

/kind doc

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

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.

[XinruZhang]

/assign @jerop

[XinruZhang]

/release-note-none

[tekton-robot]

@XinruZhang: The label(s) kind/docs cannot be applied, because the repository doesn't have them.

In response to this:

/kind docs

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.

[XinruZhang]

/kind documentation

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from jerop after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[lbernick]

I don't think it's true that timeout should be considered for the sum of retries. The timeout value passed to runs comes from the Pipeline Task timeout. For TaskRuns, we've decided that the pipeline task timeout should apply to each retry independently, and the tasks and pipeline timeouts on the PipelineRun take into account time spent during retries. We should be consistent between TaskRuns and Runs here.

[lbernick]

Although I guess if we've been documenting that it applies to all retries people have already been implementing it in this way? FYI @jerop there's some inconsistency here between taskruns and runs

[XinruZhang]

/unhold Thanks @lbernick for pointing it out! Updated the doc. To reflect what we discussed, basically, the behavior of Run should be aligned with PipelineTask -- timeout is for each retries not the sum of all retries.

[lbernick]

Thanks @XinruZhang for the updates! my only concern is that even though this is a documentation change, it's sort of backwards incompatible because it reflects the contract we expect run controllers to implement (defined in TEP-0069). I think it's worth getting some more opinions on whether we'd prefer to keep the run reconciler contract as is, or whether we'd prefer consistency between TaskRuns and Runs. In the meantime, it's worth documenting that the Pipeline Task timeout is applied independently to each retry of a TaskRun.

[dibyom]

I think it's worth getting some more opinions on whether we'd prefer to keep the run reconciler contract as is, or whether we'd prefer consistency between TaskRuns and Runs. In the meantime, it's worth documenting that the Pipeline Task timeout is applied independently to each retry of a TaskRun.

I think we should at least mark this as a breaking change with an action-required - probably worth sending to the mailing lists as well if we decide to go ahead

[XinruZhang]

Thanks @lbernick, that's a great concern. Thanks @dibyom for your input. What I can think of we need to do is: creating an issue to

• Clarify the retry behavior gap between Runs and TaskRuns
• Collect people's opinion on this issue
• Reach consensus about next steps

Do you think this is a good idea?

BTW, @dibyom I'm wondering what needs to be done for breaking change? Adding action-required notice in release notes? or anything else?

[lbernick]

we could probably just discuss here-- @tektoncd/core-maintainers

[XinruZhang]

Though I do think it would be better if we keep the behaviors of Runs & TaskRuns aligned, then the action item would be what's the process of making breaking changes to Custom Task Run.

[XinruZhang]

Opened a TEP (TEP-0121 tektoncd/community#816) to fully discuss the behavior of retries, we can continue our discussions there.

[XinruZhang]

As discussed in the Tekton API Working Group today, we'd like to modify the development guidance of Custom Task, suggesting users to set timeout for each retry attempt.

[XinruZhang]

/hold this PR until TEP-0121 is implemented.

[XinruZhang]

I'm closing this PR because we've made changes to v1beta1.CustomRun. We can leave it as what it is for v1alpha1.Run. Please feel free to comment if there's any other concerns.