From d00ed5c65cb886a927cbe4ac76140625016012c2 Mon Sep 17 00:00:00 2001 From: Chris Baker Date: Wed, 29 May 2019 22:38:43 +0000 Subject: [PATCH] docker: DestroyTask was not cleaning up Docker images because it was erroring early due to an attempt to inspect an image that had already been removed --- drivers/docker/driver.go | 18 +++- drivers/docker/driver_test.go | 132 +++++++++++++++++++++++++++ drivers/docker/handle.go | 9 -- plugins/drivers/testutils/testing.go | 2 +- 4 files changed, 147 insertions(+), 14 deletions(-) diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index 3754d032b7ed..7b2fa73a48e2 100644 --- a/drivers/docker/driver.go +++ b/drivers/docker/driver.go @@ -1109,12 +1109,22 @@ func (d *Driver) DestroyTask(taskID string, force bool) error { if err != nil { return fmt.Errorf("failed to inspect container state: %v", err) } - if c.State.Running && !force { - return fmt.Errorf("must call StopTask for the given task before Destroy or set force to true") + + if c.State.Running { + if !force { + return fmt.Errorf("must call StopTask for the given task before Destroy or set force to true") + } + if err := h.client.StopContainer(h.containerID, 0); err != nil { + h.logger.Warn("failed to stop container during destroy", "error", err) + } } - if err := h.client.StopContainer(h.containerID, 0); err != nil { - h.logger.Warn("failed to stop container during destroy", "error", err) + if h.removeContainerOnExit { + if err := h.client.RemoveContainer(docker.RemoveContainerOptions{ID: h.containerID, RemoveVolumes: true, Force: true}); err != nil { + h.logger.Error("error removing container", "error", err) + } + } else { + h.logger.Debug("not removing container due to config") } if err := d.cleanupImage(h); err != nil { diff --git a/drivers/docker/driver_test.go b/drivers/docker/driver_test.go index 70cd9a5e4d6a..ca2a453e18be 100644 --- a/drivers/docker/driver_test.go +++ b/drivers/docker/driver_test.go @@ -1386,12 +1386,16 @@ func TestDockerDriver_CleanupContainer(t *testing.T) { waitCh, err := d.WaitTask(context.Background(), task.ID) require.NoError(t, err) + select { case res := <-waitCh: if !res.Successful() { t.Fatalf("err: %v", res) } + err = d.DestroyTask(task.ID, false) + require.NoError(t, err) + time.Sleep(3 * time.Second) // Ensure that the container isn't present @@ -1405,6 +1409,134 @@ func TestDockerDriver_CleanupContainer(t *testing.T) { } } +func TestDockerDriver_EnableImageGC(t *testing.T) { + testutil.DockerCompatible(t) + + task, cfg, _ := dockerTask(t) + cfg.Command = "echo" + cfg.Args = []string{"hello"} + require.NoError(t, task.EncodeConcreteDriverConfig(cfg)) + + client := newTestDockerClient(t) + driver := dockerDriverHarness(t, map[string]interface{}{ + "gc": map[string]interface{}{ + "container": true, + "image": true, + "image_delay": "2s", + }, + }) + cleanup := driver.MkAllocDir(task, true) + defer cleanup() + + // remove the image before the test + client.RemoveImage(cfg.Image) + + copyImage(t, task.TaskDir(), "busybox.tar") + _, _, err := driver.StartTask(task) + require.NoError(t, err) + + dockerDriver, ok := driver.Impl().(*Driver) + require.True(t, ok) + _, ok = dockerDriver.tasks.Get(task.ID) + require.True(t, ok) + + waitCh, err := dockerDriver.WaitTask(context.Background(), task.ID) + require.NoError(t, err) + select { + case res := <-waitCh: + if !res.Successful() { + t.Fatalf("err: %v", res) + } + + case <-time.After(time.Duration(tu.TestMultiplier()*5) * time.Second): + t.Fatalf("timeout") + } + + // we haven't called DestroyTask, image should be present + _, err = client.InspectImage(cfg.Image) + require.NoError(t, err) + + err = dockerDriver.DestroyTask(task.ID, false) + require.NoError(t, err) + + // image_delay is 3s, so image should still be around for a bit + _, err = client.InspectImage(cfg.Image) + require.NoError(t, err) + + // Ensure image was removed + tu.WaitForResult(func() (bool, error) { + if _, err := client.InspectImage(cfg.Image); err == nil { + return false, fmt.Errorf("image exists but should have been removed. Does another %v container exist?", cfg.Image) + } + + return true, nil + }, func(err error) { + require.NoError(t, err) + }) +} + +func TestDockerDriver_DisableImageGC(t *testing.T) { + testutil.DockerCompatible(t) + + task, cfg, _ := dockerTask(t) + cfg.Command = "echo" + cfg.Args = []string{"hello"} + require.NoError(t, task.EncodeConcreteDriverConfig(cfg)) + + client := newTestDockerClient(t) + driver := dockerDriverHarness(t, map[string]interface{}{ + "gc": map[string]interface{}{ + "container": true, + "image": false, + "image_delay": "1s", + }, + }) + cleanup := driver.MkAllocDir(task, true) + defer cleanup() + + // remove the image before the test + client.RemoveImage(cfg.Image) + + copyImage(t, task.TaskDir(), "busybox.tar") + _, _, err := driver.StartTask(task) + require.NoError(t, err) + + dockerDriver, ok := driver.Impl().(*Driver) + require.True(t, ok) + handle, ok := dockerDriver.tasks.Get(task.ID) + require.True(t, ok) + + waitCh, err := dockerDriver.WaitTask(context.Background(), task.ID) + require.NoError(t, err) + select { + case res := <-waitCh: + if !res.Successful() { + t.Fatalf("err: %v", res) + } + + case <-time.After(time.Duration(tu.TestMultiplier()*5) * time.Second): + t.Fatalf("timeout") + } + + // we haven't called DestroyTask, image should be present + _, err = client.InspectImage(handle.containerImage) + require.NoError(t, err) + + err = dockerDriver.DestroyTask(task.ID, false) + require.NoError(t, err) + + // image_delay is 1s, wait a little longer + time.Sleep(3 * time.Second) + + // image should not have been removed or scheduled to be removed + _, err = client.InspectImage(cfg.Image) + require.NoError(t, err) + dockerDriver.coordinator.imageLock.Lock() + _, ok = dockerDriver.coordinator.deleteFuture[handle.containerImage] + require.False(t, ok, "image should not be registered for deletion") + dockerDriver.coordinator.imageLock.Unlock() +} + func TestDockerDriver_Stats(t *testing.T) { if !tu.IsCI() { t.Parallel() diff --git a/drivers/docker/handle.go b/drivers/docker/handle.go index 1c52591334ad..398b53d74b89 100644 --- a/drivers/docker/handle.go +++ b/drivers/docker/handle.go @@ -212,15 +212,6 @@ func (h *taskHandle) run() { } } - // Remove the container - if h.removeContainerOnExit == true { - if err := h.client.RemoveContainer(docker.RemoveContainerOptions{ID: h.containerID, RemoveVolumes: true, Force: true}); err != nil { - h.logger.Error("error removing container", "error", err) - } - } else { - h.logger.Debug("not removing container due to config") - } - // Set the result h.exitResultLock.Lock() h.exitResult = &drivers.ExitResult{ diff --git a/plugins/drivers/testutils/testing.go b/plugins/drivers/testutils/testing.go index f28c3d4fa9ef..5a941f97be46 100644 --- a/plugins/drivers/testutils/testing.go +++ b/plugins/drivers/testutils/testing.go @@ -75,7 +75,7 @@ func (h *DriverHarness) Kill() { h.server.Stop() } -// MkAllocDir creates a tempory directory and allocdir structure. +// MkAllocDir creates a temporary directory and allocdir structure. // If enableLogs is set to true a logmon instance will be started to write logs // to the LogDir of the task // A cleanup func is returned and should be defered so as to not leak dirs