Ensure containers are stopped on daemon startup by cpuguy83 · Pull Request #35805 · moby/moby

cpuguy83 · 2017-12-14T18:42:49Z

When the containerd 1.0 runtime changes were made, we inadvertently
removed the functionality where any running containers are killed on
startup when not using live-restore.
This change restores that behavior.

ping @fcrisciani

fcrisciani · 2017-12-14T18:54:30Z

LGTM

andrewhsu · 2017-12-14T23:01:14Z

Seeing failures:

FAIL: docker_cli_nat_test.go:62: DockerSuite.TestNetworkLocalhostTCPNat
FAIL: docker_cli_nat_test.go:46: DockerSuite.TestNetworkNat

22:55:14 ----------------------------------------------------------------------
22:55:14 FAIL: docker_cli_nat_test.go:62: DockerSuite.TestNetworkLocalhostTCPNat
22:55:14 
22:55:14 docker_cli_nat_test.go:76:
22:55:14     c.Assert(final, checker.Equals, msg)
22:55:14 ... obtained string = ""
22:55:14 ... expected string = "hi yall"
22:55:14 
22:55:15 ----------------------------------------------------------------------
22:55:15 FAIL: docker_cli_nat_test.go:46: DockerSuite.TestNetworkNat
22:55:15 
22:55:15 docker_cli_nat_test.go:52:
22:55:15     c.Assert(err, check.IsNil)
22:55:15 ... value *net.OpError = &net.OpError{Op:"dial", Net:"tcp", Source:net.Addr(nil), Addr:(*net.TCPAddr)(0xc4203ecb70), Err:(*os.SyscallError)(0xc4204b3900)} ("dial tcp 172.17.0.3:8080: getsockopt: connection refused")
22:55:15 
22:55:16

vdemeester

LGTM 🐯

anusha-ragunathan · 2017-12-15T20:34:33Z

Tested this and it doesnt work. After daemon restart, docker ps shows no live containers, but the process is still running. Probably needs a SIGTERM

anusha-ragunathan · 2017-12-15T21:00:03Z

Adding a SignalProcess call terminates the process cleanly. Needs addition of wait and check logic for process to be dead and SIGKILL after wait.

index 96b39e8..c18379b 100644
--- a/daemon/daemon.go
+++ b/daemon/daemon.go
@@ -44,6 +44,7 @@ import (
 	"github.com/docker/docker/pkg/containerfs"
 	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/plugingetter"
+	"github.com/docker/docker/pkg/signal"
 	"github.com/docker/docker/pkg/sysinfo"
 	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/pkg/truncindex"
@@ -241,12 +242,19 @@ func (daemon *Daemon) restore() error {
 				logrus.Errorf("Failed to restore container %s with containerd: %s", c.ID, err)
 				return
 			}
-			if !alive {
+
+			if !daemon.configStore.LiveRestoreEnabled {
+				err = daemon.containerd.SignalProcess(context.Background(), c.ID, libcontainerd.InitProcessName, int(signal.SignalMap["TERM"]))
+				logrus.WithError(err).Errorf("Failed to kill container %v due to %v", c.ID, err)
+			}
+
+			if !alive || !daemon.configStore.LiveRestoreEnabled {
 				ec, exitedAt, err = daemon.containerd.DeleteTask(context.Background(), c.ID)
 				if err != nil && !errdefs.IsNotFound(err) {
 					logrus.WithError(err).Errorf("Failed to delete container %s from containerd", c.ID)
 					return
 				}
+				alive = false
 			}
 
 			if c.IsRunning() || c.IsPaused() {

fcrisciani · 2017-12-15T21:56:52Z

daemon/daemon.go

if !daemon.configStore.LiveRestoreEnabled, am I missing something?

Yes, you are correct...

andrewhsu · 2017-12-15T23:57:11Z

Just saw failure on experimental build job:

FAIL: docker_cli_network_unix_test.go:1135: DockerNetworkSuite.TestDockerNetworkHostModeUngracefulDaemonRestart

[d271672e27723] waiting for daemon to start
[d271672e27723] daemon started

Daemon d271672e27723 is not started
[d271672e27723] waiting for daemon to start
[d271672e27723] daemon started

docker_cli_network_unix_test.go:1157:
    c.Assert(err, checker.IsNil)
... value *errors.fundamental = condition ""false" == "true"" not true in time (10s) ("condition \"\"false\" == \"true\"\" not true in time (10s)")

thaJeztah · 2017-12-16T00:48:30Z

https://jenkins.dockerproject.org/job/Docker-PRs-experimental/38431/console

23:51:15 ----------------------------------------------------------------------
23:51:15 FAIL: docker_cli_network_unix_test.go:1135: DockerNetworkSuite.TestDockerNetworkHostModeUngracefulDaemonRestart
23:51:15 
23:51:15 [d271672e27723] waiting for daemon to start
23:51:15 [d271672e27723] daemon started
23:51:15 
23:51:15 Daemon d271672e27723 is not started
23:51:15 [d271672e27723] waiting for daemon to start
23:51:15 [d271672e27723] daemon started
23:51:15 
23:51:15 docker_cli_network_unix_test.go:1157:
23:51:15     c.Assert(err, checker.IsNil)
23:51:15 ... value *errors.fundamental = condition ""false" == "true"" not true in time (10s) ("condition \"\"false\" == \"true\"\" not true in time (10s)")
23:51:15 
23:51:15 [d271672e27723] exiting daemon
23:51:30

cpuguy83 · 2017-12-18T18:07:17Z

experimental CI failure is unrelated and is tracked in #35825
All green otherwise.

anusha-ragunathan · 2017-12-18T19:08:17Z

integration/container/restart_test.go

The daemon is not started yet. So remove defer d.Stop

When the containerd 1.0 runtime changes were made, we inadvertantly removed the functionality where any running containers are killed on startup when not using live-restore. This change restores that behavior. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

anusha-ragunathan · 2017-12-18T20:49:41Z

LGTM (Once CI is green)

seemethere

LGTM on green as well

cpuguy83 requested review from dnephin and vdemeester as code owners December 14, 2017 18:42

GordonTheTurtle added the status/0-triage label Dec 14, 2017

cpuguy83 added status/2-code-review and removed status/0-triage labels Dec 14, 2017

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 15, 2017

vdemeester approved these changes Dec 15, 2017

View reviewed changes

cpuguy83 force-pushed the fix_kill_containers_on_restart branch from da15ad0 to 5d7041c Compare December 15, 2017 15:46

cpuguy83 force-pushed the fix_kill_containers_on_restart branch 2 times, most recently from ebd6a08 to c2f3b3b Compare December 15, 2017 21:31

fcrisciani reviewed Dec 15, 2017

View reviewed changes

cpuguy83 force-pushed the fix_kill_containers_on_restart branch 5 times, most recently from ea89a1e to e0cdd4f Compare December 18, 2017 14:09

cpuguy83 added rebuild/experimental and removed status/failing-ci Indicates that the PR in its current state fails the test suite labels Dec 18, 2017

GordonTheTurtle removed the rebuild/experimental label Dec 18, 2017

cpuguy83 added the rebuild/experimental label Dec 18, 2017

GordonTheTurtle removed the rebuild/experimental label Dec 18, 2017

anusha-ragunathan reviewed Dec 18, 2017

View reviewed changes

integration/container/restart_test.go Outdated

Copy link
Copy Markdown

Contributor

anusha-ragunathan Dec 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The daemon is not started yet. So remove defer d.Stop

cpuguy83 force-pushed the fix_kill_containers_on_restart branch from e0cdd4f to e69127b Compare December 18, 2017 19:34

seemethere approved these changes Dec 19, 2017

View reviewed changes

anusha-ragunathan merged commit c3a9f5d into moby:master Dec 19, 2017

cpuguy83 deleted the fix_kill_containers_on_restart branch December 19, 2017 20:21

This was referenced Dec 19, 2017

[17.12] Ensure containers are stopped on daemon startup docker-archive/docker-ce#366

Merged

Misc test improvements #35840

Merged

Conversation

cpuguy83 commented Dec 14, 2017

Uh oh!

fcrisciani commented Dec 14, 2017

Uh oh!

andrewhsu commented Dec 14, 2017 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

anusha-ragunathan commented Dec 15, 2017

Uh oh!

anusha-ragunathan commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fcrisciani Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

andrewhsu commented Dec 15, 2017

Uh oh!

thaJeztah commented Dec 16, 2017

Uh oh!

cpuguy83 commented Dec 18, 2017

Uh oh!

anusha-ragunathan Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

anusha-ragunathan commented Dec 18, 2017

Uh oh!

seemethere left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

andrewhsu commented Dec 14, 2017 •

edited by thaJeztah

Loading

anusha-ragunathan commented Dec 15, 2017 •

edited

Loading