Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for new graceful termination options from TEP-0058 #2094

Closed
AlanGreene opened this issue Jun 20, 2021 · 7 comments · Fixed by #2398
Closed

Add support for new graceful termination options from TEP-0058 #2094

AlanGreene opened this issue Jun 20, 2021 · 7 comments · Fixed by #2398
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@AlanGreene
Copy link
Member

AlanGreene commented Jun 20, 2021

Add support for the new graceful termination feature added in Pipelines v0.25 (added as alpha) which provides three different ways to terminate a PipelineRun, differentiating between 'Stop' and 'Cancel'.

  • "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.

There are some additional considerations here:

  • feature detection - can we tell whether the feature is enabled or not? If not, should we wait for it to be promoted in a future Pipeline release?

  • action name - the TEP differentiates between 'Stop' and 'Cancel'
    Using 'Cancel' as the action name was problematic in the past as it's the same as the option to dismiss the confirmation dialog, so that's why we originally went with 'Stop', otherwise we end up with something like this:

    image

    We could use something like 'Stop PipelineRun' and 'Cancel PipelineRun' as the action labels to reduce the ambiguity but this could still cause some confusion.

  • should we have a single action or separate them out? If we go with a single action in the overflow menu how do we allow the user to select between the different types of termination? Radio button selection in the confirmation dialog for example?

Originally posted by @AlanGreene in #2069 (comment)

@AlanGreene AlanGreene added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 20, 2021
@tekton-robot
Copy link
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 15, 2021
@AlanGreene
Copy link
Member Author

Still something we want to add
/lifecycle frozen

@tekton-robot tekton-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 15, 2021
AlanGreene added a commit to AlanGreene/dashboard that referenced this issue Feb 28, 2022
TEP-0058 Graceful Termination deprecated the existing `PipelineRunCancelled`
status. Instead, runs have the status reason `Cancelled`.

The feature is still in alpha, so to test it out you can edit the `feature-flags`
ConfigMap to set `enable-api-fields: alpha`, then use one of the following values
when cancelling the PipelineRun:
- `Cancelled` replaces the existing `PipelineRunCancelled`, interrupts any
  executing tasks and does not run finally tasks
- `StoppedRunFinally` lets any executing tasks complete normally but does not
  start any new non-finally tasks, then runs finally tasks
- `CancelledRunFinally` interrupts any executing non-finally tasks, then executes
  finally tasks

In all 3 cases above, the PipelineRun status reason is `Cancelled`

Update tests and storybook to include 'Cancelled' variants.

The Dashboard API will still use `PipelineRunCancelled` to stop a PipelineRun.
A future change once the feature has been promoted to beta will switch to the
replacement `Cancelled` value. Some UI change may also be made to support the
alternative behaviours, tracking this in tektoncd#2094
AlanGreene added a commit to AlanGreene/dashboard that referenced this issue Mar 1, 2022
TEP-0058 Graceful Termination deprecated the existing `PipelineRunCancelled`
status. Instead, runs have the status reason `Cancelled`.

The feature is still in alpha, so to test it out you can edit the `feature-flags`
ConfigMap to set `enable-api-fields: alpha`, then use one of the following values
when cancelling the PipelineRun:
- `Cancelled` replaces the existing `PipelineRunCancelled`, interrupts any
  executing tasks and does not run finally tasks
- `StoppedRunFinally` lets any executing tasks complete normally but does not
  start any new non-finally tasks, then runs finally tasks
- `CancelledRunFinally` interrupts any executing non-finally tasks, then executes
  finally tasks

In all 3 cases above, the PipelineRun status reason is `Cancelled`

Update tests and storybook to include 'Cancelled' variants.

The Dashboard API will still use `PipelineRunCancelled` to stop a PipelineRun.
A future change once the feature has been promoted to beta will switch to the
replacement `Cancelled` value. Some UI change may also be made to support the
alternative behaviours, tracking this in tektoncd#2094
tekton-robot pushed a commit that referenced this issue Mar 1, 2022
TEP-0058 Graceful Termination deprecated the existing `PipelineRunCancelled`
status. Instead, runs have the status reason `Cancelled`.

The feature is still in alpha, so to test it out you can edit the `feature-flags`
ConfigMap to set `enable-api-fields: alpha`, then use one of the following values
when cancelling the PipelineRun:
- `Cancelled` replaces the existing `PipelineRunCancelled`, interrupts any
  executing tasks and does not run finally tasks
- `StoppedRunFinally` lets any executing tasks complete normally but does not
  start any new non-finally tasks, then runs finally tasks
- `CancelledRunFinally` interrupts any executing non-finally tasks, then executes
  finally tasks

In all 3 cases above, the PipelineRun status reason is `Cancelled`

Update tests and storybook to include 'Cancelled' variants.

The Dashboard API will still use `PipelineRunCancelled` to stop a PipelineRun.
A future change once the feature has been promoted to beta will switch to the
replacement `Cancelled` value. Some UI change may also be made to support the
alternative behaviours, tracking this in #2094
@AlanGreene
Copy link
Member Author

AlanGreene commented Apr 5, 2022

Promotion of TEP-0058 fields to beta and deprecation of the PipelineRunCancelled status are being tracked in tektoncd/pipeline#4611

Similar issue to this one tracking CLI plans: tektoncd/cli#1553

@williamlfish
Copy link

williamlfish commented Apr 19, 2022

@AlanGreene what are your thoughts on a 3 option radio btn situation in the overlay that essentially offers a trimmed down version of the descriptions above? None of them are default and the submit btn is disabled until one is selected?

  • "Stop Gracefully " - Let the running tasks complete, execute finally.
  • "Cancel Gracefully" - Stop all tasks, execute finally.
  • "Cancel" - Stop everything, do not run finally.

or is this ^^ to wordy?

As for checking the version, would a simple switch on the pipeline version catch that? It also looks like there are a few other helper functions that can get resources if needed. Either way, it would benefit devs to display that there are more features available with an upgrade, by either having the other options disabled, or some kind of toast or messaging.

@AlanGreene
Copy link
Member Author

Yes I think radio buttons make sense here, at least we have room to explain the difference between the options in the modal.

With radio button groups one radio button should always be selected by default, in this case I think it would be the 'Cancel' option which is equivalent to our current behaviour. It probably makes sense to persist the user's choice in localStorage so we can remember it next time they open the modal.

In terms of the labels, I think we'll have to play with this a bit to find the right balance of clarity vs. wordiness.

  • Cancel: interrupt non-finally tasks, skip finally
  • Cancel with finally: interrupt non-finally tasks, execute finally tasks
  • Stop with finally: complete currently running non-finally tasks, execute finally tasks

Is it important to clarify that no new tasks will be scheduled (apart from finally) or is this already apparent to the user?
If we feel the descriptions aren't always necessary we could hide them in a tooltip icon after the radio button labels or after the radio group title. This way they don't clutter the UI and users can easily discover the differences / refer to them until they're familiar with the feature at which point they're no longer needed and easily ignored.

Should we use the status values in the labels, e.g. StoppedRunFinally or go with something like 'Stop gracefully' / 'Stop with finally'? Are there any translation issues? finally wouldn't be translated in any of the strings since it's referring to a specific field in the spec.

In terms of version support, we could try to detect the installed Pipelines version using the existing code (useProperties etc.), and either display the new options or an InlineNotification about the required minimum version for this feature to prompt folks to upgrade. We would have to fallback to the current PipelineRunCancelled status if they're running an older version. Looks like the current plan is to keep support for the existing status for at least 3 Pipelines releases once the new values are promoted. At that point we would come back and cleanup the old status, the notification, etc.

@williamlfish
Copy link

So radio buttons and having "Cancel" as default, along with saving the user choice all sound good to me.

Labeling is still a bit it of a head scratcher though. Is the intent of cancel+finally, and stop+finally to stop and cancel a pipeline gracefully? If so, it feels appropriate to use that wording. Perhaps something like

  • Cancel ℹ️
  • Cancel Gracefully ℹ️
  • Stop Gracefully ℹ️
    with the ℹ️ some kind of icon representing "more info"?

or even wording that is not as closely related to the actual statuses, but are potentially more descriptive to the users who are not familiar with tektons inner workings?

  • Stop ( just stop everything )
  • Finish ( stop, and run finally )
  • Wrap-up ( finish running tasks, skip others, run finally )

If I'm not mistaken the newest release of the pipelines has this implemented no? I'm happy to start messing with a pr and different versions of the pipelines to see what feels like the cleanest implementation of the version check/get unit testing in place, etc. while we hash out the labels. Once the wording is in place updating the labels on that pr or with a subsequent pr should be pretty low weight.

@AlanGreene
Copy link
Member Author

Yes Graceful Termination is available in Pipelines v0.35 👍

Absolutely, changing the strings is cheap so go ahead with the rest and we can iterate on the labels / tooltips for the UI.

My main problem at the moment is with 'cancel' vs. 'graceful' as both of the 'cancel' options interrupt any currently running non-finally tasks which I wouldn't consider 'graceful'. I'd also be hesitant to use language that's not reflected in the API / documentation for the feature or could be problematic for translation (e.g. stop / finish). Also using 'stop' in the UI for anything other than the 'StoppedRunFinally' option would be confusing as 'stop' has a specific meaning for this feature.

For that reason I'm currently leaning towards 'Cancel with finally' with more info in a tooltip rather than 'Cancel gracefully'. It's still concise, while providing an indication that the default 'Cancel' just bails out and won't run finally tasks. Open to suggestions / feedback.

  • Cancel ℹ️
  • Cancel with finally ℹ️
  • Stop with finally ℹ️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants