Handle multi-platform images when listing them by rumpl · Pull Request #113 · rumpl/moby

rumpl · 2022-11-16T09:39:52Z

The images list will return an entry for each platform of an image

vvoland

Left one DRY comment and noticed an issue with assigning the SharedSize.

Overall, with the current CLI UX (each image entry differs only with the size, there's no indication that they are actually different platforms) I think it would make more sense to show just one entry, but with combined size of all present platforms.
It would also be more true to reality - it's one multi-platform image and not different images. What does the user gain by having multiple separate entries with the same ID? He can't even reference them on their own, for example if he saw one entry with big size, took its ID and did docker rm then all other entries would get deleted too.

Ideally (this would require CLI changes too though), I think it would make sense to have root entry representing the manifest list (which would show combined size) and children entry for each present platform that directly represents the manifest for that platform (which shows the size only for that platform). Root entry would have the manifest list hash and each platform entry would have the platform's manifest hash. Problem with this approach is that we would have to support manifest-hash as image id in all commands, which would be a bit difficulty due to being forced to have a separate image in the ImageStore for each manifest.

vvoland · 2022-11-16T12:28:29Z

daemon/containerd/image_list.go

+		platforms, err := images.Platforms(ctx, i.client.ContentStore(), img.Target)
 		if err != nil {
-			return nil, err
+			image, c, err := i.singlePlatformImage(ctx, img, c8dplatforms.DefaultSpec(), opts)


I don't think we need to duplicate the same logic for case where images.Platforms returns error. This can only happen if the img.Target itself is missing, so the default platform won't be there neither.

vvoland · 2022-11-16T12:30:28Z

daemon/containerd/image_list.go

 		}
-		chainIDs := identity.ChainIDs(diffIDs)
 		if opts.SharedSize {
 			root[n] = &chainIDs


... which is used here to increment usages of each layer.

vvoland · 2022-11-16T12:30:47Z

daemon/containerd/image_list.go

+		} else {
+			for _, platform := range platforms {
+				image, c, err := i.singlePlatformImage(ctx, img, platform, opts)
+				chainIDs = c


This will save the chainIDs only for the last iterated platforms (see below) ...

daemon/containerd/image_list.go

rumpl · 2022-11-16T17:33:46Z

Overall, with the current CLI UX (each image entry differs only with the size, there's no indication that they are actually different platforms) I think it would make more sense to show just one entry, but with combined size of all present platforms. It would also be more true to reality - it's one multi-platform image and not different images. What does the user gain by having multiple separate entries with the same ID? He can't even reference them on their own, for example if he saw one entry with big size, took its ID and did docker rm then all other entries would get deleted too.

Ideally (this would require CLI changes too though), I think it would make sense to have root entry representing the manifest list (which would show combined size) and children entry for each present platform that directly represents the manifest for that platform (which shows the size only for that platform). Root entry would have the manifest list hash and each platform entry would have the platform's manifest hash. Problem with this approach is that we would have to support manifest-hash as image id in all commands, which would be a bit difficulty due to being forced to have a separate image in the ImageStore for each manifest.

I agree, this is a first step, we can make the changes to the API/cli later...

rumpl · 2022-11-16T17:34:45Z

Overall, with the current CLI UX (each image entry differs only with the size, there's no indication that they are actually different platforms) I think it would make more sense to show just one entry, but with combined size of all present platforms.

I don't think this is the best way to represent it, I think it should be one per platform but with a PLATFROM column

vvoland

A few more thing related to SharedSize and checking platform presence 😅

I don't think this is the best way to represent it, I think it should be one per platform but with a PLATFROM column

Yes, I meant that until we have that column in the CLI. But maybe we can just ignore this for now and let it be confusing for now 😄 At least it's better than having no output at all.

vvoland · 2022-11-17T14:11:26Z

daemon/containerd/image_list.go

@@ -66,63 +70,32 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions)
 		root = make([]*[]digest.Digest, len(imgs))


The slice size is too small for the root[n+m] = &chainIDs (will panic). Perhaps we should just use append and reserve capacity instead of setting the length upfront.

Suggested change

root = make([]*[]digest.Digest, len(imgs))

root = make([]*[]digest.Digest, 0, len(imgs))

(see next comment)

vvoland · 2022-11-17T14:18:56Z

daemon/containerd/image_list.go

-			repoTags = []string{familiarName}
-			repoDigests = []string{familiarName + "@" + img.Target().Digest.String()} // "hello-world@sha256:bfea6278a0a267fad2634554f4f0c6f31981eea41c553fdf5a83e95a41d40c38"
+			if opts.SharedSize {
+				root[n+m] = &chainIDs


(follow-up to 1 comment)

Suggested change

root[n+m] = &chainIDs

root = append(root, &chainIDs)

daemon/containerd/image_list.go

vvoland · 2022-11-17T14:38:10Z

daemon/containerd/image_list.go

-type imageFilterFunc func(image containerd.Image) bool
+func (i *ImageService) singlePlatformImage(ctx context.Context, img images.Image, platform v1.Platform, opts types.ImageListOptions) (*types.ImageSummary, []digest.Digest, error) {
+	platMC := c8dplatforms.OnlyStrict(platform)
+	if avail, _, _, _, err := images.Check(ctx, i.client.ContentStore(), img.Target, platMC); !avail {


avail can be true in case the manifest is present in the content store, but some of its children is missing.
In this case, and if the Check returns an error, we probably want to do an early return:

Suggested change

if avail, _, _, _, err := images.Check(ctx, i.client.ContentStore(), img.Target, platMC); !avail {

avail, _, _, missing, err := images.Check(ctx, i.client.ContentStore(), img.Target, platMC)

if !avail || err != nil || len(missing) > 0 {

Are you sure about that, the docs says If available is true, the caller can assume that required represents the complete set of content required for the image.

Yes. The available name is a bit misleading, one has to read doc multiple times to understand it correctly 😄

If available is true, the caller can assume that required (one of return values) represents the complete set of content required for the image.

And the latter fragment:
missing will have the components that are part of required but not available in the provider.

The code also confirms:
https://github.com/containerd/containerd/blob/main/images/image.go#L323

Yeah ok looks like the documentation is not right

So I just tested this (on an ARM machine):

$ docker pull --platform linux/amd64 hello-world $ docker images

If I add the check for missing I can't see the image, if I remove it everything is fine, I can even see the size of the image. So I don't think we should check for missing.

missing will contain the descriptor of the rootfs but I can run the image just fine (with docker run --platform linux/amd64 hello-world)...

That's strange, I'm getting different results. Pulling amd64 starting from fresh state, yields these results for me:

# docker pull --platform linux/amd64 hello-world

available=true error="<nil>" missing="[]" platform="{amd64 linux [] }" available=false error="<nil>" missing="[{application/vnd.docker.distribution.manifest.list.v2+json sha256:faa03e786c97f07ef34423fccceeec2398ec8a5759259f94d99078f264e9d7af 2561 [] map[] [] <nil>}]" platform="{arm linux [] v5}" available=false error="<nil>" missing="[{application/vnd.docker.distribution.manifest.list.v2+json sha256:faa03e786c97f07ef34423fccceeec2398ec8a5759259f94d99078f264e9d7af 2561 [] map[] [] <nil>}]" platform="{arm linux [] v7}" available=false error="<nil>" missing="[{application/vnd.docker.distribution.manifest.list.v2+json sha256:faa03e786c97f07ef34423fccceeec2398ec8a5759259f94d99078f264e9d7af 2561 [] map[] [] <nil>}]" platform="{arm64 linux [] v8}" ...

Then doing:

# echo 'FROM hello-world' | docker buildx build -t asdf -

will produce:

available=true error="<nil>" missing="[]" platform="{amd64 linux [] }" available=true error="<nil>" missing="[{application/vnd.docker.image.rootfs.diff.tar.gzip sha256:b921b04d0447ddcd82a9220d887cd146f6ef39e20a938ee5e19a90fc3323e030 3684 [] map[] [] <nil>}]" platform="{arm linux [] v5}" available=true error="<nil>" missing="[{application/vnd.docker.image.rootfs.diff.tar.gzip sha256:9b157615502ddff86482f7fe2fa7a074db74a62fce12b4e8507827ac8f08d0ce 2993 [] map[] [] <nil>}]" platform="{arm linux [] v7}" available=true error="<nil>" missing="[]" platform="{arm64 linux [] v8}" available=false error="<nil>" missing="[{application/vnd.docker.distribution.manifest.list.v2+json sha256:faa03e786c97f07ef34423fccceeec2398ec8a5759259f94d99078f264e9d7af 2561 [] map[] [] <nil>}]" platform="{386 linux [] }" ...

so images ls will show multiple images:

# docker image ls REPOSITORY TAG IMAGE ID CREATED SIZE asdf latest 75f42e558e3d 7 seconds ago 4.63kB hello-world latest faa03e786c97 3 minutes ago 7.03kB hello-world latest faa03e786c97 3 minutes ago 8.25kB hello-world latest faa03e786c97 3 minutes ago 7.56kB hello-world latest faa03e786c97 3 minutes ago 7.78kB

.. which are not available locally:

# docker run --rm --platform linux/arm/v5 hello-world Unable to find image 'hello-world:latest' locally ^C

Okay my c8d was in a weird state, once I reset everything it works, I updated the code.

The images list will return an entry for each platform of an image Signed-off-by: Djordje Lukic <[email protected]>

thaJeztah · 2022-12-03T13:36:21Z

Looks like this introduced some issues with docker desktop (and the UX on the cli); see moby#44573

vvoland · 2022-12-19T15:17:27Z

daemon/containerd/image_list.go

-		var repoDigests, repoTags []string
-		if isDanglingImage(img) {
-			repoTags = []string{"<none>:<none>"}
-			repoDigests = []string{"<none>@<none>"}
-		} else {
-			familiarName := reference.FamiliarString(ref)
-			repoTags = []string{familiarName}
-			repoDigests = []string{familiarName + "@" + img.Target().Digest.String()} // "hello-world@sha256:bfea6278a0a267fad2634554f4f0c6f31981eea41c553fdf5a83e95a41d40c38"


Oh, I missed this one. Was this removed on purpose or just accidentally?

Created a PR to restore this: #126

vvoland reviewed Nov 16, 2022

View reviewed changes

rumpl force-pushed the feat-multi-platform-images branch 2 times, most recently from 24ca13b to e186129 Compare November 17, 2022 12:47

vvoland reviewed Nov 17, 2022

View reviewed changes

rumpl force-pushed the feat-multi-platform-images branch 2 times, most recently from ac75cea to 0789fc5 Compare November 21, 2022 17:38

Handle multi-platform images when listing them

9ac5d76

The images list will return an entry for each platform of an image Signed-off-by: Djordje Lukic <[email protected]>

rumpl force-pushed the feat-multi-platform-images branch from 0789fc5 to 9ac5d76 Compare November 21, 2022 17:41

rumpl requested a review from vvoland November 22, 2022 09:25

vvoland approved these changes Nov 22, 2022

View reviewed changes

rumpl merged commit 4284892 into c8d Nov 22, 2022

rumpl deleted the feat-multi-platform-images branch November 22, 2022 09:37

thaJeztah mentioned this pull request Dec 3, 2022

containerd-integration: listing multi-arch images is (partially) broken moby/moby#44573

Closed

thaJeztah mentioned this pull request Dec 4, 2022

UX: improve / design UX for multi-arch images moby/moby#44582

Open

vvoland mentioned this pull request Dec 19, 2022

c8d/list: Make dangling images parseable by the cli #126

Merged

vvoland reviewed Dec 19, 2022

View reviewed changes

thaJeztah added the to-be-upstreamed label Jan 17, 2023

laurazard mentioned this pull request Feb 13, 2023

containerd integration: handle multi-platform images moby/moby#44989

Merged

rumpl added upstreamed and removed to-be-upstreamed labels Mar 9, 2023

thaJeztah mentioned this pull request Mar 19, 2023

docker build -t <tag> ... creates 2 image references moby/moby#45179

Closed

		@@ -66,63 +70,32 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions)
		root = make([]*[]digest.Digest, len(imgs))

	root = make([]*[]digest.Digest, len(imgs))
	root = make([]*[]digest.Digest, 0, len(imgs))

	if avail, _, _, _, err := images.Check(ctx, i.client.ContentStore(), img.Target, platMC); !avail {
	avail, _, _, missing, err := images.Check(ctx, i.client.ContentStore(), img.Target, platMC)
	if !avail \|\| err != nil \|\| len(missing) > 0 {

Comments

Conversation

rumpl commented Nov 16, 2022

Uh oh!

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

Uh oh!

rumpl commented Nov 16, 2022

Uh oh!

rumpl commented Nov 16, 2022

Uh oh!

vvoland left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

vvoland Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 3, 2022

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!

3 participants

vvoland left a comment •

edited

Loading

vvoland Nov 17, 2022 •

edited

Loading

vvoland Nov 21, 2022 •

edited

Loading

vvoland Nov 21, 2022 •

edited

Loading