Removing an image that fails, also removes the image name/tag.

Fixes #7845 and #7801, and a real pain point I had :)

Docker-DCO-1.1-Signed-off-by: Jessica Frazelle <jess@docker.com> (github: jfrazelle)

daemon/image_delete.go

@@ -33,7 +33,6 @@ func (daemon *Daemon) DeleteImage(eng *engine.Engine, name string, imgs *engine.
 	var (
 		repoName, tag string
 		tags          = []string{}
-		tagDeleted    bool
 	)
 
 	// FIXME: please respect DRY and centralize repo+tag parsing in a single central place! -- shykes
@@ -60,9 +59,11 @@ func (daemon *Daemon) DeleteImage(eng *engine.Engine, name string, imgs *engine.
 		return err
 	}
 
+	repos := daemon.Repositories().ByID()[img.ID]
+
 	//If delete by id, see if the id belong only to one repository
 	if repoName == "" {
-		for _, repoAndTag := range daemon.Repositories().ByID()[img.ID] {
+		for _, repoAndTag := range repos {
 			parsedRepo, parsedTag := parsers.ParseRepositoryTag(repoAndTag)
 			if repoName == "" || repoName == parsedRepo {
 				repoName = parsedRepo
@@ -83,9 +84,15 @@ func (daemon *Daemon) DeleteImage(eng *engine.Engine, name string, imgs *engine.
 		return nil
 	}
 
-	//Untag the current image
+	if len(repos) <= 1 {
+		if err := daemon.canDeleteImage(img.ID, force); err != nil {
+			return err
+		}
+	}
+
+	// Untag the current image
 	for _, tag := range tags {
-		tagDeleted, err = daemon.Repositories().Delete(repoName, tag)
+		tagDeleted, err := daemon.Repositories().Delete(repoName, tag)
 		if err != nil {
 			return err
 		}
@@ -99,9 +106,6 @@ func (daemon *Daemon) DeleteImage(eng *engine.Engine, name string, imgs *engine.
 	tags = daemon.Repositories().ByID()[img.ID]
 	if (len(tags) <= 1 && repoName == "") || len(tags) == 0 {
 		if len(byParents[img.ID]) == 0 {
-			if err := daemon.canDeleteImage(img.ID, force, tagDeleted); err != nil {
-				return err
-			}
 			if err := daemon.Repositories().DeleteAll(img.ID); err != nil {
 				return err
 			}
@@ -125,11 +129,7 @@ func (daemon *Daemon) DeleteImage(eng *engine.Engine, name string, imgs *engine.
 	return nil
 }
 
-func (daemon *Daemon) canDeleteImage(imgID string, force, untagged bool) error {
-	var message string
-	if untagged {
-		message = " (docker untagged the image)"
-	}
+func (daemon *Daemon) canDeleteImage(imgID string, force bool) error {
 	for _, container := range daemon.List() {
 		parent, err := daemon.Repositories().LookupImage(container.Image)
 		if err != nil {
@@ -140,11 +140,11 @@ func (daemon *Daemon) canDeleteImage(imgID string, force, untagged bool) error {
 			if imgID == p.ID {
 				if container.IsRunning() {
 					if force {
-						return fmt.Errorf("Conflict, cannot force delete %s because the running container %s is using it%s, stop it and retry", utils.TruncateID(imgID), utils.TruncateID(container.ID), message)
+						return fmt.Errorf("Conflict, cannot force delete %s because the running container %s is using it, stop it and retry", utils.TruncateID(imgID), utils.TruncateID(container.ID))
 					}
-					return fmt.Errorf("Conflict, cannot delete %s because the running container %s is using it%s, stop it and use -f to force", utils.TruncateID(imgID), utils.TruncateID(container.ID), message)
+					return fmt.Errorf("Conflict, cannot delete %s because the running container %s is using it, stop it and use -f to force", utils.TruncateID(imgID), utils.TruncateID(container.ID))
 				} else if !force {
-					return fmt.Errorf("Conflict, cannot delete %s because the container %s is using it%s, use -f to force", utils.TruncateID(imgID), utils.TruncateID(container.ID), message)
+					return fmt.Errorf("Conflict, cannot delete %s because the container %s is using it, use -f to force", utils.TruncateID(imgID), utils.TruncateID(container.ID))
 				}
 			}
 			return nil

integration-cli/docker_cli_images_test.go

@@ -20,44 +20,6 @@ func TestImagesEnsureImageIsListed(t *testing.T) {
 	logDone("images - busybox should be listed")
 }
 
-func TestCLIImageTagRemove(t *testing.T) {
-	imagesBefore, _, _ := cmd(t, "images", "-a")
-	cmd(t, "tag", "busybox", "utest:tag1")
-	cmd(t, "tag", "busybox", "utest/docker:tag2")
-	cmd(t, "tag", "busybox", "utest:5000/docker:tag3")
-	{
-		imagesAfter, _, _ := cmd(t, "images", "-a")
-		if nLines(imagesAfter) != nLines(imagesBefore)+3 {
-			t.Fatalf("before: %q\n\nafter: %q\n", imagesBefore, imagesAfter)
-		}
-	}
-	cmd(t, "rmi", "utest/docker:tag2")
-	{
-		imagesAfter, _, _ := cmd(t, "images", "-a")
-		if nLines(imagesAfter) != nLines(imagesBefore)+2 {
-			t.Fatalf("before: %q\n\nafter: %q\n", imagesBefore, imagesAfter)
-		}
-
-	}
-	cmd(t, "rmi", "utest:5000/docker:tag3")
-	{
-		imagesAfter, _, _ := cmd(t, "images", "-a")
-		if nLines(imagesAfter) != nLines(imagesBefore)+1 {
-			t.Fatalf("before: %q\n\nafter: %q\n", imagesBefore, imagesAfter)
-		}
-
-	}
-	cmd(t, "rmi", "utest:tag1")
-	{
-		imagesAfter, _, _ := cmd(t, "images", "-a")
-		if nLines(imagesAfter) != nLines(imagesBefore)+0 {
-			t.Fatalf("before: %q\n\nafter: %q\n", imagesBefore, imagesAfter)
-		}
-
-	}
-	logDone("tag,rmi- tagging the same images multiple times then removing tags")
-}
-
 func TestImagesOrderedByCreationDate(t *testing.T) {
 	defer deleteImages("order:test_a")
 	defer deleteImages("order:test_c")

integration-cli/docker_cli_rmi_test.go

@@ -0,0 +1,77 @@
+package main
+
+import (
+	"fmt"
+	"os/exec"
+	"strings"
+	"testing"
+)
+
+func TestImageRemoveWithContainerFails(t *testing.T) {
+	errSubstr := "is using it"
+
+	// create a container
+	runCmd := exec.Command(dockerBinary, "run", "-d", "busybox", "true")
+	out, _, err := runCommandWithOutput(runCmd)
+	errorOut(err, t, fmt.Sprintf("failed to create a container: %v %v", out, err))
+
+	cleanedContainerID := stripTrailingCharacters(out)
+
+	// try to delete the image
+	runCmd = exec.Command(dockerBinary, "rmi", "busybox")
+	out, _, err = runCommandWithOutput(runCmd)
+	if err == nil {
+		t.Fatalf("Container %q is using image, should not be able to rmi: %q", cleanedContainerID, out)
+	}
+	if !strings.Contains(out, errSubstr) {
+		t.Fatalf("Container %q is using image, error message should contain %q: %v", cleanedContainerID, errSubstr, out)
+	}
+
+	// make sure it didn't delete the busybox name
+	images, _, _ := cmd(t, "images")
+	if !strings.Contains(images, "busybox") {
+		t.Fatalf("The name 'busybox' should not have been removed from images: %q", images)
+	}
+
+	deleteContainer(cleanedContainerID)
+
+	logDone("rmi- container using image while rmi, should not remove image name")
+}
+
+func TestImageTagRemove(t *testing.T) {
+	imagesBefore, _, _ := cmd(t, "images", "-a")
+	cmd(t, "tag", "busybox", "utest:tag1")
+	cmd(t, "tag", "busybox", "utest/docker:tag2")
+	cmd(t, "tag", "busybox", "utest:5000/docker:tag3")
+	{
+		imagesAfter, _, _ := cmd(t, "images", "-a")
+		if nLines(imagesAfter) != nLines(imagesBefore)+3 {
+			t.Fatalf("before: %q\n\nafter: %q\n", imagesBefore, imagesAfter)
+		}
+	}
+	cmd(t, "rmi", "utest/docker:tag2")
+	{
+		imagesAfter, _, _ := cmd(t, "images", "-a")
+		if nLines(imagesAfter) != nLines(imagesBefore)+2 {
+			t.Fatalf("before: %q\n\nafter: %q\n", imagesBefore, imagesAfter)
+		}
+
+	}
+	cmd(t, "rmi", "utest:5000/docker:tag3")
+	{
+		imagesAfter, _, _ := cmd(t, "images", "-a")
+		if nLines(imagesAfter) != nLines(imagesBefore)+1 {
+			t.Fatalf("before: %q\n\nafter: %q\n", imagesBefore, imagesAfter)
+		}
+
+	}
+	cmd(t, "rmi", "utest:tag1")
+	{
+		imagesAfter, _, _ := cmd(t, "images", "-a")
+		if nLines(imagesAfter) != nLines(imagesBefore)+0 {
+			t.Fatalf("before: %q\n\nafter: %q\n", imagesBefore, imagesAfter)
+		}
+
+	}
+	logDone("tag,rmi- tagging the same images multiple times then removing tags")
+}