Implement timeout for custom tasks. #3976

ScrapCodes · 2021-05-26T11:04:47Z

Tekton shall give custom task controller an opportunity to respect timeout and do a proper cleanup. It depends on the individual custom task controller implementation - how exactly it responds.

closes #3962

Changes

Tekton controllers does not forcibly update the .status of a Run directly, that responsibility belongs to the Custom Task controller.

Run can be created with a timeout now, and it is upto the custom task controller to support it or not.
Custom tasks inside a PipelineRun with pipeline wide timeout and/or pipeline task level timeout is updated to run with same policy as it is for task runs. On timeout, the run's status is updated with "RunCancelled".

Added relevant tests.

Corresponding TEP update at: tektoncd/community#433

/kind feature

Submitter Checklist

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

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes


Added timeout support for custom task. 
e.g.

        apiVersion: tekton.dev/v1alpha1
        kind: Run
        metadata:
          generateName: simpletasklooprun
        spec:
          timeout: 10s # set timeouts here.
          params:
            - name: word
              value:
                - jump
                - land
                - roll
            - name: suffix
              value: ing
          ref:
            apiVersion: custom.tekton.dev/v1alpha1
            kind: TaskLoop
            name: simpletaskloop

Existing custom task controller, can either support the new field or ignore it.

tekton-robot · 2021-05-26T11:05:02Z

Hi @ScrapCodes. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

afrittoli · 2021-05-26T12:49:46Z

/ok-to-test

tekton-robot · 2021-05-26T12:53:01Z

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/v1alpha1/run_types.go	43.8%	61.5%	17.8
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	95.8%	-0.1
pkg/reconciler/pipelinerun/cancel.go	84.0%	85.2%	1.2
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	83.3%	-0.5

tekton-robot · 2021-05-26T13:11:24Z

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/v1alpha1/run_types.go	43.8%	61.5%	17.8
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	95.8%	-0.1
pkg/reconciler/pipelinerun/cancel.go	84.0%	85.2%	1.2
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	83.3%	-0.5

tekton-robot · 2021-05-26T13:14:02Z

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/v1alpha1/run_types.go	43.8%	61.5%	17.8
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	95.8%	-0.1
pkg/reconciler/pipelinerun/cancel.go	84.0%	85.2%	1.2
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	83.3%	-0.5

tekton-robot · 2021-05-27T11:23: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/apis/pipeline/v1alpha1/run_types.go	43.8%	61.5%	17.8
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	95.8%	-0.1
pkg/reconciler/pipelinerun/cancel.go	84.0%	85.2%	1.2
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	83.3%	-0.5

tekton-robot · 2021-05-27T11:24:03Z

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/v1alpha1/run_types.go	43.8%	61.5%	17.8
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	95.8%	-0.1
pkg/reconciler/pipelinerun/cancel.go	84.0%	85.2%	1.2
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	83.3%	-0.5

tekton-robot · 2021-05-27T12:09:54Z

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/v1alpha1/run_types.go	43.8%	61.5%	17.8
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	95.8%	-0.1
pkg/reconciler/pipelinerun/cancel.go	84.0%	85.2%	1.2
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	83.3%	-0.5

## Changes Today, users can specify a timeout for a component Task of a Pipeline (see [PipelineTask.Timeout]) Remove the existing validation that forbids adding PipelineTask.Timeout to a custom task definition. The Run type will specify a Timeout field to hold this value when created as part of a PipelineRun (or when Runs are created directly). Custom Task authors can read and propagate this field if desired. Tekton-owned controller will not forcibly update the .status of a Run directly. This will be the responsibility of Custom Task controller. For a PipelineRun with either a pipeline level timeout configured and/or the custom task level timout configuration, timeout is updated to the run with same policy as it is for task runs. On timeout, the running run's status is updated with "RunCancelled". A Custom Task author can watch for this status update (i.e. Run.Spec.Status == RunCancelled) and or Run.HasTimedOut() and take any corresponding actions ( i.e. a clean up e.g., cancel a cloud build, stop the waiting timer, tear down the approval listener). Example: apiVersion: tekton.dev/v1alpha1 kind: Run metadata: generateName: simpleexample spec: timeout: 10s # set timeouts here. params: - name: searching value: the purpose of my existence ref: apiVersion: custom.tekton.dev/v1alpha1 kind: Example name: exampleName Unit and end to end test, ensures that on timeout either Pipeline level of task level, the custom task was patched /spec/status as RunCancelled.

1. Removed timeout from custom task limitations 2. Added an example

tekton-robot · 2021-07-23T12:57:17Z

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/v1alpha1/run_types.go	43.8%	61.5%	17.8
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	95.8%	-0.1
pkg/reconciler/pipelinerun/cancel.go	88.9%	89.5%	0.6
pkg/reconciler/pipelinerun/pipelinerun.go	83.4%	82.9%	-0.5

ScrapCodes · 2021-07-23T15:19:05Z

/test tekton-pipeline-unit-tests

ghost · 2021-08-05T12:46:49Z

@imjasonh @afrittoli do you have time to review this PR before the 0.27 release on Monday?

imjasonh

Looks good to me, just some docs comments otherwise this seems good to go.

docs/runs.md

imjasonh · 2021-08-05T13:00:49Z

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -51,7 +52,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
+	k8sApiLables "k8s.io/apimachinery/pkg/labels"


typo: labels

Maybe k8slabels? Or, just keep the original import path, and don't rename it at all?

I renamed because, labels is freely used as a variable name and it is also imported in the same scope. Do you think it is ok to keep it as labels ? (it failed a style check in my IDE though. )

I don't feel strongly either way, maybe k8slabels

tekton-robot · 2021-08-05T13:15:08Z

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/v1alpha1/run_types.go	43.8%	61.5%	17.8
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	95.8%	-0.1
pkg/reconciler/pipelinerun/cancel.go	88.9%	89.5%	0.6
pkg/reconciler/pipelinerun/pipelinerun.go	83.7%	83.2%	-0.5

tekton-robot · 2021-08-05T13:15:39Z

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/v1alpha1/run_types.go	43.8%	61.5%	17.8
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	95.8%	-0.1
pkg/reconciler/pipelinerun/cancel.go	88.9%	89.5%	0.6
pkg/reconciler/pipelinerun/pipelinerun.go	83.7%	83.2%	-0.5

imjasonh · 2021-08-05T13:58:00Z

/test pull-tekton-pipeline-alpha-integration-tests

ghost

I have one nit in the docs but otherwise this looks ready to approve from my pov!

ghost · 2021-08-05T14:24:27Z

docs/runs.md

+#### Developer guide for custom controllers supporting `Timeout`
+
+1. Tekton controllers will never directly update the status of the
+   `Run`, it is the responsibility of the custom task controller to support


Am I right that what we mean here is that the custom task developer is expected to update the status/condition of the Run? It might be worth describing a simple common status/condition/reason that is used for timeouts. I'm thinking something like:

Once resources or timers are cleaned up it is good practice to set a
condition on the Run's status of Succeeded/False with a Reason
of RunTimedOut.

tekton-robot · 2021-08-05T15:04:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [vdemeester]

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

Co-authored-by: Jason Hall <jasonhall@redhat.com>

tekton-robot · 2021-08-05T15:22:34Z

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/v1alpha1/run_types.go	43.8%	61.5%	17.8
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	95.8%	-0.1
pkg/reconciler/pipelinerun/cancel.go	88.9%	89.5%	0.6
pkg/reconciler/pipelinerun/pipelinerun.go	83.7%	83.2%	-0.5

ghost · 2021-08-05T15:44:00Z

/test tekton-pipeline-unit-tests

ghost · 2021-08-05T16:11:50Z

/test pull-tekton-pipeline-integration-tests

ghost · 2021-08-05T17:19:26Z

/test pull-tekton-pipeline-integration-tests

ghost · 2021-08-05T18:00:04Z

/lgtm

We already designed and implemented support for `Timeouts` and `TaskSpec` in `Custom Tasks`. `Timeouts`: - TEP: tektoncd/community#433 - PR: tektoncd#3976 `TaskSpec`: - TEP: https://github.com/tektoncd/community/blob/main/teps/0061-allow-custom-task-to-be-embedded-in-pipeline.md - PR: tektoncd#3901 This item cleans up the todo items in `RunSpec`.

We already designed and implemented support for `Timeouts` and `TaskSpec` in `Custom Tasks`. `Timeouts`: - TEP: tektoncd/community#433 - PR: #3976 `TaskSpec`: - TEP: https://github.com/tektoncd/community/blob/main/teps/0061-allow-custom-task-to-be-embedded-in-pipeline.md - PR: #3901 This item cleans up the todo items in `RunSpec`.

We already designed and implemented support for `Timeouts` and `TaskSpec` in `Custom Tasks`. `Timeouts`: - TEP: tektoncd/community#433 - PR: tektoncd#3976 `TaskSpec`: - TEP: https://github.com/tektoncd/community/blob/main/teps/0061-allow-custom-task-to-be-embedded-in-pipeline.md - PR: tektoncd#3901 This item cleans up the todo items in `RunSpec`.

tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 26, 2021

tekton-robot requested review from dlorenc and vdemeester May 26, 2021 11:04

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 26, 2021

ScrapCodes marked this pull request as draft May 26, 2021 11:12

ScrapCodes marked this pull request as ready for review May 26, 2021 11:15

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 May 26, 2021

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 26, 2021

ScrapCodes marked this pull request as draft May 26, 2021 15:17

ScrapCodes force-pushed the custom-task-timeout branch 2 times, most recently from 4c54723 to 78a913b Compare May 27, 2021 11:21

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2021

ScrapCodes force-pushed the custom-task-timeout branch from 78a913b to c467708 Compare May 27, 2021 12:07

ScrapCodes marked this pull request as ready for review May 27, 2021 12:08

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 May 27, 2021

ScrapCodes force-pushed the custom-task-timeout branch from c467708 to 9f1891f Compare May 27, 2021 12:19

ScrapCodes added 2 commits July 23, 2021 18:13

docs: Implement timeout for custom tasks.

e076b47

1. Removed timeout from custom task limitations 2. Added an example

ScrapCodes force-pushed the custom-task-timeout branch from b09b5cb to e076b47 Compare July 23, 2021 12:54

imjasonh reviewed Aug 5, 2021

View reviewed changes

ScrapCodes force-pushed the custom-task-timeout branch from 67feaa3 to fd3046f Compare August 5, 2021 13:12

ghost reviewed Aug 5, 2021

View reviewed changes

vdemeester approved these changes Aug 5, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2021

Apply suggestions from code review

a60722c

Co-authored-by: Jason Hall <jasonhall@redhat.com>

ScrapCodes force-pushed the custom-task-timeout branch from fd3046f to a60722c Compare August 5, 2021 15:20

tekton-robot assigned ghost Aug 5, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2021

tekton-robot merged commit 50ebf6a into tektoncd:main Aug 5, 2021

This was referenced Jan 13, 2022

Timeout and TaskSpec supported in Custom Tasks jerop/pipeline#2

Closed

Timeout and TaskSpec supported in Custom Tasks #4481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement timeout for custom tasks. #3976

Implement timeout for custom tasks. #3976

ScrapCodes commented May 26, 2021 •

edited

Loading

tekton-robot commented May 26, 2021

afrittoli commented May 26, 2021

tekton-robot commented May 26, 2021

tekton-robot commented May 26, 2021

tekton-robot commented May 26, 2021

tekton-robot commented May 27, 2021

tekton-robot commented May 27, 2021

tekton-robot commented May 27, 2021

tekton-robot commented Jul 23, 2021

ScrapCodes commented Jul 23, 2021

ghost commented Aug 5, 2021

imjasonh left a comment

imjasonh Aug 5, 2021

ScrapCodes Aug 5, 2021

imjasonh Aug 5, 2021

tekton-robot commented Aug 5, 2021

tekton-robot commented Aug 5, 2021

imjasonh commented Aug 5, 2021

ghost left a comment

ghost Aug 5, 2021

tekton-robot commented Aug 5, 2021

tekton-robot commented Aug 5, 2021

ghost commented Aug 5, 2021

ghost commented Aug 5, 2021

ghost commented Aug 5, 2021

ghost commented Aug 5, 2021

Implement timeout for custom tasks. #3976

Implement timeout for custom tasks. #3976

Conversation

ScrapCodes commented May 26, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 26, 2021

afrittoli commented May 26, 2021

tekton-robot commented May 26, 2021

tekton-robot commented May 26, 2021

tekton-robot commented May 26, 2021

tekton-robot commented May 27, 2021

tekton-robot commented May 27, 2021

tekton-robot commented May 27, 2021

tekton-robot commented Jul 23, 2021

ScrapCodes commented Jul 23, 2021

ghost commented Aug 5, 2021

imjasonh left a comment

Choose a reason for hiding this comment

imjasonh Aug 5, 2021

Choose a reason for hiding this comment

ScrapCodes Aug 5, 2021

Choose a reason for hiding this comment

imjasonh Aug 5, 2021

Choose a reason for hiding this comment

tekton-robot commented Aug 5, 2021

tekton-robot commented Aug 5, 2021

imjasonh commented Aug 5, 2021

ghost left a comment

Choose a reason for hiding this comment

ghost Aug 5, 2021

Choose a reason for hiding this comment

tekton-robot commented Aug 5, 2021

tekton-robot commented Aug 5, 2021

ghost commented Aug 5, 2021

ghost commented Aug 5, 2021

ghost commented Aug 5, 2021

ghost commented Aug 5, 2021

ScrapCodes commented May 26, 2021 •

edited

Loading