Browse code

Merge pull request #36874 from kolyshkin/stop-timeout

daemon.ContainerStop(): fix for a negative timeout

Brian Goff authored on 2018/05/31 02:38:42
Showing 5 changed files
... ...
@@ -8,8 +8,13 @@ import (
8 8
 	timetypes "github.com/docker/docker/api/types/time"
9 9
 )
10 10
 
11
-// ContainerStop stops a container without terminating the process.
12
-// The process is blocked until the container stops or the timeout expires.
11
+// ContainerStop stops a container. In case the container fails to stop
12
+// gracefully within a time frame specified by the timeout argument,
13
+// it is forcefully terminated (killed).
14
+//
15
+// If the timeout is nil, the container's StopTimeout value is used, if set,
16
+// otherwise the engine default. A negative timeout value can be specified,
17
+// meaning no timeout, i.e. no forceful termination is performed.
13 18
 func (cli *Client) ContainerStop(ctx context.Context, containerID string, timeout *time.Duration) error {
14 19
 	query := url.Values{}
15 20
 	if timeout != nil {
... ...
@@ -52,9 +52,9 @@ func TestContainerStopTimeout(t *testing.T) {
52 52
 	c = &Container{
53 53
 		Config: &container.Config{StopTimeout: &stopTimeout},
54 54
 	}
55
-	s = c.StopSignal()
56
-	if s != 15 {
57
-		t.Fatalf("Expected 15, got %v", s)
55
+	s = c.StopTimeout()
56
+	if s != stopTimeout {
57
+		t.Fatalf("Expected %v, got %v", stopTimeout, s)
58 58
 	}
59 59
 }
60 60
 
... ...
@@ -23,7 +23,8 @@ import (
23 23
 )
24 24
 
25 25
 const (
26
-	// DefaultStopTimeout is the timeout (in seconds) for the syscall signal used to stop a container.
26
+	// DefaultStopTimeout sets the default time, in seconds, to wait
27
+	// for the graceful container stop before forcefully terminating it.
27 28
 	DefaultStopTimeout = 10
28 29
 
29 30
 	containerSecretMountPath = "/run/secrets"
... ...
@@ -10,13 +10,15 @@ import (
10 10
 	"github.com/sirupsen/logrus"
11 11
 )
12 12
 
13
-// ContainerStop looks for the given container and terminates it,
14
-// waiting the given number of seconds before forcefully killing the
15
-// container. If a negative number of seconds is given, ContainerStop
16
-// will wait for a graceful termination. An error is returned if the
17
-// container is not found, is already stopped, or if there is a
18
-// problem stopping the container.
19
-func (daemon *Daemon) ContainerStop(name string, seconds *int) error {
13
+// ContainerStop looks for the given container and stops it.
14
+// In case the container fails to stop gracefully within a time duration
15
+// specified by the timeout argument, in seconds, it is forcefully
16
+// terminated (killed).
17
+//
18
+// If the timeout is nil, the container's StopTimeout value is used, if set,
19
+// otherwise the engine default. A negative timeout value can be specified,
20
+// meaning no timeout, i.e. no forceful termination is performed.
21
+func (daemon *Daemon) ContainerStop(name string, timeout *int) error {
20 22
 	container, err := daemon.GetContainer(name)
21 23
 	if err != nil {
22 24
 		return err
... ...
@@ -24,21 +26,17 @@ func (daemon *Daemon) ContainerStop(name string, seconds *int) error {
24 24
 	if !container.IsRunning() {
25 25
 		return containerNotModifiedError{running: false}
26 26
 	}
27
-	if seconds == nil {
27
+	if timeout == nil {
28 28
 		stopTimeout := container.StopTimeout()
29
-		seconds = &stopTimeout
29
+		timeout = &stopTimeout
30 30
 	}
31
-	if err := daemon.containerStop(container, *seconds); err != nil {
31
+	if err := daemon.containerStop(container, *timeout); err != nil {
32 32
 		return errdefs.System(errors.Wrapf(err, "cannot stop container: %s", name))
33 33
 	}
34 34
 	return nil
35 35
 }
36 36
 
37
-// containerStop halts a container by sending a stop signal, waiting for the given
38
-// duration in seconds, and then calling SIGKILL and waiting for the
39
-// process to exit. If a negative duration is given, Stop will wait
40
-// for the initial signal forever. If the container is not running Stop returns
41
-// immediately.
37
+// containerStop sends a stop signal, waits, sends a kill signal.
42 38
 func (daemon *Daemon) containerStop(container *containerpkg.Container, seconds int) error {
43 39
 	if !container.IsRunning() {
44 40
 		return nil
... ...
@@ -69,8 +67,12 @@ func (daemon *Daemon) containerStop(container *containerpkg.Container, seconds i
69 69
 	}
70 70
 
71 71
 	// 2. Wait for the process to exit on its own
72
-	ctx, cancel := context.WithTimeout(context.Background(), time.Duration(seconds)*time.Second)
73
-	defer cancel()
72
+	ctx := context.Background()
73
+	if seconds >= 0 {
74
+		var cancel context.CancelFunc
75
+		ctx, cancel = context.WithTimeout(ctx, time.Duration(seconds)*time.Second)
76
+		defer cancel()
77
+	}
74 78
 
75 79
 	if status := <-container.Wait(ctx, containerpkg.WaitConditionNotRunning); status.Err() != nil {
76 80
 		logrus.Infof("Container %v failed to exit within %d seconds of signal %d - using the force", container.ID, seconds, stopSignal)
... ...
@@ -3,6 +3,7 @@ package container // import "github.com/docker/docker/integration/container"
3 3
 import (
4 4
 	"context"
5 5
 	"fmt"
6
+	"strconv"
6 7
 	"strings"
7 8
 	"testing"
8 9
 	"time"
... ...
@@ -42,6 +43,60 @@ func TestStopContainerWithRestartPolicyAlways(t *testing.T) {
42 42
 	}
43 43
 }
44 44
 
45
+// TestStopContainerWithTimeout checks that ContainerStop with
46
+// a timeout works as documented, i.e. in case of negative timeout
47
+// waiting is not limited (issue #35311).
48
+func TestStopContainerWithTimeout(t *testing.T) {
49
+	defer setupTest(t)()
50
+	client := request.NewAPIClient(t)
51
+	ctx := context.Background()
52
+
53
+	testCmd := container.WithCmd("sh", "-c", "sleep 2 && exit 42")
54
+	testData := []struct {
55
+		doc              string
56
+		timeout          int
57
+		expectedExitCode int
58
+	}{
59
+		// In case container is forcefully killed, 137 is returned,
60
+		// otherwise the exit code from the above script
61
+		{
62
+			"zero timeout: expect forceful container kill",
63
+			0, 137,
64
+		},
65
+		{
66
+			"too small timeout: expect forceful container kill",
67
+			1, 137,
68
+		},
69
+		{
70
+			"big enough timeout: expect graceful container stop",
71
+			3, 42,
72
+		},
73
+		{
74
+			"unlimited timeout: expect graceful container stop",
75
+			-1, 42,
76
+		},
77
+	}
78
+
79
+	for _, d := range testData {
80
+		d := d
81
+		t.Run(strconv.Itoa(d.timeout), func(t *testing.T) {
82
+			t.Parallel()
83
+			id := container.Run(t, ctx, client, testCmd)
84
+
85
+			timeout := time.Duration(d.timeout) * time.Second
86
+			err := client.ContainerStop(ctx, id, &timeout)
87
+			assert.NilError(t, err)
88
+
89
+			poll.WaitOn(t, container.IsStopped(ctx, client, id),
90
+				poll.WithDelay(100*time.Millisecond))
91
+
92
+			inspect, err := client.ContainerInspect(ctx, id)
93
+			assert.NilError(t, err)
94
+			assert.Equal(t, inspect.State.ExitCode, d.expectedExitCode)
95
+		})
96
+	}
97
+}
98
+
45 99
 func TestDeleteDevicemapper(t *testing.T) {
46 100
 	skip.If(t, testEnv.DaemonInfo.Driver != "devicemapper")
47 101
 	skip.If(t, testEnv.IsRemoteDaemon, "cannot start daemon on remote test run")