TEP-0114: Cancellation is required

[XinruZhang]

We shoud mark Custom Task Cancellation as required in Custom Task Beta, because pipeline-level timeout won't work if it is optional.

[XinruZhang]

/hold until #880 is merged

[XinruZhang]

/assign @jerop @abayer

[XinruZhang]

/unhold

[jerop]

cc @tektoncd/core-maintainers

[abayer]

/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, jerop, vdemeester

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:

• teps/OWNERS [abayer,jerop,vdemeester]

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

[XinruZhang]

/hold explain the cancellation details.

[XinruZhang]

/unhold

[abayer]

👍

[pritidesai]

/cc @ScrapCodes

[tekton-robot]

@pritidesai: GitHub didn't allow me to request PR reviews from the following users: ScrapCodes.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @ScrapCodes

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.

[pritidesai]

What does properly allude to here?

@ScrapCodes please share your experience here if you have any

[ScrapCodes]

To ensure that cancellation is required, are we planning to have validations now or in future? if no, is this just a documentation change?

Also, do we support pipeline level Timeout for custom task? I remember it was removed?

What are the downside of keeping as: "It is upto the custom task controller to support it." i.e. optional, e.g. not all custom task may want to support it e.g. CEL. Those custom task who do not wish to support they are responsible for not supporting timeout as well as of today.

Today Tekton controller does not depend on custom task's support of cancel, is it going to change?

Pipeline timeouts and PipelineRun cancellation will not work properly.
Can you elaborate?

[XinruZhang]

@pritidesai

What does properly allude to here?

PiplineRun controller has hard code dependencies on status report of CustomRun controller: See the code, which is used to decide if the CustomRun is failed or not.

@ScrapCodes

To ensure that cancellation is required, are we planning to have validations now or in future? if no, is this just a documentation change?

Yes I think we'll have code validations in the future in the ideal world. I opened an issue for it tektoncd/pipeline#5700

do we support pipeline level Timeout for custom task? I remember it was removed?

There are two kinds of timeout for Custom Task:

• One is PipelineTask.Timeout, which is passed to CustomRunSpec.Timeout.
• The other is PipelineRunSpec.Timeouts.

What we removed is the first one. i.e. we stopped PipelineRun controller to modify CustomRun.status.Conditions upon CustomRun timeout -- (CustomRun executes more than CustomRunSpec.Timeout, see tektoncd/pipeline#5658).

[pritidesai]

From @ScrapCodes: What are the downside of keeping as: "It is upto the custom task controller to support it." i.e. optional, e.g. not all custom task may want to support it e.g. CEL. Those custom task who do not wish to support they are responsible for not supporting timeout as well as of today.

Hey @XinruZhang, my understanding here is its upto the custom task author whether to implement it or not since it depends on their use case e.g. CEL kind of custom controller v/s wait custom task v/s pipelines in pipelines custom controller.

Please confirm and I can merge this change, thanks! 🙏

[XinruZhang]

Thanks for double check @pritidesai, I strongly support marking Cancellation as required, mainly because:

We'd like the PipelineRun API semantics to be independent of how its sub-resources are implemented. No matter how the custom task is implemented, the PipelineRun behavior should keep consistent. That's said, in Custom Task, anything that Pipeline Controller relies on MUST be strictly defined.

Let's take the following YAML as an example:

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  generateName: pr-custom-task-
spec:
  timeouts:
    tasks: "2s"
  pipelineSpec:
    tasks:
    - name: wait
      taskRef:
        apiVersion: wait.testing.tekton.dev/v1alpha1
        kind: Wait-No-Cancellation
      params:
      - name: duration
        value: "60s"

To test it, install a POC Wait Task following https://github.com/XinruZhang/xinru-custom-task/tree/no-cancellation and submit the YAML above

We set PipelineRun.Spec.Timeouts.Tasks as 2s, meaning we expect each tasks to be done in 2s, however, If the wait task didn't implement Cancellation, the Wait task will run until the duration elapses, and the PipelineRun will also need to run for at least "60s"

[XinruZhang]

/kind tep

[pritidesai]

Thank you @XinruZhang, I understand the wait task example you have mentioned above. There is no intend to request any inconsistency in pipelineRun controller.

If I have implemented waiting in my pipeline for 60 seconds, as a user why would I set the timeout of the entire pipeline to 2 seconds? Irrespective of the values chosen here, the cancellation must be required for the use cases which mandate timeout of a pipeline to make that timeout effective so that this example pipeline can be cancelled in 2 seconds. At the same time, this requirement of mandatory cancellation might not even apply to other custom controllers. required can not be made generic for all custom tasks.

I suggest changing:

Implementing cancellation is `required`. If the Custom Task implementor does not support cancellation via `.spec.status`, `Pipeline` timeouts and `PipelineRun` cancellation will not work properly.

to

The custom task is responsible for implementing `cancellation` to support pipelineRun level `timeouts` and `cancellation`. If the Custom Task implementor does not support cancellation via `.spec.status`, `Pipeline` **can not** timeout within the specified interval/duration and **can not** be cancelled as expected upon request.

Let me know what you think. Please feel free to merge it otherwise. Thanks!

[XinruZhang]

Thanks @pritidesai!

I do understand the required cancellation does not apply to some custom tasks.

And I also think we are aligned on that all long running custom tasks MUST implement the cancellation to support PipelineRunSpec.timetouts.

My only concern is, in the future, how do we enforce Custom Tasks conform to Conformance Policy and corresponding Test Suite. See tektoncd/pipeline#5700.

Nevertheless, this is a complex area that requires more discussion, I will update the doc as you suggested -- not required, and get it in. Let's iterate on it :)

cc @lbernick

[pritidesai]

thank you @XinruZhang 👍

/lgtm