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

support cancellation #13717

Open
chengjoey opened this issue Oct 7, 2024 · 5 comments
Open

support cancellation #13717

chengjoey opened this issue Oct 7, 2024 · 5 comments
Labels
area/shutdown Shutdown Strategy: Stop and Terminate problem/more information needed Not enough information has been provide to diagnose this issue. type/feature Feature request

Comments

@chengjoey
Copy link
Contributor

chengjoey commented Oct 7, 2024

Summary

support cancellation for workflow

Use Cases

Cancel the running workflow. If the current user wants to cancel the executed workflow, they can only delete it, so that the current environment cannot be retained. It is expected that the user can choose to cancel the workflow.

What I thought of is to add a field cancelled in the spec. When it is true, kill all containers through the wait container

The controller injects the downward api volume into the wait container. When the user cancels the workflow, the controller injects values into the annotations (workflows.argoproj.io/cancellation) of the workflow pods. wait listens to specific files under the downward api path. When the file generates content, it calls killContainers to cancel all tasks.

draft pr #13708, test case can refer to e2e test cancel_test


Message from the maintainers:

Love this feature request? Give it a 👍. We prioritise the proposals with the most 👍.

@chengjoey chengjoey added the type/feature Feature request label Oct 7, 2024
@Joibel
Copy link
Member

Joibel commented Oct 7, 2024

There is already a ShutdownStrategy so I'd suggest that we shouldn't add a new field, just add a new value to that.

What is the approach with exit handlers going to be?

@chengjoey
Copy link
Contributor Author

There is already a ShutdownStrategy so I'd suggest that we shouldn't add a new field, just add a new value to that.

okay

What is the approach with exit handlers going to be?

exit handlers by wait container killContainers

@Joibel
Copy link
Member

Joibel commented Oct 7, 2024

What is the approach with exit handlers going to be?

exit handlers by wait container killContainers

I'm not sure what you're trying to say. My question wasn't well phrased, sorry.

I was trying to ask: Will exit-handlers be run in the case of a cancelled workflow? I think there are arguments for both options, and I wondered if you'd thought through the cases and come up with a suggestion. My inclination is to say they should run.

@chengjoey
Copy link
Contributor Author

hi @Joibel , I got what you mean, I haven’t considered exit-handlers yet. I also tend to run exit-handlers. I haven’t added corresponding e2e tests in draft-pr. I will take this into consideration when it is fully implemented.

@agilgur5
Copy link
Member

agilgur5 commented Oct 7, 2024

If the current user wants to cancel the executed workflow, they can only delete it,

Workflows can already be "Stopped" or "Terminated" via the API/CLI/UI.

I'm not sure how "cancelled" is different from either of those?
In fact, there is an existing issue for a Canceled status, #11849 , to reflect the state after "Stop" or "Terminate". Currently the Workflow is considered Failed, which is a bit inaccurate

What is the approach with exit handlers going to be?

Alan asked this because "Stop" and "Terminate" are different based on how they interact with exit handlers, like SIGTERM and SIGKILL, respectively. See also #11626

The controller injects the downward api volume into the wait container. When the user cancels the workflow, the controller injects values into the annotations (workflows.argoproj.io/cancellation) of the workflow pods. wait listens to specific files under the downward api path. When the file generates content, it calls killContainers to cancel all tasks.

This is an interesting alternative that may make #13307 possible 👀
Although it might still be necessary to use pods/exec for killing injected sidecars

@agilgur5 agilgur5 added the area/shutdown Shutdown Strategy: Stop and Terminate label Oct 7, 2024
@agilgur5 agilgur5 added the problem/more information needed Not enough information has been provide to diagnose this issue. label Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/shutdown Shutdown Strategy: Stop and Terminate problem/more information needed Not enough information has been provide to diagnose this issue. type/feature Feature request
Projects
None yet
Development

No branches or pull requests

3 participants