-
Notifications
You must be signed in to change notification settings - Fork 1
use ContainerKill instead of ContainerStop to StopTask #454
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Have you signed the CLA already but the status is still pending? Recheck it. |
Container stop sends the SIGTERM and SIGKILL signals after the timeout has been reached. Setting the timeout to 0 is the same as killing, if that's what you're after? |
Without this change, my tasks are not receiving the stop signals I am configuring. I want to send e.g. SIGQUIT, not the default kill signals. When I was investigating why this was, I noticed that the docker driver does not seem to be using the equivalent of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @dylrich!
This would allow the task.kill_signal
to be specified, which is great. But then we need to follow-up with sending SIGKILL
when the task.kill_timeout
expires.
Because the Podman API can't do that for us with ContainerKill
, you'll need to implement that here as well. As you've noticed in your comment, that'll be similar to what we've done for the in-tree docker
driver here.
return drivers.ErrTaskNotFound | ||
} | ||
|
||
// fixme send proper signal to container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've fixed it now, let's remove this comment!
I wanted to start a conversation about signal handling -- I noticed that the Docker driver was sending a kill signal to the container instead of the stop command and thought it would be nice if the Podman driver did this as well. I wasn't able to run all of the tests successfully locally on clone, so I'm unsure if this breaks some other expectation.