Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix rmi bug when checking containers with deleted image #1998

Merged
merged 1 commit into from
Feb 18, 2023
+100 −22
Merged

Fix rmi bug when checking containers with deleted image #1998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions cmd/nerdctl/image_encrypt_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
djdongjin marked this conversation as resolved.
Show resolved Hide resolved

base.T.Logf("Pruning build caches")
if _, err := buildkitutil.GetBuildkitHost(testutil.Namespace); err == nil {
Expand Down Expand Up @@ -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()
}
}

Expand Down
58 changes: 56 additions & 2 deletions cmd/nerdctl/image_remove_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
djdongjin marked this conversation as resolved.
Show resolved Hide resolved
base.Cmd("images").AssertNoOut(testutil.ImageRepo(testutil.NginxAlpineImage))
}
56 changes: 38 additions & 18 deletions pkg/cmd/image/remove.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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()
}
}

Expand All @@ -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())
Expand All @@ -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)
Comment on lines +95 to +115
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to exactly match docker's error handling behavior (for each err, if not IsErrNotFound, then it's fatal; if there is any fatal error, return a joined error, else log error and return nil).

If using WalkAll, an edge case will be inconsistent with docker: if all errors are IsErrNotFound (e.g. docker|nerdctl rmi -f afsd afdafaf aff), docker return nil and nerdctl return a joined err. TBH I don't feel it makes much sense to distinguish the specific edge case in docker...

}
return err
return nil
}