c8d/prune: Fix images being deleted when they're still used with a different reference by vvoland · Pull Request #45839 · moby/moby

vvoland · 2023-06-28T14:27:58Z

Fixes: [containerd snapshotter] Prune attempts to remove untagged/dangling images still in use by running containers #45732

- What I did
Fixed image prune -a deleting dangling images used by containers.

- How I did it

Exclude dangling tag of the images used by containers (this makes sure that if the original image was deleted we don't delete the dangling image)
Exclude images that are used by containers which were started by specifying an image ID instead of a specific name.

- How to verify it

$ make TEST_INTEGRATION_USE_SNAPSHOTTER=1 \
       DOCKER_GRAPHDRIVER=overlayfs \
       TEST_FILTER=TestPruneDontDeleteUsedDangling \
       test-integration
...
INFO: Testing against a local daemon
=== RUN   TestPruneDontDeleteUsedDangling
--- PASS: TestPruneDontDeleteUsedDangling (0.07s)
PASS

DONE 1 tests in 1.079s

- Description for the changelog

containerd integration: `docker image prune -a` no longer removes images still in use by running containers

- A picture of a cute animal (not mandatory but encouraged)

laurazard

LGTM

thaJeztah · 2023-06-28T15:31:48Z

Looks like there's a failure;

=== FAIL: github.com/docker/docker/integration/image TestPruneDontDeleteUsedDangling (0.03s)
    prune_test.go:23: assertion failed: error is not nil: Error response from daemon: No such image: sha256:0df1207206e5288f4a989a2f13d1f5b3c4e70467702c1d5d21dfc9f002b7bd43

thaJeztah · 2023-06-28T15:38:53Z

I'm wondering though; if 2 images are tags for the same digest, wouldn't we have to use the reverse? Untag the image, and let containerd prune / garbage collect unused content?

(If we use "force", them probably we must change the tag name to a dangling image name instead)

thaJeztah · 2023-06-28T15:43:14Z

(I'm not sure if that would still provide us a way to report the amount of space we reclaimed though)

thaJeztah · 2023-06-28T15:50:45Z

Discussing with @neersighted and we were considering if we should take a different approach in general;

whenever we create an image, always create two images:
- 1 image with the name:tag (if any)
- 1 "dangling" image
the second one is effectively replacing the graph driver "content addressable" store (access image by digest)

Now when we remove an image; we either remove the image's name:tag from containerd's store, or the dangling image name (if there's no more references to it).

So instead of creating dangling image references "on the fly", we can create them up-front, and use them for reference counting on our side

vvoland · 2023-06-29T14:06:04Z

Ah, the failure is on Windows only, because the hack/make/.build-empty-images doesn't run on Windows, so there's no special dangling image.

So instead of creating dangling image references "on the fly", we can create them up-front, and use them for reference counting on our side.

I thought about it before, but it seemed to be a bit more tricky because the image creation is not always on our side (for example building image with Buildkit) whereas it's more common for Moby to perform image deletion.

On the other hand, that would indeed give us more control over the GC and would make more sense in cases where an explicit image digest is used instead of a name (so for example docker run 3abd238 wouldn't pick any image that points to the sha256:3abd238...).

We could give it a try and see if that doesn't complicate things too much. It's a bit orthogonal to this PR though, so possibly we should just fix this issue here and then continue in a follow-up/separate ticket.

I'm wondering though; if 2 images are tags for the same digest, wouldn't we have to use the reverse? Untag the image, and let containerd prune / garbage collect unused content?

Yes, this will cause the unused tag which also points to the same target as the used tag.
Looks like this can be fixed easily by just always excluding the dangling image of the container image.

Also, there's another unsolved issue - images referenced by their id (docker run 2376a0c12759aa or docker run sha256:...) will be pruned.

Fixed it now.

integration/image/inspect_test.go

daemon/containerd/soft_delete.go

integration/image/prune_test.go

daemon/containerd/image_prune.go

neersighted

Some initial nits, though I'm not a fan of how imperative this code is, and I think I might send a PR to your branch with a restructured implementation that keeps each category of prune exclusion more discrete.

daemon/containerd/image_prune.go

neersighted

One nit, but overall LGTM. I think that we should restructure this logic in the future, but I think that we can take this as an improvement for now, and think about lifecycle management more holistically in the future (during which we can revisit this logic, and perhaps even generalize it).

daemon/containerd/image_prune.go

integration/image/inspect_test.go

thaJeztah

LGTM (but let me know if you want to address that nit; not a blocker)

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

So we don't override the original Labels in the passed image object. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

If an image is only by id instead of its name, don't prune it completely. but only untag it and create a dangling image for it. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

thaJeztah

LGTM

neersighted · 2023-06-30T16:37:18Z

Backported at #45857

thaJeztah · 2023-06-30T16:39:36Z

@neersighted commits still align? I see there may be a potential race condition between you cherry-picking and Pawel force-pushing 😓 🙈

neersighted · 2023-06-30T16:40:43Z

That's one of the things to check when reviewing a backport PR 😉

But yes, I made sure to re-cherry (with rerere) once Pawel pushed again; all SHAs are aligned to the latest version of this PR.

thaJeztah · 2023-06-30T16:42:30Z

Thanks!

thaJeztah · 2023-06-30T17:12:38Z

Timeout in one of the BuildKit tests (panic: test timed out after 30m0s); kicked it again

vvoland added status/2-code-review area/images Image Distribution kind/bugfix PR's that fix bugs containerd-integration Issues and PRs related to containerd integration process/cherry-pick/24.0 labels Jun 28, 2023

vvoland added this to the 25.0.0 milestone Jun 28, 2023

vvoland requested review from laurazard, neersighted and thaJeztah June 28, 2023 14:30

laurazard approved these changes Jun 28, 2023

View reviewed changes

vvoland force-pushed the c8d-dont-prune-used branch from 577f7c1 to 1d6d432 Compare June 29, 2023 14:03

thaJeztah reviewed Jun 29, 2023

View reviewed changes

neersighted requested changes Jun 29, 2023

View reviewed changes

vvoland force-pushed the c8d-dont-prune-used branch from 1d6d432 to c9d8569 Compare June 30, 2023 09:18

vvoland changed the title ~~c8d/prune: Exclude used images by id~~ c8d/prune: Fix images being deleted when they're still used with a different reference Jun 30, 2023

neersighted approved these changes Jun 30, 2023

View reviewed changes

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

vvoland force-pushed the c8d-dont-prune-used branch from c9d8569 to edbfecb Compare June 30, 2023 16:09

thaJeztah reviewed Jun 30, 2023

View reviewed changes

integration/image/inspect_test.go Show resolved Hide resolved

thaJeztah approved these changes Jun 30, 2023

View reviewed changes

vvoland added 5 commits June 30, 2023 18:18

testutil/environment: Add GetTestDanglingImageId

a96e604

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

c8d/softDelete: Extract ensureDanglingImage

2b0655a

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

c8d/softDelete: Deep copy Labels

a6d5db3

So we don't override the original Labels in the passed image object. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

c8d/prune: Exclude dangling tag of the images used by containers

a93298d

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

c8d/prune: Handle containers started from image id

e638351

If an image is only by id instead of its name, don't prune it completely. but only untag it and create a dangling image for it. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the c8d-dont-prune-used branch from edbfecb to e638351 Compare June 30, 2023 16:19

thaJeztah approved these changes Jun 30, 2023

View reviewed changes

neersighted mentioned this pull request Jun 30, 2023

[24.0 backport] c8d/prune: Fix images being deleted when they're still used with a different reference #45857

Merged

neersighted added process/cherry-picked and removed process/cherry-pick/24.0 labels Jun 30, 2023

neersighted added status/4-merge and removed status/2-code-review labels Jun 30, 2023

thaJeztah merged commit a61494e into moby:master Jun 30, 2023

Conversation

vvoland commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laurazard left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 28, 2023

Uh oh!

thaJeztah commented Jun 28, 2023

Uh oh!

thaJeztah commented Jun 28, 2023

Uh oh!

thaJeztah commented Jun 28, 2023

Uh oh!

vvoland commented Jun 29, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neersighted left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neersighted left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thaJeztah 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!

neersighted commented Jun 30, 2023

Uh oh!

thaJeztah commented Jun 30, 2023

Uh oh!

neersighted commented Jun 30, 2023

Uh oh!

thaJeztah commented Jun 30, 2023

Uh oh!

thaJeztah commented Jun 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vvoland commented Jun 28, 2023 •

edited

Loading