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

Allow stopping of paused container #34027

Merged
merged 1 commit into from Jul 13, 2017

Conversation

Projects
None yet
6 participants
@cpuguy83
Contributor

cpuguy83 commented Jul 9, 2017

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

@boaz1337

👍

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 9, 2017

Member

ping @coolljt0725 - guess you want to have a look

Member

thaJeztah commented Jul 9, 2017

ping @coolljt0725 - guess you want to have a look

@coolljt0725

This comment has been minimized.

Show comment
Hide comment
@coolljt0725

coolljt0725 Jul 10, 2017

Contributor

I like this, 👍 LGTM

Contributor

coolljt0725 commented Jul 10, 2017

I like this, 👍 LGTM

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Jul 10, 2017

Collaborator

SGTM

Collaborator

vieux commented Jul 10, 2017

SGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 11, 2017

Member

powerpc failing on #34051

Member

thaJeztah commented Jul 11, 2017

powerpc failing on #34051

@thaJeztah

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?

}
} else {
container.ExitOnNext()
unpause = container.Paused
}
if !daemon.IsShuttingDown() {

This comment has been minimized.

@thaJeztah

thaJeztah Jul 12, 2017

Member

can you swap the if/else here?

if daemon.IsShuttingDown() {
    unpause = container.Paused
} else {
    container.HasBeenManuallyStopped = true
}
@thaJeztah

thaJeztah Jul 12, 2017

Member

can you swap the if/else here?

if daemon.IsShuttingDown() {
    unpause = container.Paused
} else {
    container.HasBeenManuallyStopped = true
}

This comment has been minimized.

@cpuguy83

cpuguy83 Jul 12, 2017

Contributor

Removed else.

@cpuguy83

cpuguy83 Jul 12, 2017

Contributor

Removed else.

Show outdated Hide outdated daemon/kill.go Outdated
@@ -103,6 +103,13 @@ func (daemon *Daemon) killWithSignal(container *containerpkg.Container, sig int)
}
}
if unpause {

This comment has been minimized.

@thaJeztah

thaJeztah Jul 12, 2017

Member

Looks like we try to resume the container, even if the daemon.kill() above returned "container not found" or "no such process"

@thaJeztah

thaJeztah Jul 12, 2017

Member

Looks like we try to resume the container, even if the daemon.kill() above returned "container not found" or "no such process"

This comment has been minimized.

@cpuguy83

cpuguy83 Jul 12, 2017

Contributor

Fixed

@cpuguy83

cpuguy83 Jul 12, 2017

Contributor

Fixed

Allow stopping of paused container
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 <cpuguy83@gmail.com>
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jul 12, 2017

Contributor

@thaJeztah I think we only want to unpause if it's a stop-type signal. signal.

Contributor

cpuguy83 commented Jul 12, 2017

@thaJeztah I think we only want to unpause if it's a stop-type signal. signal.

@thaJeztah

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Jul 13, 2017

ping @coolljt0725 @vieux PTAL

@coolljt0725

This comment has been minimized.

Show comment
Hide comment
@coolljt0725

coolljt0725 Jul 13, 2017

Contributor

LGTM

Contributor

coolljt0725 commented Jul 13, 2017

LGTM

@coolljt0725 coolljt0725 merged commit 6fdb2fb into moby:master Jul 13, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 35636 has succeeded
Details
janky Jenkins build Docker-PRs 44250 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 4625 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 15628 has succeeded
Details
z Jenkins build Docker-PRs-s390x 4325 has succeeded
Details

@cpuguy83 cpuguy83 deleted the cpuguy83:15853_allow_stopping_paused_container branch Sep 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment