Graceful Pipeline Run Termination #3915

rafalbigaj · 2021-05-05T11:03:11Z

The implementation of TEP-0058: Graceful Pipeline Run Termination.

The new spec.Status values:

StoppedRunFinally - To "stop" (i.e. let the tasks complete, then execute finally tasks) a Pipeline
CancelledRunFinally - To "cancel" (i.e. interrupt any executing non finally tasks, then execute finally tasks)
Cancelled - Same as today's PipelineRunCancelled - i.e. interrupt any executing tasks without running finally tasks

PipelineRunCancelled is deprecated (replaced by Cancelled)

Changes

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

# The new PipelineRun spec statuses have been added to control the way who a PipelineRun is being canceled or stopped.

- "StoppedRunFinally" - To stop (i.e. let the tasks complete, then execute finally tasks) a PipelineRun
- "CancelledRunFinally" - To cancel (i.e. interrupt any executing non finally tasks, then execute finally tasks)
- "Cancelled" - replaces today's "PipelineRunCancelled" - i.e. interrupt any executing tasks without running finally tasks

Support for existing statuses has been left unchanged.

The status "PipelineRunCancelled" is deprecated and replaced by "Cancelled" (it would be removed in v1).

The new states are released as alpha API features.

[1] https://github.com/tektoncd/community/blob/main/teps/0058-graceful-pipeline-run-termination.md

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

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

vdemeester · 2021-05-05T14:15:16Z

/ok-to-test

vdemeester · 2021-05-05T14:16:42Z

/kind feature

tekton-robot · 2021-05-05T14:17:58Z

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
internal/builder/v1beta1/pipeline.go	85.6%	84.7%	-1.0
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	75.8%	73.5%	-2.2
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	100.0%	97.1%	-2.9
pkg/reconciler/pipelinerun/cancel.go	84.0%	81.0%	-3.0
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	83.8%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	69.8%	68.8%	-1.1

internal/builder/v1beta1/pipeline.go

pkg/apis/pipeline/v1beta1/pipelinerun_types.go

souleb · 2021-05-05T12:51:33Z

pkg/apis/pipeline/v1beta1/pipelinerun_validation.go

 	return errs
 }
+
+func validateSpecStatus(status PipelineRunSpecStatus) *apis.FieldError {


would be nice to update the test function

pipeline/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go

Line 34 in 565c55b

func TestPipelineRun_Invalid(t *testing.T) {

Also guard validating the new fields with the feature gate

Thanks! Done.

souleb · 2021-05-05T14:54:05Z

pkg/reconciler/pipelinerun/cancel_test.go

+		pipelineRun: &v1beta1.PipelineRun{
+			ObjectMeta: metav1.ObjectMeta{Name: "test-pipeline-run-cancelled"},
+			Spec: v1beta1.PipelineRunSpec{
+				Status: v1beta1.PipelineRunSpecStatusCancelledDeprecated,


can we have a test with the non deprecated stopping and cancelling fields?

This test was added to cover deprecated status name (PipelineRunCancelled). The already existing ones cover the new status name (Cancelled).

rafalbigaj · 2021-05-06T12:05:46Z

/retest

tekton-robot · 2021-05-06T12:06:46Z

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
internal/builder/v1beta1/pipeline.go	85.6%	84.7%	-1.0
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	75.8%	73.5%	-2.2
pkg/reconciler/pipelinerun/cancel.go	84.0%	88.1%	4.1
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	84.0%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	69.8%	68.4%	-1.4

tekton-robot · 2021-05-07T13:32:19Z

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
internal/builder/v1beta1/pipeline.go	85.6%	84.2%	-1.5
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	75.8%	73.5%	-2.2
pkg/reconciler/pipelinerun/cancel.go	84.0%	88.1%	4.1
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	84.0%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	71.7%	70.3%	-1.4

tekton-robot · 2021-05-07T13:44:46Z

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
internal/builder/v1beta1/pipeline.go	85.6%	84.2%	-1.5
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	75.8%	73.5%	-2.2
pkg/reconciler/pipelinerun/cancel.go	84.0%	88.1%	4.1
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	84.0%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	71.7%	70.3%	-1.4

tekton-robot · 2021-05-07T13:59: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
internal/builder/v1beta1/pipeline.go	85.6%	84.2%	-1.5
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	75.8%	73.5%	-2.2
pkg/reconciler/pipelinerun/cancel.go	84.0%	88.1%	4.1
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	84.0%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	71.7%	70.3%	-1.4

rafalbigaj · 2021-05-07T15:05:54Z

/retest

tekton-robot · 2021-05-07T15:47:42Z

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
internal/builder/v1beta1/pipeline.go	85.6%	84.2%	-1.5
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	75.8%	73.5%	-2.2
pkg/reconciler/pipelinerun/cancel.go	84.0%	88.1%	4.1
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	84.0%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	71.7%	70.3%	-1.4

rafalbigaj · 2021-05-08T10:36:59Z

/test pull-tekton-pipeline-build-tests

rafalbigaj · 2021-05-10T08:32:37Z

/test pull-tekton-pipeline-build-tests

souleb · 2021-05-10T14:50:59Z

pkg/apis/pipeline/v1beta1/pipelinerun_validation.go

+			return nil
+		}
+	}
+	if alphaAPIFieldsEnabled {


Sorry to come back to this again. Here when the alphaFields is not enabled, we might have an error without any information given to the user. Would be nice to have a specific error in that case stating that it fails because the alphaFields are not enabled.

There is a helper function you may use:

pipeline/pkg/apis/pipeline/v1beta1/version_validation.go

Line 30 in b86a9a2

func ValidateEnabledAPIFields(ctx context.Context, featureName, wantVersion string) *apis.FieldError {

Thanks for pointing! Fixed

pritidesai · 2021-06-11T06:29:34Z

We are almost there, thanks a bunch once again for all the effort @rafalbigaj

The changes look great and the pipeline state transitioning is much more meaningful now:

01-terminate-pipelinerun   Unknown     Running     4s
01-terminate-pipelinerun   False       PipelineRunCancelled   49s         43s

01-terminate-pipelinerun   Unknown     Running                   4s
01-terminate-pipelinerun   Unknown     StoppedRunningFinally     37s
01-terminate-pipelinerun   False       Cancelled                 39s

01-terminate-pipelinerun   Unknown     Running                8s
01-terminate-pipelinerun   Unknown     CancelledRunningFinally   18s
01-terminate-pipelinerun   False       Cancelled                 19s         21s

01-terminate-pipelinerun   Unknown     Running   4s
01-terminate-pipelinerun   False       Cancelled   31s         21s

I would like to hear your thoughts on isRunning before approving 🎉

The implementation of TEP-0058: Graceful Pipeline Run Termination. The new `spec.Status` values: - `StoppedRunFinally` - To "stop" (i.e. let the tasks complete, then execute finally tasks) a Pipeline - `CancelledRunFinally` - To "cancel" (i.e. interrupt any executing non finally tasks, then execute finally tasks) - `Cancelled` - Same as today's `PipelineRunCancelled` - i.e. interrupt any executing tasks without running finally tasks The new `PipelineRun` condition reasons: - `CancelledRunningFinally` - indicates that pipeline has been gracefully cancelled, but final tasks are now running - `StoppedRunningFinally` - indicates that pipeline has been gracefully stopped, but final tasks are now running Status `PipelineRunCancelled` is deprecated (replaced by `Cancelled`). The same applies to the condition reason. New features hidden by alpha API fields flag.

tekton-robot · 2021-06-11T07:41:25Z

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
internal/builder/v1beta1/pipeline.go	86.5%	86.7%	0.2
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	73.5%	74.3%	0.8
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	98.3%	98.5%	0.2
pkg/reconciler/pipelinerun/cancel.go	84.0%	88.9%	4.9
pkg/reconciler/pipelinerun/pipelinerun.go	82.9%	83.1%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.0%	92.2%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	71.7%	73.4%	1.8

afrittoli

Thanks for yet another update and for your patience!
I went through all the various tests and coverage again, and everything looks good to me.
I would be nice to have another quick docs PR on top to document that a pending pipeline cannot be cancelled via graceful cancel 🙏
/approve

tekton-robot · 2021-06-11T11:34:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli

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

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

pritidesai · 2021-06-11T15:35:49Z

thank you once again @rafalbigaj for all the work and your patience, the changes look great. Very exciting for this 🎉

/lgtm

With tektoncd/pipeline#3915, there is now a possibility to graceful cancel. As of today, the cli only supports force cancelling. When we updated the dependencies, we didn't change the constant we are using from tektoncd/pipeline and thus, we were using the new value that is only supported when alpha api are enabled — hence the CI is broken. This make the cli use the deprecated field for now, but we need to follow up to support graceful cancellation. Signed-off-by: Vincent Demeester <[email protected]>

With tektoncd/pipeline#3915, there is now a possibility to graceful cancel. As of today, the cli only supports force cancelling. In this patch, a new flag is introduced `--grace` which handles all the scenarios as follows --grace int Gracefully cancel a PipelineRun To use this, you need to change the feature-flags configmap enable-api-fields to alpha instead of stable. Ignored if set to a negative value. Set to 1 if you want to cancel the current running task and directly run the finally tasks. Set to 2 if you want to cancel the remaining non-final task and directly run the finally tasks. Signed-off-by: vinamra28 <[email protected]>

Fixes tektoncd#4611. We deprecated the original cancelled status (with the value `PipelineRunCancelled`) in tektoncd#3915, and we're now at the time for it to be removed in favor of the value `Cancelled`. Signed-off-by: Andrew Bayer <[email protected]>

Fixes #4611. We deprecated the original cancelled status (with the value `PipelineRunCancelled`) in #3915, and we're now at the time for it to be removed in favor of the value `Cancelled`. 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 May 5, 2021

tekton-robot requested review from pritidesai and vdemeester May 5, 2021 11:03

tekton-robot added size/XL Denotes a PR that changes 500-999 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 5, 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 5, 2021

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 5, 2021

souleb reviewed May 5, 2021

View reviewed changes

rafalbigaj force-pushed the main branch from 53b373d to 94b5fe0 Compare May 6, 2021 12:04

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 6, 2021

rafalbigaj force-pushed the main branch from 94b5fe0 to 25533ef Compare May 7, 2021 13:30

rafalbigaj force-pushed the main branch from 25533ef to 898e4a0 Compare May 7, 2021 13:42

rafalbigaj force-pushed the main branch from 898e4a0 to f9b33cf Compare May 7, 2021 13:56

rafalbigaj force-pushed the main branch from f9b33cf to e60eb6c Compare May 7, 2021 15:44

souleb reviewed May 10, 2021

View reviewed changes

rafalbigaj force-pushed the main branch from a71286b to f4037c7 Compare June 11, 2021 07:38

afrittoli reviewed Jun 11, 2021

View reviewed changes

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

tekton-robot assigned pritidesai Jun 11, 2021

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

tekton-robot merged commit c8dc797 into tektoncd:main Jun 11, 2021

This was referenced Jul 9, 2021

Force cancel when using tkn pipelinerun cancel 👼 tektoncd/cli#1415

Merged

Support for Graceful termination tektoncd/cli#1416

Closed

Support for Graceful termination tektoncd/cli#1417

Closed

jerop mentioned this pull request Aug 3, 2021

Graceful Termination: Cancelled Task is Failed (even with Retries) #4136

Merged

5 tasks

vinamra28 mentioned this pull request Jan 10, 2022

Handle graceful termination of Pipeline tektoncd/cli#1479

Merged

4 tasks

jerop mentioned this pull request Feb 23, 2022

TEP-0058: Graceful Termination - PipelineRunCancelled is deprecated #4612

Merged

4 tasks

abayer mentioned this pull request Jun 22, 2022

Remove deprecated PipelineRunSpecStatusCancelledDeprecated #5022

Merged

5 tasks

Graceful Pipeline Run Termination #3915

Graceful Pipeline Run Termination #3915

Uh oh!

Conversation

rafalbigaj commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented May 5, 2021

Uh oh!

vdemeester commented May 5, 2021

Uh oh!

vdemeester commented May 5, 2021

Uh oh!

tekton-robot commented May 5, 2021

Uh oh!

Uh oh!

Uh oh!

souleb May 5, 2021

Choose a reason for hiding this comment

Uh oh!

souleb May 5, 2021

Choose a reason for hiding this comment

Uh oh!

rafalbigaj May 6, 2021

Choose a reason for hiding this comment

Uh oh!

souleb May 5, 2021

Choose a reason for hiding this comment

Uh oh!

rafalbigaj May 7, 2021

Choose a reason for hiding this comment

Uh oh!

rafalbigaj commented May 6, 2021

Uh oh!

tekton-robot commented May 6, 2021

Uh oh!

tekton-robot commented May 7, 2021

Uh oh!

tekton-robot commented May 7, 2021

Uh oh!

tekton-robot commented May 7, 2021

Uh oh!

rafalbigaj commented May 7, 2021

Uh oh!

tekton-robot commented May 7, 2021

Uh oh!

rafalbigaj commented May 8, 2021

Uh oh!

rafalbigaj commented May 10, 2021

Uh oh!

souleb May 10, 2021

Choose a reason for hiding this comment

Uh oh!

rafalbigaj May 11, 2021

Choose a reason for hiding this comment

Uh oh!

pritidesai commented Jun 11, 2021

Uh oh!

tekton-robot commented Jun 11, 2021

Uh oh!

afrittoli left a comment

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Jun 11, 2021

Uh oh!

pritidesai commented Jun 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rafalbigaj commented May 5, 2021 •

edited

Loading