From 32802bc7d94639e79c5b959b20bf50ad6d7c8ac8 Mon Sep 17 00:00:00 2001 From: Ruilin Li Date: Mon, 11 Jun 2018 17:59:26 +0800 Subject: [PATCH 1/4] do not stop health check before sending signal Docker daemon always stops healthcheck before sending signal to a container now. However, when we use "docker kill" to send signals other than SIGTERM or SIGKILL to a container, such as SIGINT, daemon still stops container health check though container process handles the signal normally and continues to work. Signed-off-by: Ruilin Li (cherry picked from commit da574f93432e600fda561da5e6983e7f69b364a9) Signed-off-by: Dani Louca --- daemon/kill.go | 2 -- integration-cli/docker_cli_health_test.go | 26 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/daemon/kill.go b/daemon/kill.go index 3e97158a0c..0fe7412913 100644 --- a/daemon/kill.go +++ b/daemon/kill.go @@ -64,8 +64,6 @@ func (daemon *Daemon) killWithSignal(container *containerpkg.Container, sig int) container.Lock() defer container.Unlock() - daemon.stopHealthchecks(container) - if !container.Running { return errNotRunning(container.ID) } diff --git a/integration-cli/docker_cli_health_test.go b/integration-cli/docker_cli_health_test.go index 4fb63994bb..6a94e6b61a 100644 --- a/integration-cli/docker_cli_health_test.go +++ b/integration-cli/docker_cli_health_test.go @@ -165,3 +165,29 @@ ENTRYPOINT /bin/sh -c "sleep 600"`)) waitForHealthStatus(c, name, "starting", "healthy") } + +// GitHub #37263 +func (s *DockerSuite) TestHealthKillContainer(c *check.C) { + testRequires(c, DaemonIsLinux) // busybox doesn't work on Windows + + imageName := "testhealth" + buildImageSuccessfully(c, imageName, build.WithDockerfile(`FROM busybox +HEALTHCHECK --interval=1s --timeout=5s --retries=5 CMD /bin/sh -c "sleep 1" +ENTRYPOINT /bin/sh -c "sleep 600"`)) + + name := "test_health_kill" + dockerCmd(c, "run", "-d", "--name", name, imageName) + defer func() { + dockerCmd(c, "rm", "-f", name) + dockerCmd(c, "rmi", imageName) + }() + + // Start + dockerCmd(c, "start", name) + waitForHealthStatus(c, name, "starting", "healthy") + + dockerCmd(c, "kill", "-s", "SIGINT", name) + out, _ := dockerCmd(c, "inspect", "--format={{.State.Health.Status}}", name) + c.Check(out, checker.Equals, "healthy\n") + +} From 8533594ad66623c9bdbc8e2c2572402767064794 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Fri, 12 Jul 2019 13:26:14 -0700 Subject: [PATCH 2/4] Move kill health test to integration Signed-off-by: Brian Goff (cherry picked from commit f8aef6a92f5961f2615ada37b7d108774a0821e0) Signed-off-by: Dani Louca --- integration-cli/docker_cli_health_test.go | 26 -------------------- integration/container/health_test.go | 29 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 26 deletions(-) diff --git a/integration-cli/docker_cli_health_test.go b/integration-cli/docker_cli_health_test.go index 6a94e6b61a..4fb63994bb 100644 --- a/integration-cli/docker_cli_health_test.go +++ b/integration-cli/docker_cli_health_test.go @@ -165,29 +165,3 @@ ENTRYPOINT /bin/sh -c "sleep 600"`)) waitForHealthStatus(c, name, "starting", "healthy") } - -// GitHub #37263 -func (s *DockerSuite) TestHealthKillContainer(c *check.C) { - testRequires(c, DaemonIsLinux) // busybox doesn't work on Windows - - imageName := "testhealth" - buildImageSuccessfully(c, imageName, build.WithDockerfile(`FROM busybox -HEALTHCHECK --interval=1s --timeout=5s --retries=5 CMD /bin/sh -c "sleep 1" -ENTRYPOINT /bin/sh -c "sleep 600"`)) - - name := "test_health_kill" - dockerCmd(c, "run", "-d", "--name", name, imageName) - defer func() { - dockerCmd(c, "rm", "-f", name) - dockerCmd(c, "rmi", imageName) - }() - - // Start - dockerCmd(c, "start", name) - waitForHealthStatus(c, name, "starting", "healthy") - - dockerCmd(c, "kill", "-s", "SIGINT", name) - out, _ := dockerCmd(c, "inspect", "--format={{.State.Health.Status}}", name) - c.Check(out, checker.Equals, "healthy\n") - -} diff --git a/integration/container/health_test.go b/integration/container/health_test.go index f4117f562a..bb21abcc3c 100644 --- a/integration/container/health_test.go +++ b/integration/container/health_test.go @@ -9,6 +9,7 @@ import ( containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/client" "github.com/docker/docker/integration/internal/container" + "gotest.tools/assert" "gotest.tools/poll" "gotest.tools/skip" ) @@ -32,6 +33,34 @@ func TestHealthCheckWorkdir(t *testing.T) { poll.WaitOn(t, pollForHealthStatus(ctx, client, cID, types.Healthy), poll.WithDelay(100*time.Millisecond)) } +// GitHub #37263 +// Do not stop healthchecks just because we sent a signal to the container +func TestHealthKillContainer(t *testing.T) { + defer setupTest(t)() + + ctx := context.Background() + client := testEnv.APIClient() + + id := container.Run(ctx, t, client, func(c *container.TestContainerConfig) { + c.Config.Healthcheck = &containertypes.HealthConfig{ + Test: []string{"CMD-SHELL", "sleep 1"}, + Interval: time.Second, + Retries: 5, + } + }) + + ctxPoll, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + poll.WaitOn(t, pollForHealthStatus(ctxPoll, client, id, "healthy"), poll.WithDelay(100*time.Millisecond)) + + err := client.ContainerKill(ctx, id, "SIGUSR1") + assert.NilError(t, err) + + ctxPoll, cancel = context.WithTimeout(ctx, 30*time.Second) + defer cancel() + poll.WaitOn(t, pollForHealthStatus(ctxPoll, client, id, "healthy"), poll.WithDelay(100*time.Millisecond)) +} + func pollForHealthStatus(ctx context.Context, client client.APIClient, containerID string, healthStatus string) func(log poll.LogT) poll.Result { return func(log poll.LogT) poll.Result { inspect, err := client.ContainerInspect(ctx, containerID) From ef5dd6e46de6930b7adb2715ebc0473f25ba7995 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 19 Jul 2019 16:09:50 +0200 Subject: [PATCH 3/4] Skip TestHealthKillContainer on Windows This test is failing on Windows currently: ``` 11:59:47 --- FAIL: TestHealthKillContainer (8.12s) 11:59:47 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1 `` That test was added recently in https://github.com/moby/moby/pull/39454, but rewritten in a commit in the same PR: https://github.com/moby/moby/commit/f8aef6a92f5961f2615ada37b7d108774a0821e0 In that rewrite, there were some changes: - originally it was skipped on Windows, but the rewritten test doesn't have that skip: ```go testRequires(c, DaemonIsLinux) // busybox doesn't work on Windows ``` - the original test used `SIGINT`, but the new one uses `SIGUSR1` Analysis: - The Error bubbles up from: https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal.go#L29-L44 - Interestingly; `ContainerKill` should validate if a signal is valid for the given platform, but somehow we don't hit that part; https://github.com/moby/moby/blob/f1b5612f2008827fdcf838abb4539064c682181e/daemon/kill.go#L40-L48 - Windows only looks to support 2 signals currently https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal_windows.go#L17-L26 - Upstream Golang looks to define `SIGINT` as well; https://github.com/golang/go/blob/77f9b2728eb08456899e6500328e00ec4829dddf/src/runtime/defs_windows.go#L44 - This looks like the current list of Signals upstream in Go; https://github.com/golang/sys/blob/3b58ed4ad3395d483fc92d5d14123ce2c3581fec/windows/types_windows.go#L52-L67 ```go const ( // More invented values for signals SIGHUP = Signal(0x1) SIGINT = Signal(0x2) SIGQUIT = Signal(0x3) SIGILL = Signal(0x4) SIGTRAP = Signal(0x5) SIGABRT = Signal(0x6) SIGBUS = Signal(0x7) SIGFPE = Signal(0x8) SIGKILL = Signal(0x9) SIGSEGV = Signal(0xb) SIGPIPE = Signal(0xd) SIGALRM = Signal(0xe) SIGTERM = Signal(0xf) ) ``` Signed-off-by: Sebastiaan van Stijn (cherry picked from commit eeaa0b30d47e6b9dac8d8ea2ced6d5ce44c24463) Signed-off-by: Dani Louca --- integration/container/health_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/integration/container/health_test.go b/integration/container/health_test.go index bb21abcc3c..7039462a01 100644 --- a/integration/container/health_test.go +++ b/integration/container/health_test.go @@ -36,6 +36,7 @@ func TestHealthCheckWorkdir(t *testing.T) { // GitHub #37263 // Do not stop healthchecks just because we sent a signal to the container func TestHealthKillContainer(t *testing.T) { + skip.If(t, testEnv.OSType == "windows", "Windows only supports SIGKILL and SIGTERM? See https://github.com/moby/moby/issues/39574") defer setupTest(t)() ctx := context.Background() From 8fca769bd5363b4dd8a7861ab1c9011a425fa30a Mon Sep 17 00:00:00 2001 From: Dani Louca Date: Tue, 13 Aug 2019 14:33:28 -0400 Subject: [PATCH 4/4] Fixing integration test Signed-off-by: Dani Louca (cherry picked from commit 614daf117112e8c9576967764281cc6fe617bbb2) Signed-off-by: Dani Louca --- integration/container/health_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/container/health_test.go b/integration/container/health_test.go index 7039462a01..64284c8860 100644 --- a/integration/container/health_test.go +++ b/integration/container/health_test.go @@ -42,7 +42,7 @@ func TestHealthKillContainer(t *testing.T) { ctx := context.Background() client := testEnv.APIClient() - id := container.Run(ctx, t, client, func(c *container.TestContainerConfig) { + id := container.Run(t, ctx, client, func(c *container.TestContainerConfig) { c.Config.Healthcheck = &containertypes.HealthConfig{ Test: []string{"CMD-SHELL", "sleep 1"}, Interval: time.Second,