From e6783656f917c5a8b8c6f346b4ff840d97b1b6ce Mon Sep 17 00:00:00 2001 From: David Wang <00107082@163.com> Date: Fri, 8 Jun 2018 11:07:48 +0800 Subject: [PATCH] Fix race condition between exec start and resize Signed-off-by: David Wang <00107082@163.com> --- daemon/exec.go | 5 +- daemon/exec/exec.go | 2 + daemon/resize.go | 11 +- daemon/resize_test.go | 103 ++++++++++++++++++ daemon/util_test.go | 65 +++++++++++ .../docker_api_exec_resize_test.go | 15 +-- 6 files changed, 192 insertions(+), 9 deletions(-) create mode 100644 daemon/resize_test.go create mode 100644 daemon/util_test.go diff --git a/daemon/exec.go b/daemon/exec.go index 289c6bfb30..f0b43d7253 100644 --- a/daemon/exec.go +++ b/daemon/exec.go @@ -16,7 +16,7 @@ import ( "github.com/docker/docker/pkg/pools" "github.com/docker/docker/pkg/signal" "github.com/docker/docker/pkg/term" - "github.com/opencontainers/runtime-spec/specs-go" + specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -249,6 +249,9 @@ func (d *Daemon) ContainerExecStart(ctx context.Context, name string, stdin io.R ec.Lock() c.ExecCommands.Lock() systemPid, err := d.containerd.Exec(ctx, c.ID, ec.ID, p, cStdin != nil, ec.InitializeStdio) + // the exec context should be ready, or error happened. + // close the chan to notify readiness + close(ec.Started) if err != nil { c.ExecCommands.Unlock() ec.Unlock() diff --git a/daemon/exec/exec.go b/daemon/exec/exec.go index ca4e1aeb49..c036c46a0c 100644 --- a/daemon/exec/exec.go +++ b/daemon/exec/exec.go @@ -15,6 +15,7 @@ import ( // examined both during and after completion. type Config struct { sync.Mutex + Started chan struct{} StreamConfig *stream.Config ID string Running bool @@ -40,6 +41,7 @@ func NewConfig() *Config { return &Config{ ID: stringid.GenerateNonCryptoID(), StreamConfig: stream.NewConfig(), + Started: make(chan struct{}), } } diff --git a/daemon/resize.go b/daemon/resize.go index 369b8fdb3d..21240650f8 100644 --- a/daemon/resize.go +++ b/daemon/resize.go @@ -3,6 +3,7 @@ package daemon // import "github.com/docker/docker/daemon" import ( "context" "fmt" + "time" "github.com/docker/docker/libcontainerd" ) @@ -37,5 +38,13 @@ func (daemon *Daemon) ContainerExecResize(name string, height, width int) error if err != nil { return err } - return daemon.containerd.ResizeTerminal(context.Background(), ec.ContainerID, ec.ID, width, height) + // TODO: the timeout is hardcoded here, it would be more flexible to make it + // a parameter in resize request context, which would need API changes. + timeout := 10 * time.Second + select { + case <-ec.Started: + return daemon.containerd.ResizeTerminal(context.Background(), ec.ContainerID, ec.ID, width, height) + case <-time.After(timeout): + return fmt.Errorf("timeout waiting for exec session ready") + } } diff --git a/daemon/resize_test.go b/daemon/resize_test.go new file mode 100644 index 0000000000..dea26c6116 --- /dev/null +++ b/daemon/resize_test.go @@ -0,0 +1,103 @@ +// +build linux + +package daemon + +import ( + "context" + "testing" + + "github.com/docker/docker/container" + "github.com/docker/docker/daemon/exec" + "github.com/gotestyourself/gotestyourself/assert" +) + +// This test simply verify that when a wrong ID used, a specific error should be returned for exec resize. +func TestExecResizeNoSuchExec(t *testing.T) { + n := "TestExecResize" + d := &Daemon{ + execCommands: exec.NewStore(), + } + c := &container.Container{ + ExecCommands: exec.NewStore(), + } + ec := &exec.Config{ + ID: n, + } + d.registerExecCommand(c, ec) + err := d.ContainerExecResize("nil", 24, 8) + assert.ErrorContains(t, err, "No such exec instance") +} + +type execResizeMockContainerdClient struct { + MockContainerdClient + ProcessID string + ContainerID string + Width int + Height int +} + +func (c *execResizeMockContainerdClient) ResizeTerminal(ctx context.Context, containerID, processID string, width, height int) error { + c.ProcessID = processID + c.ContainerID = containerID + c.Width = width + c.Height = height + return nil +} + +// This test is to make sure that when exec context is ready, resize should call ResizeTerminal via containerd client. +func TestExecResize(t *testing.T) { + n := "TestExecResize" + width := 24 + height := 8 + ec := &exec.Config{ + ID: n, + ContainerID: n, + Started: make(chan struct{}), + } + close(ec.Started) + mc := &execResizeMockContainerdClient{} + d := &Daemon{ + execCommands: exec.NewStore(), + containerd: mc, + containers: container.NewMemoryStore(), + } + c := &container.Container{ + ExecCommands: exec.NewStore(), + State: &container.State{Running: true}, + } + d.containers.Add(n, c) + d.registerExecCommand(c, ec) + err := d.ContainerExecResize(n, height, width) + assert.NilError(t, err) + assert.Equal(t, mc.Width, width) + assert.Equal(t, mc.Height, height) + assert.Equal(t, mc.ProcessID, n) + assert.Equal(t, mc.ContainerID, n) +} + +// This test is to make sure that when exec context is not ready, a timeout error should happen. +// TODO: the expect running time for this test is 10s, which would be too long for unit test. +func TestExecResizeTimeout(t *testing.T) { + n := "TestExecResize" + width := 24 + height := 8 + ec := &exec.Config{ + ID: n, + ContainerID: n, + Started: make(chan struct{}), + } + mc := &execResizeMockContainerdClient{} + d := &Daemon{ + execCommands: exec.NewStore(), + containerd: mc, + containers: container.NewMemoryStore(), + } + c := &container.Container{ + ExecCommands: exec.NewStore(), + State: &container.State{Running: true}, + } + d.containers.Add(n, c) + d.registerExecCommand(c, ec) + err := d.ContainerExecResize(n, height, width) + assert.ErrorContains(t, err, "timeout waiting for exec session ready") +} diff --git a/daemon/util_test.go b/daemon/util_test.go new file mode 100644 index 0000000000..b2c464f737 --- /dev/null +++ b/daemon/util_test.go @@ -0,0 +1,65 @@ +// +build linux + +package daemon + +import ( + "context" + "time" + + "github.com/containerd/containerd" + "github.com/docker/docker/libcontainerd" + specs "github.com/opencontainers/runtime-spec/specs-go" +) + +// Mock containerd client implementation, for unit tests. +type MockContainerdClient struct { +} + +func (c *MockContainerdClient) Version(ctx context.Context) (containerd.Version, error) { + return containerd.Version{}, nil +} +func (c *MockContainerdClient) Restore(ctx context.Context, containerID string, attachStdio libcontainerd.StdioCallback) (alive bool, pid int, err error) { + return false, 0, nil +} +func (c *MockContainerdClient) Create(ctx context.Context, containerID string, spec *specs.Spec, runtimeOptions interface{}) error { + return nil +} +func (c *MockContainerdClient) Start(ctx context.Context, containerID, checkpointDir string, withStdin bool, attachStdio libcontainerd.StdioCallback) (pid int, err error) { + return 0, nil +} +func (c *MockContainerdClient) SignalProcess(ctx context.Context, containerID, processID string, signal int) error { + return nil +} +func (c *MockContainerdClient) Exec(ctx context.Context, containerID, processID string, spec *specs.Process, withStdin bool, attachStdio libcontainerd.StdioCallback) (int, error) { + return 0, nil +} +func (c *MockContainerdClient) ResizeTerminal(ctx context.Context, containerID, processID string, width, height int) error { + return nil +} +func (c *MockContainerdClient) CloseStdin(ctx context.Context, containerID, processID string) error { + return nil +} +func (c *MockContainerdClient) Pause(ctx context.Context, containerID string) error { return nil } +func (c *MockContainerdClient) Resume(ctx context.Context, containerID string) error { return nil } +func (c *MockContainerdClient) Stats(ctx context.Context, containerID string) (*libcontainerd.Stats, error) { + return nil, nil +} +func (c *MockContainerdClient) ListPids(ctx context.Context, containerID string) ([]uint32, error) { + return nil, nil +} +func (c *MockContainerdClient) Summary(ctx context.Context, containerID string) ([]libcontainerd.Summary, error) { + return nil, nil +} +func (c *MockContainerdClient) DeleteTask(ctx context.Context, containerID string) (uint32, time.Time, error) { + return 0, time.Time{}, nil +} +func (c *MockContainerdClient) Delete(ctx context.Context, containerID string) error { return nil } +func (c *MockContainerdClient) Status(ctx context.Context, containerID string) (libcontainerd.Status, error) { + return "null", nil +} +func (c *MockContainerdClient) UpdateResources(ctx context.Context, containerID string, resources *libcontainerd.Resources) error { + return nil +} +func (c *MockContainerdClient) CreateCheckpoint(ctx context.Context, containerID, checkpointDir string, exit bool) error { + return nil +} diff --git a/integration-cli/docker_api_exec_resize_test.go b/integration-cli/docker_api_exec_resize_test.go index c561d60f56..2db3d3e317 100644 --- a/integration-cli/docker_api_exec_resize_test.go +++ b/integration-cli/docker_api_exec_resize_test.go @@ -71,16 +71,17 @@ func (s *DockerSuite) TestExecResizeImmediatelyAfterExecStart(c *check.C) { defer conn.Close() _, rc, err := request.Post(fmt.Sprintf("/exec/%s/resize?h=24&w=80", execID), request.ContentType("text/plain")) - // It's probably a panic of the daemon if io.ErrUnexpectedEOF is returned. - if err == io.ErrUnexpectedEOF { - return fmt.Errorf("The daemon might have crashed.") + if err != nil { + // It's probably a panic of the daemon if io.ErrUnexpectedEOF is returned. + if err == io.ErrUnexpectedEOF { + return fmt.Errorf("The daemon might have crashed.") + } + // Other error happened, should be reported. + return fmt.Errorf("Fail to exec resize immediately after start. Error: %q", err.Error()) } - if err == nil { - rc.Close() - } + rc.Close() - // We only interested in the io.ErrUnexpectedEOF error, so we return nil otherwise. return nil }