Implement 0 as no timeout for TaskRun and PipelineRun #1040

houshengbo · 2019-07-03T19:18:31Z

Changes

If the timeout is set to 0, there is no timeout for the TaskRun or PipelineRun.
If the timeout is empty, the default timeout 60 mins will be applied.
If the timeout is set to -1, the taskrun is created in pipelinerun after it timed out.

Closes: #978

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

The default timeout of TaskRun and PipelineRun is configurable by editing the value of
`default-timeout-minutes` in config/config-defaults.yaml. The default timeout
is 60 minutes, if it is unset. If it is set to 0, there is no timeout for TaskRun or PipelineRun
by default.

If the `timeout` is unset for the TaskRun or PipelineRun, the default timeout will be picked.
If the `timeout` is set to 0, there is no timeout for the TaskRun or PipelineRun.

vdemeester

Thanks for the PR 🎉 Looks good, just few comments (no blockers)
We should add a release-note item other than release-note, especially as this is a almost-breaking change as the 0 value for timeout now has a different meaning than before.

/cc @bobcatfish @abayer

docs/taskruns.md

pkg/apis/config/default.go

abayer · 2019-07-05T12:33:00Z

/lgtm

abayer · 2019-07-05T16:59:45Z

/lgtm

houshengbo · 2019-07-05T17:36:57Z

/test pull-tekton-pipeline-integration-tests

houshengbo · 2019-07-10T16:05:14Z

/test pull-tekton-pipeline-integration-tests

afrittoli

Nice work, thank you!

afrittoli · 2019-07-12T11:14:24Z

docs/pipelineruns.md

-  - `timeout` - Specifies timeout after which the `PipelineRun` will fail.
+  - [`timeout`] - Specifies timeout after which the `PipelineRun` will fail. If the value of
+    `timeout` is empty, the default timeout will be applied. If the value is set to 0,
+    there is no timeout. `PipelineRun` shares the same default timouet as `TaskRun`. You can


NIT: s/timouet/timeout

afrittoli · 2019-07-12T11:15:23Z

pkg/apis/config/default.go

@@ -27,6 +28,7 @@ const (
 	// ConfigName is the name of the configmap
 	DefaultsConfigName       = "config-defaults"
 	DefaultTimeoutMinutes    = 60
+	NoTimeoutDuration = 0 * time.Minute


NIT: alignment

If the timeout is set to 0, there is no timeout for the TaskRun and PipelineRun. If the timeout is empty, the default timeout 60 mins will be applied. If the timeout is set to -1, the taskrun is created in pipelinerun after it timed out.

houshengbo · 2019-07-12T14:33:01Z

/test pull-tekton-pipeline-integration-tests

bobcatfish

Looking good, thanks again @houshengbo ! 🎉

/lgtm
/approve
/meow space

bobcatfish · 2019-07-15T14:26:24Z

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

 			taskRunTimeout = &metav1.Duration{Duration: time.Until(pTimeoutTime)}
 			if taskRunTimeout.Duration < 0 {
-				taskRunTimeout = &metav1.Duration{Duration: 0 * time.Second}
+				taskRunTimeout = &metav1.Duration{Duration: 1 * time.Second}


the more I look at this, I'm not really sure why this case is being explicitly handled at all - the way that the creation works, we could easily hit a case where by the time a TaskRun is actually created, the current time is after pTimeoutTime, even if this case isn't hit 🤔

But I'm fine with leaving this as-is in this PR.

tekton-robot · 2019-07-15T14:30:40Z

@bobcatfish:

In response to this:

Looking good, thanks again @houshengbo ! 🎉

/lgtm
/approve
/meow space

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.

tekton-robot · 2019-07-15T14:30:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, houshengbo

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~~ [bobcatfish]

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

This was already done in tektoncd#1040 for TaskRuns, but PipelineRuns seem to have been missed. This fixes tektoncd#1303.

This was already done in #1040 for TaskRuns, but PipelineRuns seem to have been missed. This fixes #1303.

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 Jul 3, 2019

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jul 3, 2019

tekton-robot requested review from bobcatfish and dlorenc July 3, 2019 19:18

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 3, 2019

houshengbo force-pushed the no-timeout branch 2 times, most recently from aaec5b5 to 709498e Compare July 3, 2019 20:19

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 3, 2019

houshengbo force-pushed the no-timeout branch from 709498e to 34354d9 Compare July 3, 2019 20:21

houshengbo changed the title ~~WIP: Implement 0 as no timeout for TaskRun~~ Implement 0 as no timeout for TaskRun Jul 3, 2019

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 Jul 3, 2019

houshengbo force-pushed the no-timeout branch 4 times, most recently from 487a0bd to b8265bf Compare July 3, 2019 23:36

vdemeester reviewed Jul 4, 2019

View reviewed changes

docs/taskruns.md Show resolved Hide resolved

pkg/apis/config/default.go Show resolved Hide resolved

tekton-robot requested a review from abayer July 4, 2019 16:10

houshengbo force-pushed the no-timeout branch from b8265bf to 0005045 Compare July 4, 2019 22:43

tekton-robot assigned abayer Jul 5, 2019

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

houshengbo force-pushed the no-timeout branch from 0005045 to de3e249 Compare July 5, 2019 16:56

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2019

houshengbo changed the title ~~Implement 0 as no timeout for TaskRun~~ Implement 0 as no timeout for TaskRun and PipelineRun Jul 5, 2019

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

houshengbo force-pushed the no-timeout branch from de3e249 to 57a884e Compare July 5, 2019 17:09

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2019

houshengbo force-pushed the no-timeout branch from 566012a to c2f6e80 Compare July 9, 2019 21:14

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 9, 2019

houshengbo force-pushed the no-timeout branch 9 times, most recently from 1b3fc99 to df8308d Compare July 10, 2019 14:17

houshengbo force-pushed the no-timeout branch from df8308d to 5d04759 Compare July 11, 2019 19:01

afrittoli reviewed Jul 12, 2019

View reviewed changes

houshengbo force-pushed the no-timeout branch from 5d04759 to 5e328c9 Compare July 12, 2019 13:03

bobcatfish reviewed Jul 15, 2019

View reviewed changes

tekton-robot assigned bobcatfish Jul 15, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2019

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2019

tekton-robot merged commit ab66c3b into tektoncd:master Jul 15, 2019

imjasonh mentioned this pull request Sep 20, 2019

timeout: 0 will fail the pipeline immediately #1303

Closed

dlorenc added a commit to dlorenc/build-pipeline that referenced this pull request Sep 30, 2019

Add support for specifiying "0" as no-timeout for PipelineRuns.

488a54a

This was already done in tektoncd#1040 for TaskRuns, but PipelineRuns seem to have been missed. This fixes tektoncd#1303.

dlorenc mentioned this pull request Sep 30, 2019

Add support for specifiying "0" as no-timeout for PipelineRuns. #1365

Merged

3 tasks

tekton-robot pushed a commit that referenced this pull request Sep 30, 2019

Add support for specifiying "0" as no-timeout for PipelineRuns.

52bc003

This was already done in #1040 for TaskRuns, but PipelineRuns seem to have been missed. This fixes #1303.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement 0 as no timeout for TaskRun and PipelineRun #1040

Implement 0 as no timeout for TaskRun and PipelineRun #1040

houshengbo commented Jul 3, 2019 •

edited

vdemeester left a comment

abayer commented Jul 5, 2019

abayer commented Jul 5, 2019

houshengbo commented Jul 5, 2019

houshengbo commented Jul 10, 2019

afrittoli left a comment

afrittoli Jul 12, 2019

houshengbo Jul 12, 2019

afrittoli Jul 12, 2019

houshengbo Jul 12, 2019

houshengbo commented Jul 12, 2019

bobcatfish left a comment

bobcatfish Jul 15, 2019

tekton-robot commented Jul 15, 2019

tekton-robot commented Jul 15, 2019

Implement 0 as no timeout for TaskRun and PipelineRun #1040

Implement 0 as no timeout for TaskRun and PipelineRun #1040

Conversation

houshengbo commented Jul 3, 2019 • edited

Changes

Submitter Checklist

Release Notes

vdemeester left a comment

Choose a reason for hiding this comment

abayer commented Jul 5, 2019

abayer commented Jul 5, 2019

houshengbo commented Jul 5, 2019

houshengbo commented Jul 10, 2019

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Jul 12, 2019

Choose a reason for hiding this comment

houshengbo Jul 12, 2019

Choose a reason for hiding this comment

afrittoli Jul 12, 2019

Choose a reason for hiding this comment

houshengbo Jul 12, 2019

Choose a reason for hiding this comment

houshengbo commented Jul 12, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Jul 15, 2019

Choose a reason for hiding this comment

tekton-robot commented Jul 15, 2019

tekton-robot commented Jul 15, 2019

houshengbo commented Jul 3, 2019 •

edited