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
hashicorp · May 30, 2019 · d00ed5c · d00ed5c
1 parent 86a6569
commit d00ed5c
Show file tree

Hide file tree

Showing 4 changed files with 147 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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