remote kill: don't wait for the container to stop#7572
remote kill: don't wait for the container to stop#7572openshift-merge-robot merged 2 commits intocontainers:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Would you mind removing the "FIXME: pending #7135" skip in |
Invert the branch logic to match the comment. Docker seems to wait for the container while Podman does not. Enable the remote-disabled system test as well. Fixes: containers#7135 Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
Done! @jwhonce, the kill-wait code has been added with commit d924494 but I found no sign in docker that it would actually wait. Did the behaviour change over time? |
Ah, Docker is only waiting on sigkill: // ContainerKill sends signal to the container
// If no signal is given (sig 0), then Kill with SIGKILL and wait
// for the container to exit.
// If a signal is given, then just send it to the container and return.
func (daemon *Daemon) ContainerKill(name string, sig uint64) error {
container, err := daemon.GetContainer(name)
if err != nil {
return err
}
if sig != 0 && !signal.ValidSignalForPlatform(syscall.Signal(sig)) {
return fmt.Errorf("The %s daemon does not support signal %d", runtime.GOOS, sig)
}
// If no signal is passed, or SIGKILL, perform regular Kill (SIGKILL + wait())
if sig == 0 || syscall.Signal(sig) == syscall.SIGKILL {
return daemon.Kill(container)
}
return daemon.killWithSignal(container, int(sig))
} |
|
/hold Let me fix that before |
Docker does not wait unconditionally. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
|
/hold cancel |
| // side of things and mimic that behavior | ||
| // Docker waits for the container to stop if the signal is 0 or | ||
| // SIGKILL. | ||
| if !utils.IsLibpodRequest(r) && (signal == 0 || syscall.Signal(signal) == syscall.SIGKILL) { |
There was a problem hiding this comment.
Why would signal=0 stop the container? I though signal==0 means just check if the PID exists?
There was a problem hiding this comment.
signal == 0 in docker terms means default which in turn is SIGKILL
There was a problem hiding this comment.
That's my understanding too: sig0 is never delivered to the target, it's only used as a check by calling code (for existence, also for EPERM). Good catch.
|
LGTM |
1 similar comment
|
LGTM |
|
I'm going to hold/LGTM and hope tests go green. |
|
@mheon, can you /lgtm again. I accidentally added another commit on top but Cirrus is smart enough to cache results :) |
|
/lgtm
…On Wed, Sep 9, 2020, 10:43 Valentin Rothberg ***@***.***> wrote:
@mheon <https://github.com/mheon>, can you /lgtm again. I accidentally
added another commit on top but Cirrus is smart enough to cache results :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7572 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3AOCBKF67DOO476YLA66DSE6ICTANCNFSM4RCDTK7Q>
.
|
|
/hold cancel |
Invert the branch logic to match the comment. Docker seems to wait for
the container while Podman does not.
Fixes: #7135
Signed-off-by: Valentin Rothberg rothberg@redhat.com