Browse code

Fix race condition between exec start and resize

Signed-off-by: David Wang <00107082@163.com>

David Wang authored on 2018/06/08 12:07:48
Showing 6 changed files
... ...
@@ -16,7 +16,7 @@ import (
16 16
 	"github.com/docker/docker/pkg/pools"
17 17
 	"github.com/docker/docker/pkg/signal"
18 18
 	"github.com/docker/docker/pkg/term"
19
-	"github.com/opencontainers/runtime-spec/specs-go"
19
+	specs "github.com/opencontainers/runtime-spec/specs-go"
20 20
 	"github.com/pkg/errors"
21 21
 	"github.com/sirupsen/logrus"
22 22
 )
... ...
@@ -249,6 +249,9 @@ func (d *Daemon) ContainerExecStart(ctx context.Context, name string, stdin io.R
249 249
 	ec.Lock()
250 250
 	c.ExecCommands.Lock()
251 251
 	systemPid, err := d.containerd.Exec(ctx, c.ID, ec.ID, p, cStdin != nil, ec.InitializeStdio)
252
+	// the exec context should be ready, or error happened.
253
+	// close the chan to notify readiness
254
+	close(ec.Started)
252 255
 	if err != nil {
253 256
 		c.ExecCommands.Unlock()
254 257
 		ec.Unlock()
... ...
@@ -15,6 +15,7 @@ import (
15 15
 // examined both during and after completion.
16 16
 type Config struct {
17 17
 	sync.Mutex
18
+	Started      chan struct{}
18 19
 	StreamConfig *stream.Config
19 20
 	ID           string
20 21
 	Running      bool
... ...
@@ -40,6 +41,7 @@ func NewConfig() *Config {
40 40
 	return &Config{
41 41
 		ID:           stringid.GenerateNonCryptoID(),
42 42
 		StreamConfig: stream.NewConfig(),
43
+		Started:      make(chan struct{}),
43 44
 	}
44 45
 }
45 46
 
... ...
@@ -3,6 +3,7 @@ package daemon // import "github.com/docker/docker/daemon"
3 3
 import (
4 4
 	"context"
5 5
 	"fmt"
6
+	"time"
6 7
 
7 8
 	"github.com/docker/docker/libcontainerd"
8 9
 )
... ...
@@ -37,5 +38,13 @@ func (daemon *Daemon) ContainerExecResize(name string, height, width int) error
37 37
 	if err != nil {
38 38
 		return err
39 39
 	}
40
-	return daemon.containerd.ResizeTerminal(context.Background(), ec.ContainerID, ec.ID, width, height)
40
+	// TODO: the timeout is hardcoded here, it would be more flexible to make it
41
+	// a parameter in resize request context, which would need API changes.
42
+	timeout := 10 * time.Second
43
+	select {
44
+	case <-ec.Started:
45
+		return daemon.containerd.ResizeTerminal(context.Background(), ec.ContainerID, ec.ID, width, height)
46
+	case <-time.After(timeout):
47
+		return fmt.Errorf("timeout waiting for exec session ready")
48
+	}
41 49
 }
42 50
new file mode 100644
... ...
@@ -0,0 +1,103 @@
0
+// +build linux
1
+
2
+package daemon
3
+
4
+import (
5
+	"context"
6
+	"testing"
7
+
8
+	"github.com/docker/docker/container"
9
+	"github.com/docker/docker/daemon/exec"
10
+	"github.com/gotestyourself/gotestyourself/assert"
11
+)
12
+
13
+// This test simply verify that when a wrong ID used, a specific error should be returned for exec resize.
14
+func TestExecResizeNoSuchExec(t *testing.T) {
15
+	n := "TestExecResize"
16
+	d := &Daemon{
17
+		execCommands: exec.NewStore(),
18
+	}
19
+	c := &container.Container{
20
+		ExecCommands: exec.NewStore(),
21
+	}
22
+	ec := &exec.Config{
23
+		ID: n,
24
+	}
25
+	d.registerExecCommand(c, ec)
26
+	err := d.ContainerExecResize("nil", 24, 8)
27
+	assert.ErrorContains(t, err, "No such exec instance")
28
+}
29
+
30
+type execResizeMockContainerdClient struct {
31
+	MockContainerdClient
32
+	ProcessID   string
33
+	ContainerID string
34
+	Width       int
35
+	Height      int
36
+}
37
+
38
+func (c *execResizeMockContainerdClient) ResizeTerminal(ctx context.Context, containerID, processID string, width, height int) error {
39
+	c.ProcessID = processID
40
+	c.ContainerID = containerID
41
+	c.Width = width
42
+	c.Height = height
43
+	return nil
44
+}
45
+
46
+// This test is to make sure that when exec context is ready, resize should call ResizeTerminal via containerd client.
47
+func TestExecResize(t *testing.T) {
48
+	n := "TestExecResize"
49
+	width := 24
50
+	height := 8
51
+	ec := &exec.Config{
52
+		ID:          n,
53
+		ContainerID: n,
54
+		Started:     make(chan struct{}),
55
+	}
56
+	close(ec.Started)
57
+	mc := &execResizeMockContainerdClient{}
58
+	d := &Daemon{
59
+		execCommands: exec.NewStore(),
60
+		containerd:   mc,
61
+		containers:   container.NewMemoryStore(),
62
+	}
63
+	c := &container.Container{
64
+		ExecCommands: exec.NewStore(),
65
+		State:        &container.State{Running: true},
66
+	}
67
+	d.containers.Add(n, c)
68
+	d.registerExecCommand(c, ec)
69
+	err := d.ContainerExecResize(n, height, width)
70
+	assert.NilError(t, err)
71
+	assert.Equal(t, mc.Width, width)
72
+	assert.Equal(t, mc.Height, height)
73
+	assert.Equal(t, mc.ProcessID, n)
74
+	assert.Equal(t, mc.ContainerID, n)
75
+}
76
+
77
+// This test is to make sure that when exec context is not ready, a timeout error should happen.
78
+// TODO: the expect running time for this test is 10s, which would be too long for unit test.
79
+func TestExecResizeTimeout(t *testing.T) {
80
+	n := "TestExecResize"
81
+	width := 24
82
+	height := 8
83
+	ec := &exec.Config{
84
+		ID:          n,
85
+		ContainerID: n,
86
+		Started:     make(chan struct{}),
87
+	}
88
+	mc := &execResizeMockContainerdClient{}
89
+	d := &Daemon{
90
+		execCommands: exec.NewStore(),
91
+		containerd:   mc,
92
+		containers:   container.NewMemoryStore(),
93
+	}
94
+	c := &container.Container{
95
+		ExecCommands: exec.NewStore(),
96
+		State:        &container.State{Running: true},
97
+	}
98
+	d.containers.Add(n, c)
99
+	d.registerExecCommand(c, ec)
100
+	err := d.ContainerExecResize(n, height, width)
101
+	assert.ErrorContains(t, err, "timeout waiting for exec session ready")
102
+}
0 103
new file mode 100644
... ...
@@ -0,0 +1,65 @@
0
+// +build linux
1
+
2
+package daemon
3
+
4
+import (
5
+	"context"
6
+	"time"
7
+
8
+	"github.com/containerd/containerd"
9
+	"github.com/docker/docker/libcontainerd"
10
+	specs "github.com/opencontainers/runtime-spec/specs-go"
11
+)
12
+
13
+// Mock containerd client implementation, for unit tests.
14
+type MockContainerdClient struct {
15
+}
16
+
17
+func (c *MockContainerdClient) Version(ctx context.Context) (containerd.Version, error) {
18
+	return containerd.Version{}, nil
19
+}
20
+func (c *MockContainerdClient) Restore(ctx context.Context, containerID string, attachStdio libcontainerd.StdioCallback) (alive bool, pid int, err error) {
21
+	return false, 0, nil
22
+}
23
+func (c *MockContainerdClient) Create(ctx context.Context, containerID string, spec *specs.Spec, runtimeOptions interface{}) error {
24
+	return nil
25
+}
26
+func (c *MockContainerdClient) Start(ctx context.Context, containerID, checkpointDir string, withStdin bool, attachStdio libcontainerd.StdioCallback) (pid int, err error) {
27
+	return 0, nil
28
+}
29
+func (c *MockContainerdClient) SignalProcess(ctx context.Context, containerID, processID string, signal int) error {
30
+	return nil
31
+}
32
+func (c *MockContainerdClient) Exec(ctx context.Context, containerID, processID string, spec *specs.Process, withStdin bool, attachStdio libcontainerd.StdioCallback) (int, error) {
33
+	return 0, nil
34
+}
35
+func (c *MockContainerdClient) ResizeTerminal(ctx context.Context, containerID, processID string, width, height int) error {
36
+	return nil
37
+}
38
+func (c *MockContainerdClient) CloseStdin(ctx context.Context, containerID, processID string) error {
39
+	return nil
40
+}
41
+func (c *MockContainerdClient) Pause(ctx context.Context, containerID string) error  { return nil }
42
+func (c *MockContainerdClient) Resume(ctx context.Context, containerID string) error { return nil }
43
+func (c *MockContainerdClient) Stats(ctx context.Context, containerID string) (*libcontainerd.Stats, error) {
44
+	return nil, nil
45
+}
46
+func (c *MockContainerdClient) ListPids(ctx context.Context, containerID string) ([]uint32, error) {
47
+	return nil, nil
48
+}
49
+func (c *MockContainerdClient) Summary(ctx context.Context, containerID string) ([]libcontainerd.Summary, error) {
50
+	return nil, nil
51
+}
52
+func (c *MockContainerdClient) DeleteTask(ctx context.Context, containerID string) (uint32, time.Time, error) {
53
+	return 0, time.Time{}, nil
54
+}
55
+func (c *MockContainerdClient) Delete(ctx context.Context, containerID string) error { return nil }
56
+func (c *MockContainerdClient) Status(ctx context.Context, containerID string) (libcontainerd.Status, error) {
57
+	return "null", nil
58
+}
59
+func (c *MockContainerdClient) UpdateResources(ctx context.Context, containerID string, resources *libcontainerd.Resources) error {
60
+	return nil
61
+}
62
+func (c *MockContainerdClient) CreateCheckpoint(ctx context.Context, containerID, checkpointDir string, exit bool) error {
63
+	return nil
64
+}
... ...
@@ -71,16 +71,17 @@ func (s *DockerSuite) TestExecResizeImmediatelyAfterExecStart(c *check.C) {
71 71
 		defer conn.Close()
72 72
 
73 73
 		_, rc, err := request.Post(fmt.Sprintf("/exec/%s/resize?h=24&w=80", execID), request.ContentType("text/plain"))
74
-		// It's probably a panic of the daemon if io.ErrUnexpectedEOF is returned.
75
-		if err == io.ErrUnexpectedEOF {
76
-			return fmt.Errorf("The daemon might have crashed.")
74
+		if err != nil {
75
+			// It's probably a panic of the daemon if io.ErrUnexpectedEOF is returned.
76
+			if err == io.ErrUnexpectedEOF {
77
+				return fmt.Errorf("The daemon might have crashed.")
78
+			}
79
+			// Other error happened, should be reported.
80
+			return fmt.Errorf("Fail to exec resize immediately after start. Error: %q", err.Error())
77 81
 		}
78 82
 
79
-		if err == nil {
80
-			rc.Close()
81
-		}
83
+		rc.Close()
82 84
 
83
-		// We only interested in the io.ErrUnexpectedEOF error, so we return nil otherwise.
84 85
 		return nil
85 86
 	}
86 87