containerd: Image delete fixes and cleanup by dmcgowan · Pull Request #46840 · moby/moby

dmcgowan · 2023-11-22T02:46:22Z

addresses various tests in [epic] fix remaining failing tests with the containerd image store #46760

Ensure that when removing an image, an image is checked consistently against the images with the same target digest.
Update logic of determining which references are the same to fix failing tests.
Add unit testing around delete.

daemon/containerd/image_delete.go

vvoland

Just posting a couple of comments/nits.

daemon/containerd/image_delete.go

daemon/containerd/image_test.go

daemon/containerd/image_delete.go

daemon/containerd/image_delete_test.go

daemon/containerd/soft_delete.go

vvoland

Looks good overall. Just a few comments/questions from me. Don't think any of those is a blocker though.

daemon/containerd/image_test.go

daemon/containerd/image_delete.go

vvoland · 2023-12-01T13:53:02Z

daemon/containerd/image.go

+	if truncatedID.MatchString(refOrID) {
+		if d, ok := parsed.(reference.Digested); ok {
+			if cimg, err := i.images.Get(ctx, d.String()); err == nil {
+				img = &cimg
+				dgst = d.Digest()
+				if cimg.Target.Digest != dgst {
+					// Ambiguous image reference, use reference name
+					log.G(ctx).WithField("image", refOrID).WithField("target", cimg.Target.Digest).Warn("digest reference points to image with a different digest")
+					dgst = cimg.Target.Digest
+				}
+			} else if !cerrdefs.IsNotFound(err) {
+				return nil, nil, convertError(err)
+			} else {
+				dgst = d.Digest()
+			}


(just a note when reviewing, wasn't sure which cases exactly would hit that path)

For the ^(sha256:)?([a-f0-9]{4,64})$ there are few distinct reference cases (refOrID):

sha256:abcdef7890123456789012345678901234567890123456789012345678901234 (algo prefixed full digest)

sha256:abce (algo prefixed truncated digest)

abcdef7890123456789012345678901234567890123456789012345678901234 (unprefixed full digest)

abce (unprefixed truncated digest)

This is what the reference is resolved to in each case (reference.ParseAnyReference):

docker.io/library/sha256:abcdef7890123456789012345678901234567890123456789012345678901234
type: reference.taggedReference

docker.io/library/sha256:abce
type: reference.taggedReference

sha256:abcdef7890123456789012345678901234567890123456789012345678901234
type: reference.digestReference

docker.io/library/abce
type: reference.repository

(https://go.dev/play/p/FU-Ysh-I2cI)

1 Seems a bit surprising. I can add more unit tests around these cases to lessen the surprise.

vvoland · 2023-12-01T14:26:59Z

Oh, also noticed this breaks the TestRemoveByDigest test.
The thing it checks is that you should be able to delete the image using the named and digested reference (taken from RepoDigests).

$ docker images
REPOSITORY   TAG       IMAGE ID       CREATED        SIZE
busybox      latest    3fbc63216742   4 months ago   6.09MB

$ docker inspect busybox -f {{.RepoDigests}}
[busybox@sha256:3fbc632167424a6d997e74f52b878d7cc478225cffac6bc977eedfe51c7f4e79]

# This should work
$ docker rmi busybox@sha256:3fbc632167424a6d997e74f52b878d7cc478225cffac6bc977eedfe51c7f4e79
Error response from daemon: No such image: busybox@sha256:3fbc632167424a6d997e74f52b878d7cc478225cffac6bc977eedfe51c7f4e79

dmcgowan · 2023-12-06T06:11:20Z

@vvoland I'll add more unit tests around that case. I don't think the current tests match all the format of references which get created.

dmcgowan · 2023-12-09T01:12:54Z

daemon/containerd/image.go

+			if !cerrdefs.IsNotFound(err) {
+				return nil, nil, convertError(err)
+			}
+			return nil, nil, images.ErrImageDoesNotExist{Ref: parsed}


Related to the remove by repo digest test, this is the case that gets hit. There is currently a unit test that checks for this condition. When a digest is provided we can probably fall back to similar logic as when just a hash is provided. The tricky part is whether to return a matched image or not, since in this case there was no actual matched image and trying to avoid any sort of fuzzy matching with whats in containerd. However I think it makes sense to support the repo@digest case even if it is new functionality and doesn't represent any specific image which has been created.

dmcgowan · 2023-12-12T06:28:53Z

Updated with functionality to match images by repo + digest on rmi.

rumpl · 2023-12-12T10:52:09Z

A lot of tests are failing now in integration-cli with assertion failed: error is not nil: Error response from daemon: No such image: busybox:latest: [d411442a41e1e] failed to download busybox https://github.com/moby/moby/actions/runs/7177758758?pr=46840#summary-19545421070

dmcgowan · 2023-12-13T00:55:02Z

Updated, @rumpl late night commit, I missed the zero case. I added a unit test for that now as well.

Still looking at TestDockerCLIRmiSuite/TestRmiUntagHistoryLayer but that isn't holding this PR back.

daemon/containerd/image_delete.go

daemon/containerd/image_delete_test.go

daemon/containerd/image.go

daemon/containerd/service.go

daemon/containerd/image_test.go

daemon/containerd/image_delete.go

daemon/containerd/image.go

daemon/containerd/image_delete.go

daemon/containerd/image.go

Add single resolve function to get a consistent list of images matching the same digest. Signed-off-by: Derek McGowan <derek@mcg.dev>

Ensure that when removing an image, an image is checked consistently against the images with the same target digest. Add unit testing around delete. Signed-off-by: Derek McGowan <derek@mcg.dev>

When repo and digest is provided, remove all references within a repository for the given digest. Signed-off-by: Derek McGowan <derek@mcg.dev>

dmcgowan · 2023-12-19T00:48:26Z

Updated with comment and added a commit which fixes TestRmiUntagHistoryLayer.

The fix for the untag is to re-add the dangling image before deleting the last tag when there are children and the rmi is not forced. I think this matches the intended logic around dangling image, another label scan right before delete is not optimal but we might be able to address that with switching to gc labels in 2.0.

daemon/containerd/image_delete.go

Signed-off-by: Derek McGowan <derek@mcg.dev>

vvoland

LGTM, thanks!

thaJeztah

LGTM

thaJeztah · 2023-12-19T16:58:45Z

integration-cli/docker_cli_rmi_test.go

 	assert.ErrorContains(c, err, "")
 	assert.Assert(c, strings.Contains(out, cID[:12]))
-	assert.Assert(c, strings.Contains(out, "(must force)"))
+	assert.Assert(c, strings.Contains(out, "(must force)") || strings.Contains(out, "(must be forced)"))


😭 do we know where these originate from, or is one in the CLI and one in the daemon?

Both are in the daemon, it is just one case in the old image code that had the reason hardcoded in as (must force), although both containerd and non-containerd use the reason as must be forced. I could have updated the non-containerd code but seems like it could be considered separately.

Oh! My search only found one, but perhaps I did it wrong.

Yeah, they were somewhat weird error messages in either case.

thaJeztah · 2023-12-19T16:59:59Z

daemon/containerd/soft_delete.go

+	// TODO: Add with target option
+	err := i.images.Delete(compatcontext.WithoutCancel(ctx), img.Name)


We should create a tracking ticket for this

thaJeztah · 2023-12-19T17:00:12Z

daemon/containerd/soft_delete.go

 }

 func isDanglingImage(image containerdimages.Image) bool {
+	// TODO: Also check for expired


Ditto (we should create a tracking ticket for this)

dmcgowan marked this pull request as ready for review November 22, 2023 04:46

vvoland added status/2-code-review area/images Image Distribution containerd-integration Issues and PRs related to containerd integration labels Nov 22, 2023

rumpl closed this Nov 27, 2023

rumpl reopened this Nov 27, 2023

rumpl reviewed Nov 27, 2023

View reviewed changes

daemon/containerd/image_delete.go Outdated Show resolved Hide resolved

vvoland reviewed Nov 27, 2023

View reviewed changes

dmcgowan force-pushed the c8d-rmi-cleanup branch 2 times, most recently from 0b9a7f7 to 04526ea Compare November 30, 2023 22:17

thaJeztah requested review from rumpl and vvoland December 1, 2023 11:55

vvoland reviewed Dec 1, 2023

View reviewed changes

vvoland mentioned this pull request Dec 7, 2023

[epic] fix remaining failing tests with the containerd image store #46760

Closed

54 tasks

thaJeztah assigned dmcgowan Dec 7, 2023

dmcgowan commented Dec 9, 2023

View reviewed changes

dmcgowan force-pushed the c8d-rmi-cleanup branch 2 times, most recently from 82a655e to 3f51b7a Compare December 12, 2023 06:28

dmcgowan force-pushed the c8d-rmi-cleanup branch from 3f51b7a to 5c48d18 Compare December 13, 2023 00:12

vvoland reviewed Dec 14, 2023

View reviewed changes

dmcgowan force-pushed the c8d-rmi-cleanup branch from 5c48d18 to 49c34d7 Compare December 15, 2023 06:32

thaJeztah reviewed Dec 18, 2023

View reviewed changes

daemon/containerd/image.go Show resolved Hide resolved

dmcgowan added 2 commits December 18, 2023 14:04

Add resolve all images and unit test

529d19b

Add single resolve function to get a consistent list of images matching the same digest. Signed-off-by: Derek McGowan <derek@mcg.dev>

Update cleanup logic to use resolve all images

87c87bc

Ensure that when removing an image, an image is checked consistently against the images with the same target digest. Add unit testing around delete. Signed-off-by: Derek McGowan <derek@mcg.dev>

dmcgowan force-pushed the c8d-rmi-cleanup branch from 49c34d7 to 30255e4 Compare December 18, 2023 22:19

Add support for removing repo and digest

0c6e912

When repo and digest is provided, remove all references within a repository for the given digest. Signed-off-by: Derek McGowan <derek@mcg.dev>

dmcgowan force-pushed the c8d-rmi-cleanup branch from 30255e4 to 5c95e2d Compare December 19, 2023 00:45

vvoland reviewed Dec 19, 2023

View reviewed changes

daemon/containerd/image_delete.go Outdated Show resolved Hide resolved

Add dangling image reference on delete when last image has children

cf1ea92

Signed-off-by: Derek McGowan <derek@mcg.dev>

dmcgowan force-pushed the c8d-rmi-cleanup branch from 5c95e2d to cf1ea92 Compare December 19, 2023 14:04

vvoland approved these changes Dec 19, 2023

View reviewed changes

thaJeztah approved these changes Dec 19, 2023

View reviewed changes

thaJeztah merged commit 43a82da into moby:master Dec 19, 2023

thaJeztah added this to the 25.0.0 milestone Dec 19, 2023

dmcgowan deleted the c8d-rmi-cleanup branch January 8, 2024 16:05

		// TODO: Add with target option
		err := i.images.Delete(compatcontext.WithoutCancel(ctx), img.Name)

Conversation

dmcgowan commented Nov 22, 2023 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland commented Dec 1, 2023

Uh oh!

dmcgowan commented Dec 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Dec 12, 2023

Uh oh!

rumpl commented Dec 12, 2023

Uh oh!

dmcgowan commented Dec 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmcgowan commented Dec 19, 2023

Uh oh!

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dmcgowan commented Nov 22, 2023 •

edited by thaJeztah

Loading