From 743f5afcd13659e2b6b76791415876797e2ede52 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 17 Apr 2018 10:52:54 -0700 Subject: [PATCH 1/2] TestContainerStopTimeout: fix The unit test is checking that setting of non-default StopTimeout works, but it checked the value of StopSignal instead. Amazingly, the test was working since the default StopSignal is SIGTERM, which has the numeric value of 15. Fixes: commit e66d21089 ("Add config parameter to change ...") Signed-off-by: Kir Kolyshkin --- container/container_unit_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/container/container_unit_test.go b/container/container_unit_test.go index bf45df942e..fbee6e5eb0 100644 --- a/container/container_unit_test.go +++ b/container/container_unit_test.go @@ -52,9 +52,9 @@ func TestContainerStopTimeout(t *testing.T) { c = &Container{ Config: &container.Config{StopTimeout: &stopTimeout}, } - s = c.StopSignal() - if s != 15 { - t.Fatalf("Expected 15, got %v", s) + s = c.StopTimeout() + if s != stopTimeout { + t.Fatalf("Expected %v, got %v", stopTimeout, s) } } From 69b4fe406540c7989bd99b2244ed67ced96e17c7 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 16 Apr 2018 16:58:42 -0700 Subject: [PATCH 2/2] daemon.ContainerStop(): fix for a negative timeout 1. As daemon.ContainerStop() documentation says, > If a negative number of seconds is given, ContainerStop > will wait for a graceful termination. but since commit cfdf84d5d04c8ee (PR #32237) this is no longer the case. This happens because `context.WithTimeout(ctx, timeout)` is implemented as `WithDeadline(ctx, time.Now().Add(timeout))`, resulting in a deadline which is in the past. To fix, don't use WithDeadline() if the timeout is negative. 2. Add a test case to validate the correct behavior and as a means to prevent a similar regression in the future. 3. Fix/improve daemon.ContainerStop() and client.ContainerStop() description for clarity and completeness. 4. Fix/improve DefaultStopTimeout description. Fixes: cfdf84d5d04c ("Update Container Wait") Signed-off-by: Kir Kolyshkin --- client/container_stop.go | 9 +++-- container/container_unix.go | 3 +- daemon/stop.go | 36 ++++++++++--------- integration/container/stop_test.go | 55 ++++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 20 deletions(-) diff --git a/client/container_stop.go b/client/container_stop.go index ca316666a9..dcb1bed5eb 100644 --- a/client/container_stop.go +++ b/client/container_stop.go @@ -8,8 +8,13 @@ import ( "golang.org/x/net/context" ) -// ContainerStop stops a container without terminating the process. -// The process is blocked until the container stops or the timeout expires. +// ContainerStop stops a container. In case the container fails to stop +// gracefully within a time frame specified by the timeout argument, +// it is forcefully terminated (killed). +// +// If the timeout is nil, the container's StopTimeout value is used, if set, +// otherwise the engine default. A negative timeout value can be specified, +// meaning no timeout, i.e. no forceful termination is performed. func (cli *Client) ContainerStop(ctx context.Context, containerID string, timeout *time.Duration) error { query := url.Values{} if timeout != nil { diff --git a/container/container_unix.go b/container/container_unix.go index e5cecf3166..c85c78bb91 100644 --- a/container/container_unix.go +++ b/container/container_unix.go @@ -22,7 +22,8 @@ import ( ) const ( - // DefaultStopTimeout is the timeout (in seconds) for the syscall signal used to stop a container. + // DefaultStopTimeout sets the default time, in seconds, to wait + // for the graceful container stop before forcefully terminating it. DefaultStopTimeout = 10 containerSecretMountPath = "/run/secrets" diff --git a/daemon/stop.go b/daemon/stop.go index 45f523848f..c3ac09056a 100644 --- a/daemon/stop.go +++ b/daemon/stop.go @@ -10,13 +10,15 @@ import ( "github.com/sirupsen/logrus" ) -// ContainerStop looks for the given container and terminates it, -// waiting the given number of seconds before forcefully killing the -// container. If a negative number of seconds is given, ContainerStop -// will wait for a graceful termination. An error is returned if the -// container is not found, is already stopped, or if there is a -// problem stopping the container. -func (daemon *Daemon) ContainerStop(name string, seconds *int) error { +// ContainerStop looks for the given container and stops it. +// In case the container fails to stop gracefully within a time duration +// specified by the timeout argument, in seconds, it is forcefully +// terminated (killed). +// +// If the timeout is nil, the container's StopTimeout value is used, if set, +// otherwise the engine default. A negative timeout value can be specified, +// meaning no timeout, i.e. no forceful termination is performed. +func (daemon *Daemon) ContainerStop(name string, timeout *int) error { container, err := daemon.GetContainer(name) if err != nil { return err @@ -24,21 +26,17 @@ func (daemon *Daemon) ContainerStop(name string, seconds *int) error { if !container.IsRunning() { return containerNotModifiedError{running: false} } - if seconds == nil { + if timeout == nil { stopTimeout := container.StopTimeout() - seconds = &stopTimeout + timeout = &stopTimeout } - if err := daemon.containerStop(container, *seconds); err != nil { + if err := daemon.containerStop(container, *timeout); err != nil { return errdefs.System(errors.Wrapf(err, "cannot stop container: %s", name)) } return nil } -// containerStop halts a container by sending a stop signal, waiting for the given -// duration in seconds, and then calling SIGKILL and waiting for the -// process to exit. If a negative duration is given, Stop will wait -// for the initial signal forever. If the container is not running Stop returns -// immediately. +// containerStop sends a stop signal, waits, sends a kill signal. func (daemon *Daemon) containerStop(container *containerpkg.Container, seconds int) error { if !container.IsRunning() { return nil @@ -69,8 +67,12 @@ func (daemon *Daemon) containerStop(container *containerpkg.Container, seconds i } // 2. Wait for the process to exit on its own - ctx, cancel := context.WithTimeout(context.Background(), time.Duration(seconds)*time.Second) - defer cancel() + ctx := context.Background() + if seconds >= 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, time.Duration(seconds)*time.Second) + defer cancel() + } if status := <-container.Wait(ctx, containerpkg.WaitConditionNotRunning); status.Err() != nil { logrus.Infof("Container %v failed to exit within %d seconds of signal %d - using the force", container.ID, seconds, stopSignal) diff --git a/integration/container/stop_test.go b/integration/container/stop_test.go index 5cb970ba58..592f585327 100644 --- a/integration/container/stop_test.go +++ b/integration/container/stop_test.go @@ -3,6 +3,7 @@ package container // import "github.com/docker/docker/integration/container" import ( "context" "fmt" + "strconv" "strings" "testing" "time" @@ -42,6 +43,60 @@ func TestStopContainerWithRestartPolicyAlways(t *testing.T) { } } +// TestStopContainerWithTimeout checks that ContainerStop with +// a timeout works as documented, i.e. in case of negative timeout +// waiting is not limited (issue #35311). +func TestStopContainerWithTimeout(t *testing.T) { + defer setupTest(t)() + client := request.NewAPIClient(t) + ctx := context.Background() + + testCmd := container.WithCmd("sh", "-c", "sleep 2 && exit 42") + testData := []struct { + doc string + timeout int + expectedExitCode int + }{ + // In case container is forcefully killed, 137 is returned, + // otherwise the exit code from the above script + { + "zero timeout: expect forceful container kill", + 0, 137, + }, + { + "too small timeout: expect forceful container kill", + 1, 137, + }, + { + "big enough timeout: expect graceful container stop", + 3, 42, + }, + { + "unlimited timeout: expect graceful container stop", + -1, 42, + }, + } + + for _, d := range testData { + d := d + t.Run(strconv.Itoa(d.timeout), func(t *testing.T) { + t.Parallel() + id := container.Run(t, ctx, client, testCmd) + + timeout := time.Duration(d.timeout) * time.Second + err := client.ContainerStop(ctx, id, &timeout) + assert.NilError(t, err) + + poll.WaitOn(t, container.IsStopped(ctx, client, id), + poll.WithDelay(100*time.Millisecond)) + + inspect, err := client.ContainerInspect(ctx, id) + assert.NilError(t, err) + assert.Equal(t, inspect.State.ExitCode, d.expectedExitCode) + }) + } +} + func TestDeleteDevicemapper(t *testing.T) { skip.IfCondition(t, testEnv.DaemonInfo.Driver != "devicemapper")