Step timeout #3087

Peaorl · 2020-08-11T13:27:39Z

Changes

This feature allows a Task author to specify a Step timeout in the TaskSpec.

An example use case is when a Task author would like to execute a Step for setting up an execution environment. One may expect this Step to execute within a few seconds. If the execution time takes longer than expected one may rather want to fail fast than wait for the TaskRun timeout to abort the TaskRun (example by @imjasonh).

Closes #1690
Corresponding TEP

Major thanks to @sbwsg for his guidance.

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
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Task authors can now specify a timeout for a Step in the TaskSpec.

tekton-robot · 2020-08-11T13:27:54Z

Hi @Peaorl. 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.

Peaorl · 2020-08-11T13:41:36Z

/kind api-change
/kind feature

ghost · 2020-08-11T13:44:49Z

/ok-to-test

tekton-robot · 2020-08-11T13:49:10Z

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
cmd/entrypoint/runner.go	81.0%	85.7%	4.8
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	83.7%	-0.9
pkg/entrypoint/entrypointer.go	56.4%	56.1%	-0.3
pkg/pod/entrypoint.go	84.4%	84.6%	0.2
pkg/pod/pod.go	87.3%	87.8%	0.6
pkg/pod/status.go	87.0%	86.2%	-0.8
pkg/reconciler/taskrun/taskrun.go	78.3%	78.4%	0.1

tekton-robot · 2020-08-11T15:33: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
cmd/entrypoint/runner.go	81.0%	85.7%	4.8
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	83.7%	-0.9
pkg/entrypoint/entrypointer.go	56.4%	56.1%	-0.3
pkg/pod/entrypoint.go	84.4%	84.6%	0.2
pkg/pod/pod.go	87.3%	87.8%	0.6
pkg/pod/status.go	87.0%	86.2%	-0.8
pkg/reconciler/taskrun/taskrun.go	78.3%	78.4%	0.1

tekton-robot · 2020-08-11T15:41:07Z

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
cmd/entrypoint/runner.go	81.0%	85.7%	4.8
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	83.7%	-0.9
pkg/entrypoint/entrypointer.go	56.4%	56.1%	-0.3
pkg/pod/entrypoint.go	84.4%	84.6%	0.2
pkg/pod/pod.go	87.3%	87.8%	0.6
pkg/pod/status.go	87.0%	86.2%	-0.8
pkg/reconciler/taskrun/taskrun.go	78.3%	78.4%	0.1

tekton-robot · 2020-08-11T15:52: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
cmd/entrypoint/runner.go	81.0%	85.7%	4.8
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	83.7%	-0.9
pkg/entrypoint/entrypointer.go	56.4%	56.1%	-0.3
pkg/pod/entrypoint.go	84.4%	84.6%	0.2
pkg/pod/pod.go	87.3%	87.8%	0.6
pkg/pod/status.go	87.0%	86.2%	-0.8
pkg/reconciler/taskrun/taskrun.go	78.3%	78.4%	0.1

imjasonh · 2020-08-11T16:06:03Z

/kind feature

cmd/entrypoint/runner_test.go

docs/tasks.md

pkg/pod/entrypoint.go

pkg/pod/pod.go

pkg/pod/script.go

pkg/pod/pod_test.go

bobcatfish

Looks like the commits need a bit of a cleanup:

There shouldn't be a merge commit, also looks like f0954c7 is actually #3077 ? probably just needs a rebase at this point

it's too bad there isn't a great solution for getting a review on a change that depends on another PR - sometimes you can open the PR against your own branch to get the feedback. in this case you might be getting double feedback on #3077 since folks would be reviewing that code too

/hold

bobcatfish

Thanks for taking this on!

docs/tasks.md

examples/v1beta1/taskruns/step-timeout.yaml

pkg/reconciler/taskrun/taskrun_test.go

docs/tasks.md

cmd/entrypoint/runner_test.go

cmd/entrypoint/main.go

In light of tektoncd#3087 the need for a `ResultType` that is not exposed as a TaskRunResult or PipelineResourceResult arises. In tektoncd#3087, a `Step` can emit a result indicating a `Step` timeout has occurred. This is a result that should not be exposed hence the need for a new `ResultType` called `InternalTektonResultType`. This commit ensures results of this type are filtered out. Introducing an InternalTektonResultType ensures a future proof solution to internal results that should not be exposed. Aside from the example in tektoncd#3087, a present candidate is the result written out by a Step containing a "StartedAt" key. Currently this result is filtered out with a specific function. Marking it as an InternalTektonResultTypes now allows for this result to automatically be filtered out.

In light of tektoncd#3087 the need for a ResultType that is not exposed as a TaskRunResult or PipelineResourceResult arises. In tektoncd#3087, a Step can emit a result indicating a Step timeout has occurred. This is a result that should not be exposed hence the need for a new ResultType called InternalTektonResultType. This commit ensures results of this type are filtered out. Introducing an InternalTektonResultType ensures a future proof solution to internal results that should not be exposed. Aside from the example in tektoncd#3087, a present candidate is the result written out by a Step containing a "StartedAt" key. Currently this result is filtered out with a specific function. Marking it as an InternalTektonResultTypes now allows for this result to automatically be filtered out.

In light of tektoncd#3087 the need for a ResultType that is not exposed as a TaskRunResult or PipelineResourceResult arises. In tektoncd#3087, a Step can emit a result indicating a Step timeout has occurred. This is a result that should not be exposed hence the need for a new ResultType called InternalTektonResultType. This commit ensures results of this type are filtered out. Introducing an InternalTektonResultType ensures a future proof solution to internal results that should not be exposed. Aside from the example in tektoncd#3087, a present candidate is the result written out by a Step containing a "StartedAt" key. Currently this result is filtered out with a specific function. Marking it as an InternalTektonResultTypes now allows for this result to automatically be filtered out. Additionally this commit brings about refactoring (and sometimes renaming) of functions deemed related to taskrun status updates from pkg/reconciler/taskrun/taskrun.go to pkg/pod/status/status.go. This is accompanied with moving test cases for unexported functions in taskrun.go to test cases for exported functions in status.go that now rely on these unexported functions.

In light of tektoncd#3087 the need for a ResultType that is not exposed as a TaskRunResult or PipelineResourceResult arises. In tektoncd#3087, a Step can emit a result indicating a Step timeout has occurred. This is a result that should not be exposed hence the need for a new ResultType called InternalTektonResultType. This commit ensures results of this type are filtered out. Introducing an InternalTektonResultType ensures a future proof solution to internal results that should not be exposed. Aside from the example in tektoncd#3087, a present candidate is the result written out by a Step containing a "StartedAt" key. Currently this result is filtered out with a specific function. Marking it as an InternalTektonResultTypes now allows for this result to automatically be filtered out. Additionally this commit brings about refactoring (and sometimes renaming) of functions related to converting pod statuses to taskrun statuses from pkg/reconciler/taskrun/taskrun.go to pkg/pod/status/status.go. This is accompanied with moving unit test cases from taskrun_test.go to status_test.go. These unit tests now solely operate on exported functions.

Peaorl · 2020-10-06T14:52:01Z

/test pull-tekton-pipeline-integration-tests

cmd/entrypoint/runner.go

pkg/apis/pipeline/v1beta1/task_types.go

imjasonh · 2020-10-06T16:14:23Z

/lgtm

tekton-robot · 2020-10-06T16:21: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
cmd/entrypoint/runner.go	81.0%	84.0%	3.0
pkg/apis/pipeline/v1beta1/task_validation.go	95.0%	95.8%	0.8
pkg/entrypoint/entrypointer.go	56.4%	64.0%	7.6
pkg/pod/entrypoint.go	84.4%	85.1%	0.7
pkg/pod/status.go	93.3%	92.4%	-0.9

imjasonh · 2020-10-06T16:34:13Z

/lgtm

Peaorl · 2020-10-06T16:51:06Z

/retest

imjasonh

/lgtm

tekton-robot · 2020-10-06T19:28:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ImJasonH, sbwsg

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~~ [ImJasonH,sbwsg]

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

vdemeester

/lgtm

ghost · 2020-10-07T14:13:39Z

Looks like all commits have been merged into 1. Given approval and lgtms I'm going to cancel the hold.

/hold cancel

This feature allows a Task author to specify a Step timeout in a Taskrun. An example use case is when a Task author would like to execute a Step for setting up an execution environment. One may expect this Step to execute within a few seconds. If the execution time takes longer than expected one may rather want to fail fast instead of waiting for the TaskRun timeout to abort the TaskRun. Closes tektoncd#1690

tekton-robot · 2020-10-07T15:22:26Z

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
cmd/entrypoint/runner.go	81.0%	84.0%	3.0
pkg/apis/pipeline/v1beta1/task_validation.go	95.0%	95.8%	0.8
pkg/entrypoint/entrypointer.go	56.4%	64.0%	7.6
pkg/pod/entrypoint.go	84.4%	85.1%	0.7
pkg/pod/status.go	93.3%	92.4%	-0.9

Peaorl · 2020-10-07T15:24:04Z

Automatic merge/rebase into master didn't go smoothly after a new commit yesterday so the code has been updated.
Additionally, I increased the timeout limit from 30s to 60s in examples/v1beta1/taskrun/workspace-in-sidecar.yaml because it seems like the cluster is too resource constrained at times for it to complete in time.

imjasonh · 2020-10-07T15:35:14Z

/lgtm

This was actually done a while back in October 2020 when tektoncd/pipeline#3087 merged. Signed-off-by: Andrew Bayer <[email protected]>

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 11, 2020

tekton-robot requested review from bobcatfish and pritidesai August 11, 2020 13:27

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 Aug 11, 2020

tekton-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. labels Aug 11, 2020

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 Aug 11, 2020

Peaorl force-pushed the step-timeout branch from 600b439 to 38e3768 Compare August 11, 2020 15:30

Peaorl force-pushed the step-timeout branch from 38e3768 to 0c2ff5c Compare August 11, 2020 15:36

imjasonh requested changes Aug 11, 2020

View reviewed changes

bobcatfish reviewed Aug 12, 2020

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 12, 2020

bobcatfish reviewed Aug 12, 2020

View reviewed changes

ghost deleted a comment from vivi2050 Aug 14, 2020

imjasonh reviewed Oct 6, 2020

View reviewed changes

cmd/entrypoint/runner.go Show resolved Hide resolved

pkg/apis/pipeline/v1beta1/task_types.go Outdated Show resolved Hide resolved

tekton-robot assigned imjasonh Oct 6, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2020

Peaorl force-pushed the step-timeout branch from d26dd1f to 02cfa1f Compare October 6, 2020 16:19

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2020

imjasonh reviewed Oct 6, 2020

View reviewed changes

imjasonh approved these changes Oct 6, 2020

View reviewed changes

vdemeester reviewed Oct 7, 2020

View reviewed changes

tekton-robot assigned vdemeester Oct 7, 2020

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2020

Peaorl force-pushed the step-timeout branch from 02cfa1f to 17e7f90 Compare October 7, 2020 15:20

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2020

tekton-robot merged commit 66096db into tektoncd:master Oct 7, 2020

Peaorl deleted the step-timeout branch November 3, 2020 13:28

abayer added a commit to abayer/community that referenced this pull request Dec 13, 2021

TEP-0014: Step Timeout - mark as implemented

0496f44

This was actually done a while back in October 2020 when tektoncd/pipeline#3087 merged. Signed-off-by: Andrew Bayer <[email protected]>

abayer mentioned this pull request Dec 13, 2021

TEP-0014: Step Timeout - mark as implemented tektoncd/community#574

Merged

tekton-robot pushed a commit to tektoncd/community that referenced this pull request Dec 14, 2021

TEP-0014: Step Timeout - mark as implemented

bdf3908

This was actually done a while back in October 2020 when tektoncd/pipeline#3087 merged. Signed-off-by: Andrew Bayer <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step timeout #3087

Step timeout #3087

Peaorl commented Aug 11, 2020 •

edited

Loading

tekton-robot commented Aug 11, 2020

Peaorl commented Aug 11, 2020

ghost commented Aug 11, 2020

tekton-robot commented Aug 11, 2020

tekton-robot commented Aug 11, 2020

tekton-robot commented Aug 11, 2020

tekton-robot commented Aug 11, 2020

imjasonh commented Aug 11, 2020

bobcatfish left a comment

bobcatfish left a comment

Peaorl commented Oct 6, 2020

imjasonh commented Oct 6, 2020

tekton-robot commented Oct 6, 2020

imjasonh commented Oct 6, 2020

Peaorl commented Oct 6, 2020

imjasonh left a comment

tekton-robot commented Oct 6, 2020

vdemeester left a comment

ghost commented Oct 7, 2020

tekton-robot commented Oct 7, 2020

Peaorl commented Oct 7, 2020 •

edited

Loading

imjasonh commented Oct 7, 2020

Step timeout #3087

Step timeout #3087

Conversation

Peaorl commented Aug 11, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Aug 11, 2020

Peaorl commented Aug 11, 2020

ghost commented Aug 11, 2020

tekton-robot commented Aug 11, 2020

tekton-robot commented Aug 11, 2020

tekton-robot commented Aug 11, 2020

tekton-robot commented Aug 11, 2020

imjasonh commented Aug 11, 2020

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

Peaorl commented Oct 6, 2020

imjasonh commented Oct 6, 2020

tekton-robot commented Oct 6, 2020

imjasonh commented Oct 6, 2020

Peaorl commented Oct 6, 2020

imjasonh left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 6, 2020

vdemeester left a comment

Choose a reason for hiding this comment

ghost commented Oct 7, 2020

tekton-robot commented Oct 7, 2020

Peaorl commented Oct 7, 2020 • edited Loading

imjasonh commented Oct 7, 2020

Peaorl commented Aug 11, 2020 •

edited

Loading

Peaorl commented Oct 7, 2020 •

edited

Loading