Allow stopping of paused container#34027
Conversation
2793589 to
f07b477
Compare
|
ping @coolljt0725 - guess you want to have a look |
|
I like this, 👍 LGTM |
|
SGTM |
f07b477 to
4dd579a
Compare
|
powerpc failing on #34051 |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
Gee, that kill.go file is really difficult to follow, there's a lot of weird logic in there.
Looking at this, I wonder why we have all the conditions to determine if we want or don't want to unpause the container.
Can't we simply modify daemon.kill() to:
func (daemon *Daemon) kill(c *containerpkg.Container, sig int) error {
if err := daemon.containerd.Signal(c.ID, sig); err != nil {
return err
}
if c.Paused {
// above kill signal will be sent once resume is finished
if err := daemon.containerd.Resume(c.ID); err != nil {
logrus.Warn("Cannot unpause container %s: %s", c.ID, err)
}
}
return nil
}Basically; if we want to send a signal to the container, we unpause (un-freeze) it afterwards, otherwise the signal never reaches the process.
Are there any conditions we can think of where that is not the desired behaviour?
There was a problem hiding this comment.
can you swap the if/else here?
if daemon.IsShuttingDown() {
unpause = container.Paused
} else {
container.HasBeenManuallyStopped = true
}There was a problem hiding this comment.
Having a bit of trouble understanding this case;
- if the container is not stopped due to the daemon is shutting down, it's "manually stopped" ✅
- if the container is stopped due to the daemon shutting down, we have to unpause ❓
Isn't unpause always needed if it is paused?
There was a problem hiding this comment.
No, unpause is only needed for kill signals.
In the case where the daemon is not shutting down, it's already going to be set. But we also need to make sure that when the daemon is shutting down that we set it.
There was a problem hiding this comment.
But, I guess it should already be set in both cases since it should be a stop sig on shutdown.
There was a problem hiding this comment.
Looks like we try to resume the container, even if the daemon.kill() above returned "container not found" or "no such process"
When a container is paused, signals are sent once the container has been unpaused. Instead of forcing the user to unpause a container before they can ever send a signal, allow the user to send the signals, and in the case of a stop signal, automatically unpause the container afterwards. This is much safer than unpausing the container first then sending a signal (what a user is currently forced to do), as the container may be paused for very good reasons and should not be unpaused except for stopping. Note that not even SIGKILL is possible while a process is paused, but it is killed the instant it is unpaused. Signed-off-by: Brian Goff <[email protected]>
4dd579a to
c3feb04
Compare
|
@thaJeztah I think we only want to unpause if it's a stop-type signal. signal. |
|
ping @coolljt0725 @vieux PTAL |
|
LGTM |
6 participants
When a container is paused, signals are sent once the container has been
unpaused.
Instead of forcing the user to unpause a container before they can ever
send a signal, allow the user to send the signals, and in the case of a
stop signal, automatically unpause the container afterwards.
This is much safer than unpausing the container first then sending a
signal (what a user is currently forced to do), as the container may be
paused for very good reasons and should not be unpaused except for
stopping.
Note that not even SIGKILL is possible while a process is paused,
but it is killed the instant it is unpaused.
Closes #15853
Closes #28366