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.

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

c8d: Adjust "image list" to return only a single item for each image store entry #45967

Merged
merged 1 commit into from Feb 26, 2024

Conversation

tianon
Copy link
Member

@tianon tianon commented Jul 13, 2023

This will return a single entry for each name/value pair, and for now all the "image specific" metadata (labels, config, size) should be either "default platform" or "first platform we have locally" (which then matches the logic for commands like docker image inspect, etc) with everything else (just ID, maybe?) coming from the manifest list/index. That leaves room for the longer-term implementation to add new fields to describe the other images that are part of the manifest list/index.

Refs #45948, #45735 (especially #45735 (comment))

@rumpl
Copy link
Member

rumpl commented Jul 18, 2023

I don't think we should merge this, the current API response might not be backwards compatible for some clients (it is for the docker cli), but at least it gives information about all the images that are pulled/built. This change would hide that information. I think we should hold on while we get the final shape of the multi-platform aware API and a new version of the cli that knows how to show all the information about all the local images.

@thaJeztah
Copy link
Member

I do think that the current situation is confusing, and that this PR brings us one step closer to the intended outcome;

docker pull busybox:latest
1fa89c01cd04: Already exists
fc9db2894f4e: Already exists
3fbc63216742: Already exists
docker.io/library/busybox:latest

docker image ls
REPOSITORY         TAG               IMAGE ID       CREATED         SIZE
busybox            latest            3fbc63216742   5 seconds ago   6.09MB

docker image inspect --format '{{.Architecture}}' 3fbc63216742
arm64

docker image inspect --format '{{.Size}}' 3fbc63216742
1920927

docker pull --platform=linux/amd64 busybox:latest
3fbc63216742: Already exists
023917ec6a88: Download complete
a416a98b71e2: Download complete
3f4d90098f5b: Download complete
docker.io/library/busybox:latest

docker image ls
REPOSITORY         TAG               IMAGE ID       CREATED         SIZE
busybox            latest            3fbc63216742   5 seconds ago   6.09MB
busybox            latest            3fbc63216742   9 days ago      6.61MB

docker image inspect --format '{{.Architecture}}' 3fbc63216742
arm64

docker image inspect --format '{{.Size}}' 3fbc63216742
1920927

Even though there's 2 images shown, there's no way to interact with them individually; removing busybox:latest will remove both, and so does removing an image by "ID".

It currently also breaks Desktop's Dashboard; here's the overview of images (the overview does not show the right size for each):

Screenshot 2023-07-20 at 10 16 29

Search for "busybox", and you now have 4 images:

Screenshot 2023-07-20 at 10 16 37

Search again, and now you have 7 images:

Screenshot 2023-07-20 at 10 16 49

@rumpl
Copy link
Member

rumpl commented Jul 20, 2023

Even though there's 2 images shown, there's no way to interact with them individually

Exactly why my proposal was to temporarily show the manifest ID and not the manifest list ID. Also adding the platform to the images list would be a nice way to tell them appart

I do think that the current situation is confusing, and that this PR brings us one step closer to the intended outcome;

I agree that it's confusing currently, but I don't agree that this takes us any closer to the end goal. This change is also confusing, building a multi-platofrm image yields only one image in docker images, which is very confusing. That's why I think we should hold on and work on the final multi-platform aware API

@thaJeztah
Copy link
Member

building a multi-platofrm image yields only one image in docker images, which is very confusing.

That's why it's a multi-platform image. If you want multiple platform images, you would give them a different name.

@thaJeztah thaJeztah added this to the 26.0.0 milestone Feb 5, 2024
…store entry

This will return a single entry for each name/value pair, and for now
all the "image specific" metadata (labels, config, size) should be
either "default platform" or "first platform we have locally" (which
then matches the logic for commands like `docker image inspect`, etc)
with everything else (just ID, maybe?) coming from the manifest
list/index.

That leaves room for the longer-term implementation to add new fields to
describe the _other_ images that are part of the manifest list/index.

Co-authored-by: Tianon Gravi <admwiggin@gmail.com>

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

vvoland commented Feb 14, 2024

I went ahead and rebased this, addressed some of the TODOs.

Windows failures expected.

Comment on lines +211 to +214
// TODO we should probably show *something* for images we've pulled
// but are 100% shallow or an empty manifest list/index
// ("tianon/scratch:index" is an empty example image index and
// "tianon/scratch:list" is an empty example manifest list)
Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, was about to ask if we could test this. As these are minimal images, perhaps we could include them in our "frozen images"?

I'm fine with doing that in a follow-up if it's complicated though.

Copy link
Contributor

@vvoland vvoland Feb 15, 2024

Choose a reason for hiding this comment

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

Not sure if it makes sense to handle this right now.

  1. It's impossible to pull such image, you can only load it via docker load (or going via containerd store directly).
  2. Even if it is loaded, there's nothing that can be done with that image (except removing/pushing it).

Copy link
Member

Choose a reason for hiding this comment

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

Oh! I should read properly, I thought this one also described "multi-arch" image to test. But this was only for the "lots' of emptiness" case.

I mostly meant; we should have a test to verify that multiple arches of an image are properly grouped into a single image when using docker image ls (i.e., with this PR merged)

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

@thaJeztah
Copy link
Member

Gave this one another spin;

docker pull busybox
Using default tag: latest
latest: Pulling from library/busybox
c2bf9493c1bf: Download complete
Digest: sha256:6d9ac9237a84afe1516540f40a0fafdc86859b2141954b4d643af7066d598b74
Status: Downloaded newer image for busybox:latest
docker.io/library/busybox:latest


docker pull --platform=linux/amd64 busybox
Using default tag: latest
latest: Pulling from library/busybox
9ad63333ebc9: Download complete
Digest: sha256:6d9ac9237a84afe1516540f40a0fafdc86859b2141954b4d643af7066d598b74
Status: Image is up to date for busybox:latest
docker.io/library/busybox:latest

Before this PR:

docker image ls
REPOSITORY   TAG       IMAGE ID       CREATED       SIZE
busybox      latest    6d9ac9237a84   5 weeks ago   6.61MB
busybox      latest    6d9ac9237a84   5 weeks ago   6.09MB

After this PR:

docker image ls
REPOSITORY   TAG       IMAGE ID       CREATED       SIZE
busybox      latest    6d9ac9237a84   5 weeks ago   6.61MB

As a follow-up (?) we should probably look at the SIZE column; as we only have a single SIZE (but multiple images), we should consider making the SIZE the total size of those images, so that pulling additional content will show that there's more space occupied by the image.

@vvoland
Copy link
Contributor

vvoland commented Feb 26, 2024

Good call, I added test and combined the image size.
It turned out to be a bit more code than I expected: #47449 🙈

Do we want to merge this as-is and then review the follow up separately?

@thaJeztah thaJeztah merged commit ffd294e into moby:master Feb 26, 2024
140 of 142 checks passed
@tianon tianon deleted the c8d-image-list branch March 14, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/images containerd-integration Issues and PRs related to containerd integration status/4-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants