Skip to content

c8d: add support for removing images by shortID#45362

Merged
thaJeztah merged 1 commit intomoby:masterfrom
laurazard:c8d-delete-short-images
Apr 26, 2023
Merged

c8d: add support for removing images by shortID#45362
thaJeztah merged 1 commit intomoby:masterfrom
laurazard:c8d-delete-short-images

Conversation

@laurazard
Copy link
Member

@laurazard laurazard commented Apr 19, 2023 •
edited
Loading

- What I did

Added support for docker image rm with the shortID

- How I did it

  • daemon/containerd/image.go - resolveDescriptor: factored out the containerd image fetching part of the code from the rest, since we need the full containerd image struct with it's name to call the containerd image service to delete it.

- How to verify it

docker rmi [shortID]

- Description for the changelog

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

image

@laurazard laurazard force-pushed the c8d-delete-short-images branch 2 times, most recently from 36b07d0 to 7be6b39 Compare April 20, 2023 15:23
@laurazard laurazard marked this pull request as ready for review April 20, 2023 15:28
vvoland
vvoland reviewed Apr 20, 2023
View reviewed changes
@laurazard laurazard force-pushed the c8d-delete-short-images branch from 7be6b39 to e57708e Compare April 21, 2023 09:17
@laurazard
c8d: add support for removing images by shortID
8df3db4 
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
@laurazard laurazard force-pushed the c8d-delete-short-images branch from e57708e to 8df3db4 Compare April 21, 2023 09:31
@thaJeztah thaJeztah 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 labels Apr 21, 2023
@thaJeztah thaJeztah added this to the 24.0.0 milestone Apr 21, 2023
@rumpl rumpl modified the milestones: 24.0.0, v-future Apr 24, 2023
@thaJeztah
Copy link
Member

thaJeztah commented Apr 25, 2023

Ugh.. the existing logic is so confusing 😞

This PR (with containerd integration enabled):

docker pull busybox
Using default tag: latest
b5d6fe071263: Download complete
ba7000206594: Download complete
3fbaf71a998b: Download complete
b50100f25006: Download complete

docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
busybox             latest              b5d6fe071263        4 minutes ago       5.86MB


docker tag busybox b5d6fe071263

docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
b5d6fe071263        latest              b5d6fe071263        4 seconds ago       5.86MB
busybox             latest              b5d6fe071263        5 minutes ago       5.86MB

docker image ls
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
busybox             latest              b5d6fe071263        5 minutes ago       5.86MB


docker tag busybox b5d6fe071263

docker rmi  b5d6

docker rmi  b5d6
Untagged: b5d6

docker image ls
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
busybox             latest              b5d6fe071263        6 minutes ago       5.86MB

without containerd integration

docker pull busybox
Using default tag: latest
latest: Pulling from library/busybox
b50100f25006: Pull complete
Digest: sha256:b5d6fe0712636ceb7430189de28819e195e8966372edfc2d9409d79402a0dc16
Status: Downloaded newer image for busybox:latest

docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
busybox             latest              3fbaf71a998b        5 weeks ago         3.73MB

docker tag busybox 3fbaf71a998b

docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
3fbaf71a998b        latest              3fbaf71a998b        5 weeks ago         3.73MB
busybox             latest              3fbaf71a998b        5 weeks ago         3.73MB

docker rmi 3fbaf71a998b
Error response from daemon: conflict: unable to delete 3fbaf71a998b (must be forced) - image is referenced in multiple repositories

docker rmi 3fba
Error response from daemon: conflict: unable to delete 3fbaf71a998b (must be forced) - image is referenced in multiple repositories

docker tag busybox 3fba

docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
3fbaf71a998b        latest              3fbaf71a998b        5 weeks ago         3.73MB
busybox             latest              3fbaf71a998b        5 weeks ago         3.73MB
3fba                latest              3fbaf71a998b        5 weeks ago         3.73MB

docker rmi 3fba
Error response from daemon: conflict: unable to delete 3fbaf71a998b (must be forced) - image is referenced in multiple repositories

And now I'm wondering if that error is correct; in this case there's a full match on the image name (3fba:latest), so deleting it should not probably not be an issue. I'm wondering if it also checks if there's a ID matching it, and if that's what's triggering the issue

moby/daemon/images/image_delete.go

Lines 380 to 386 in 8012023

// Check if any repository tags/digest reference this image.
if mask&conflictActiveReference != 0 && len(i.referenceStore.References(imgID.Digest())) > 0 {
return &imageDeleteConflict{
imgID: imgID,
message: "image is referenced in multiple repositories",
}
}

@laurazard
Copy link
Member Author

laurazard commented Apr 26, 2023

Right! After this PR I started digging in to bringing in all the logic from

moby/daemon/images/image_delete.go

Line 295 in 7624f8a

func (i *ImageService) imageDeleteHelper(imgID image.ID, records *[]types.ImageDeleteResponseItem, force, prune, quiet bool) error {
and realized this was a bit more complex than I initially considered.
Some of the logic is straightforward to port over, but some of it doesn't really map very cleanly 😅

For your question about the current implementation:

docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
3fbaf71a998b        latest              3fbaf71a998b        5 weeks ago         3.73MB
busybox             latest              3fbaf71a998b        5 weeks ago         3.73MB
3fba                latest              3fbaf71a998b        5 weeks ago         3.73MB

docker rmi 3fba
Error response from daemon: conflict: unable to delete 3fbaf71a998b (must be forced) - image is referenced in multiple repositories

I think it's okay if this is an error since it's not clear if the user wanted to delete the image tagged 3fba or 3fba was a shortID for the image ID (although docker rmi 3fba:latest should work without problem).

I'll leave this here for now since it's still an improvement over the current situation, and probably open a PR on top of this one bringing over some of the rest of the logic.

@thaJeztah
Copy link
Member

thaJeztah commented Apr 26, 2023

Sorry, probably should've approved the PR itself, as it does fix the immediate "short-ID" case 😅

and realized this was a bit more complex than I initially considered.
Some of the logic is straightforward to port over, but some of it doesn't really map very cleanly 😅

Yes, some of te logic is too complicated, but it looks like there's also some inconsistencies, and we should properly document and verify. The general "rule" in order of priority is;

  1. Full match on full ID
  2. Full match on name
  3. Match on ID prefix
  • Name is matched before ID-prefix to prevent (e.g.) a container named db (commonly used for "database" containers) to match an ID with a db prefix.
  • Names are always matched in full, not prefix
  • If 3. matches multiple items, an error is produced that the prefix is not unique enough, and a longer prefix should be provided

Try a couple of times to get a common prefix, then create a container with a name matching either a prefix or a full ID;

docker create busybox
a0aaded9b02e5c5b6080deddef3d53eb6ea2e4d552b09ef85ad9342403181cd1
docker create busybox
ae794a02ba6d69aa12772272559f48cca9646076b9a980a04ad1eb0610d4ef2b

docker create --name ae794a02ba6d69aa12772272559f48cca9646076b9a980a04ad1eb0610d4ef2b busybox
docker create --name ae794a02ba6d busybox

docker ps -a
CONTAINER ID   IMAGE     COMMAND   CREATED          STATUS    PORTS     NAMES
80b3a1d9a5cf   busybox   "sh"      3 seconds ago    Created             ae794a02ba6d
3bda3b4ea646   busybox   "sh"      4 seconds ago    Created             ae794a02ba6d69aa12772272559f48cca9646076b9a980a04ad1eb0610d4ef2b
ae794a02ba6d   busybox   "sh"      11 minutes ago   Created             quizzical_shirley
a0aaded9b02e   busybox   "sh"      11 minutes ago   Created             intelligent_lamport

A. Full ID matches the ID:

docker container inspect --format='ID: {{.Id}} NAME: {{.Name}}' ae794a02ba6d69aa12772272559f48cca9646076b9a980a04ad1eb0610d4ef2b
ID: ae794a02ba6d69aa12772272559f48cca9646076b9a980a04ad1eb0610d4ef2b NAME: /quizzical_shirley

B. Inspecting the short ID returns the container named ae794a02ba6d (this is the "db" vs "db" prefix case)

docker container inspect --format='ID: {{.Id}} NAME: {{.Name}}' ae794a02ba6d
ID: 80b3a1d9a5cff200d69ef350b01688224f9ee20cb19e0eb3f394780ab91fcad2 NAME: /ae794a02ba6d

C. Inspecting a shorter ID-prefix produces an error;

docker container inspect --format='ID: {{.Id}} NAME: {{.Name}}' a
Error response from daemon: Multiple IDs found with provided prefix: a

D. Using a longer (non-ambiguous) ID-prefix:

docker container inspect --format='ID: {{.Id}} NAME: {{.Name}}' ae
ID: ae794a02ba6d69aa12772272559f48cca9646076b9a980a04ad1eb0610d4ef2b NAME: /quizzical_shirley

E. Now do a docker container inspect with the common prefix;

container inspect a
[]
Error response from daemon: Multiple IDs found with provided prefix: a

For B. we could consider also producing an error (both "name" and "ID-prefix" matches), because the workaround for that case would be to use the full ID. That may be something to discuss (would be a breaking change). Note that there's no "formal" definition of "Short ID" (as shown in overviews); while most (all) overviews use the same prefix-length, we cannot depend on that to never change, so we should treat them just as any other "ID-prefix" (of any length).

I think it's okay if this is an error since it's not clear if the user wanted to delete the image tagged 3fba or 3fba was a shortID for the image ID (although docker rmi 3fba:latest should work without problem).

Yes, from some perspectives, it makes sense, but I have a feeling the error is produced for the wrong reasons (it should at most produce a similar error as for containers Multiple IDs found with provided prefix). The confusing bit here is that;

  • There was a "full" match on Name (that is; after normalizing 3fba to docker.io/library/3fba:latest)
  • But (for some reason) it continued and also started matching on ID-prefix (this is at least inconsistent)
    • The error produced is because of ☝️
    • It matches on ID-prefix
    • ... Finds multiple tags for that ID (prefix)
    • ... Produces an error, because it assumes the user wants to delete the image by ID
    • ... Which is not possible if multiple tags reference the same ID (as doing so would delete all tags / images with the same ID)
    • ☝️ The "must be forced" recommendation in the error message means exactly that: remove image with ID 3fba, including all tags (names / images) pointing to it
    • ⚠️ (to be verified) if --force works with ID-prefix and in that case matches on prefix, that's potentially dangerous due to race conditions; say that other images were pulled in the meantime that also match this prefix, then "force" removing with a ID PREFIX would remove those (and all names/tags pointing to them).

Continuing with my previous example;

docker images
REPOSITORY     TAG       IMAGE ID       CREATED       SIZE
7cfbbec8963d   latest    7cfbbec8963d   5 weeks ago   4.86MB
busybox        latest    7cfbbec8963d   5 weeks ago   4.86MB

Deleting with a prefix shows an error ("must be forced");

docker rmi 7cf
Error response from daemon: conflict: unable to delete 7cfbbec8963d (must be forced) - image is referenced in multiple repositories

Following that recommendation deletes both images (it's a single image: so deletes the image and any tag/name pointing to it):

docker rmi --force 7cf
Untagged: 7cfbbec8963d:latest
Untagged: busybox:latest
Untagged: busybox@sha256:b5d6fe0712636ceb7430189de28819e195e8966372edfc2d9409d79402a0dc16
Deleted: sha256:7cfbbec8963d8f13e6c70416d6592e1cc10f47a348131290a55d43c3acab3fb9
Deleted: sha256:baacf561cfff825708763ce7ee4a18293716c533e6ece3bd39009a5fb3c804d2

docker images
REPOSITORY   TAG       IMAGE ID   CREATED   SIZE

The same also happens when using 7cfbbec8963d ("full match" (after normalizing) on image name )

thaJeztah
thaJeztah approved these changes Apr 26, 2023
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I'll try to write up some tickets about the handling of ID's/Names (and the potential issue in the existing implementation)

@thaJeztah thaJeztah merged commit 2379b49 into moby:master Apr 26, 2023
@thaJeztah thaJeztah modified the milestones: v-future, 24.0.0 Apr 26, 2023
@vvoland vvoland mentioned this pull request Apr 28, 2023
Closed
This was referenced Apr 28, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@vvoland vvoland vvoland approved these changes

@rumpl rumpl Awaiting requested review from rumpl

Assignees

No one assigned

Labels

area/images Image Distribution containerd-integration Issues and PRs related to containerd integration kind/bugfix PR's that fix bugs status/2-code-review

Projects

None yet

Milestone

24.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@laurazard @thaJeztah @vvoland @rumpl