Fix rmi err checking containers with deleted image

Signed-off-by: Jin Dong <[email protected]>

cmd/nerdctl/image_encrypt_linux_test.go

@@ -68,7 +68,11 @@ func newJWEKeyPair(t testing.TB) *jweKeyPair {
 func rmiAll(base *testutil.Base) {
 	base.T.Logf("Pruning images")
 	imageIDs := base.Cmd("images", "--no-trunc", "-a", "-q").OutLines()
-	base.Cmd(append([]string{"rmi", "-f"}, imageIDs...)...).AssertOK()
+	// remove empty output line at the end
+	imageIDs = imageIDs[:len(imageIDs)-1]
+	// use `Run` on purpose (same below) because `rmi all` may fail on individual
+	// image id that has an expected running container (e.g. a registry)
+	base.Cmd(append([]string{"rmi", "-f"}, imageIDs...)...).Run()
 
 	base.T.Logf("Pruning build caches")
 	if _, err := buildkitutil.GetBuildkitHost(testutil.Namespace); err == nil {
@@ -97,7 +101,7 @@ func rmiAll(base *testutil.Base) {
 
 		base.T.Logf("Pruning all images (again?)")
 		imageIDs = base.Cmd("images", "--no-trunc", "-a", "-q").OutLines()
-		base.Cmd(append([]string{"rmi", "-f"}, imageIDs...)...).AssertOK()
+		base.Cmd(append([]string{"rmi", "-f"}, imageIDs...)...).Run()
 	}
 }
 

cmd/nerdctl/image_remove_linux_test.go

@@ -31,7 +31,7 @@ func TestRemoveImage(t *testing.T) {
 	base.Cmd("rmi", "-f", tID).AssertOK()
 
 	base.Cmd("run", "--name", tID, testutil.CommonImage).AssertOK()
-	defer base.Cmd("rm", "-f", tID).Run()
+	defer base.Cmd("rm", "-f", tID).AssertOK()
 
 	base.Cmd("rmi", testutil.CommonImage).AssertFail()
 	defer base.Cmd("rmi", "-f", testutil.CommonImage).Run()
@@ -41,13 +41,67 @@ func TestRemoveImage(t *testing.T) {
 }
 
 func TestRemoveRunningImage(t *testing.T) {
+	// If an image is associated with a running/paused containers, `docker rmi -f imageName`
+	// untags `imageName` (left a `<none>` image) without deletion; `docker rmi -rf imageID` fails.
+	// In both cases, `nerdctl rmi -f` will fail.
+	testutil.DockerIncompatible(t)
 	base := testutil.NewBase(t)
 	tID := testutil.Identifier(t)
+
+	base.Cmd("run", "--name", tID, "-d", testutil.CommonImage, "sleep", "infinity").AssertOK()
+	defer base.Cmd("rm", "-f", tID).AssertOK()
+
+	base.Cmd("rmi", testutil.CommonImage).AssertFail()
+	base.Cmd("rmi", "-f", testutil.CommonImage).AssertFail()
+	base.Cmd("images").AssertOutContains(testutil.ImageRepo(testutil.CommonImage))
+
+	base.Cmd("kill", tID).AssertOK()
+	base.Cmd("rmi", testutil.CommonImage).AssertFail()
+	base.Cmd("rmi", "-f", testutil.CommonImage).AssertOK()
+	base.Cmd("images").AssertNoOut(testutil.ImageRepo(testutil.CommonImage))
+}
+
+func TestRemovePausedImage(t *testing.T) {
+	// If an image is associated with a running/paused containers, `docker rmi -f imageName`
+	// untags `imageName` (left a `<none>` image) without deletion; `docker rmi -rf imageID` fails.
+	// In both cases, `nerdctl rmi -f` will fail.
+	testutil.DockerIncompatible(t)
+	base := testutil.NewBase(t)
+	switch base.Info().CgroupDriver {
+	case "none", "":
+		t.Skip("requires cgroup (for pausing)")
+	}
+	tID := testutil.Identifier(t)
+
 	base.Cmd("run", "--name", tID, "-d", testutil.CommonImage, "sleep", "infinity").AssertOK()
-	defer base.Cmd("rm", "-f", tID).Run()
+	base.Cmd("pause", tID).AssertOK()
+	defer base.Cmd("rm", "-f", tID).AssertOK()
+
 	base.Cmd("rmi", testutil.CommonImage).AssertFail()
+	base.Cmd("rmi", "-f", testutil.CommonImage).AssertFail()
+	base.Cmd("images").AssertOutContains(testutil.ImageRepo(testutil.CommonImage))
+
 	base.Cmd("kill", tID).AssertOK()
 	base.Cmd("rmi", testutil.CommonImage).AssertFail()
 	base.Cmd("rmi", "-f", testutil.CommonImage).AssertOK()
 	base.Cmd("images").AssertNoOut(testutil.ImageRepo(testutil.CommonImage))
 }
+
+func TestRemoveImageWithCreatedContainer(t *testing.T) {
+	base := testutil.NewBase(t)
+	tID := testutil.Identifier(t)
+
+	base.Cmd("pull", testutil.AlpineImage).AssertOK()
+	base.Cmd("pull", testutil.NginxAlpineImage).AssertOK()
+
+	base.Cmd("create", "--name", tID, testutil.AlpineImage, "sleep", "infinity").AssertOK()
+	defer base.Cmd("rm", "-f", tID).AssertOK()
+
+	base.Cmd("rmi", testutil.AlpineImage).AssertFail()
+	base.Cmd("rmi", "-f", testutil.AlpineImage).AssertOK()
+	base.Cmd("images").AssertNoOut(testutil.ImageRepo(testutil.AlpineImage))
+
+	// a created container with removed image doesn't impact other `rmi` command
+	base.Cmd("rmi", "-f", testutil.NginxAlpineImage).AssertOK()
+	base.Cmd("images").AssertNoOut(testutil.ImageRepo(testutil.NginxAlpineImage))
+}

pkg/cmd/image/remove.go

@@ -18,14 +18,15 @@ package image
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/images"
 	"github.com/containerd/containerd/platforms"
 	"github.com/containerd/nerdctl/pkg/api/types"
-	"github.com/containerd/nerdctl/pkg/formatter"
+	"github.com/containerd/nerdctl/pkg/containerutil"
 	"github.com/containerd/nerdctl/pkg/idutil/imagewalker"
 	"github.com/sirupsen/logrus"
 )
@@ -43,18 +44,20 @@ func Remove(ctx context.Context, client *containerd.Client, args []string, optio
 	if err != nil {
 		return err
 	}
-	usedImages := make(map[string]struct{})
-	runningImages := make(map[string]struct{})
+	usedImages := make(map[string]string)
+	runningImages := make(map[string]string)
 	for _, container := range containerList {
 		image, err := container.Image(ctx)
 		if err != nil {
-			return err
+			continue
 		}
-		cStatus := formatter.ContainerStatus(ctx, container)
-		if strings.HasPrefix(cStatus, "Up") {
-			runningImages[image.Name()] = struct{}{}
-		} else {
-			usedImages[image.Name()] = struct{}{}
+
+		// if err != nil, simply go to `default`
+		switch cStatus, _ := containerutil.ContainerStatus(ctx, container); cStatus.Status {
+		case containerd.Running, containerd.Pausing, containerd.Paused:
+			runningImages[image.Name()] = container.ID()
+		default:
+			usedImages[image.Name()] = container.ID()
 		}
 	}
 
@@ -66,11 +69,11 @@ func Remove(ctx context.Context, client *containerd.Client, args []string, optio
 			if found.MatchCount > 1 && !(options.Force && found.UniqueImages == 1) {
 				return fmt.Errorf("multiple IDs found with provided prefix: %s", found.Req)
 			}
-			if _, ok := runningImages[found.Image.Name]; ok {
-				return fmt.Errorf("image %s is running, can't be forced removed", found.Image.Name)
+			if cid, ok := runningImages[found.Image.Name]; ok {
+				return fmt.Errorf("conflict: unable to delete %s (cannot be forced) - image is being used by running container %s", found.Req, cid)
 			}
-			if _, ok := usedImages[found.Image.Name]; ok && !options.Force {
-				return fmt.Errorf("conflict: unable to remove repository reference %q (must force)", found.Req)
+			if cid, ok := usedImages[found.Image.Name]; ok && !options.Force {
+				return fmt.Errorf("conflict: unable to delete %s (must be forced) - image is being used by stopped container %s", found.Req, cid)
 			}
 			// digests is used only for emulating human-readable output of `docker rmi`
 			digests, err := found.Image.RootFS(ctx, cs, platforms.DefaultStrict())
@@ -89,10 +92,27 @@ func Remove(ctx context.Context, client *containerd.Client, args []string, optio
 		},
 	}
 
-	err = walker.WalkAll(ctx, args, true)
-	if err != nil && options.Force {
-		logrus.Error(err)
-		return nil
+	var errs []string
+	var fatalErr bool
+	for _, req := range args {
+		n, err := walker.Walk(ctx, req)
+		if err != nil {
+			fatalErr = true
+		}
+		if err == nil && n == 0 {
+			err = fmt.Errorf("no such image: %s", req)
+		}
+		if err != nil {
+			errs = append(errs, err.Error())
+		}
+	}
+
+	if len(errs) > 0 {
+		msg := fmt.Sprintf("%d errors:\n%s", len(errs), strings.Join(errs, "\n"))
+		if !options.Force || fatalErr {
+			return errors.New(msg)
+		}
+		logrus.Error(msg)
 	}
-	return err
+	return nil
 }