c8d: Multi-platform image list by vvoland · Pull Request #47526 · moby/moby

vvoland · 2024-03-07T10:55:36Z

needs: c8d/list: Add test and combine size #47449
related to: UX: improve / design UX for multi-arch images #44582 (comment)

Add the Manifests field to ImageSummary.

CLI initial implementation: docker/cli#4982

- How to verify it

$ docker pull busybox
$ docker images --tree

- Description for the changelog

> `GET /images/json` response now includes `Manifests` field, which contains information about the sub-manifests included in the image index. This includes things like platform-specific manifests and build attestations.
> The new field will only be populated if the request also sets the `manifests` query parameter to `true`.
> [!WARNING]  
>
> This is experimental and may change at any time without any backward compatibility.

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

vvoland · 2024-04-04T14:31:37Z

api/swagger.yaml

+          compatibility.
+        type: "array"
+        x-nullable: false
+        x-omitempty: true


Hmm CI fails with:

diff --git a/api/types/image/summary.go b/api/types/image/summary.go index 4ebbdeadf7..fed2746521 100644 --- a/api/types/image/summary.go +++ b/api/types/image/summary.go @@ -54,7 +54,7 @@ type Summary struct { // WARNING: This is experimental and may change at any time without any backward // compatibility. // - PlatformImages []PlatformImage `json:"PlatformImages,omitempty"` + PlatformImages []PlatformImage `json:"PlatformImages"` // List of content-addressable digests of locally available image manifests // that the image is referenced from. Multiple manifests can refer to the Please update api/swagger.yaml with any API changes, then run hack/generate-swagger-api.sh.

Even though I marked this one with x-omitempty 🤔

Looks like it's an issue with the outdated go-swagger, see #47827 for update

Disabled swagger model generation for this struct: 5eb892a

vvoland · 2024-05-15T15:43:59Z

moby/api/types/image/summary.go

Lines 50 to 58 in 5eb892a

    
           // Platform-specific images available for this image. 
        
           // 
        
           // Only present with the containerd integration enabled. 
        
           // 
        
           // WARNING: This is experimental and may change at any time without any backward 
        
           // compatibility. 
        
           // 
        
           PlatformImages []PlatformImage `json:"PlatformImages,omitempty"`

Thinking about it a bit more, I think we might want to expose all manifests details, not only the platform-specific images, so we can also provide information about build attestations (and possibly other things in future).

	// Manifests is a list of image manifests available in this image.  It
	// provides a more detailed view of the platform-specific image manifests or
	// other image-attached data like build attestations.
	//
	// WARNING: This is experimental and may change at any time without any backward
	// compatibility.
	//
	Manifests []ImageManifestSummary `json:"Manifests,omitempty"`

type ImageManifestKind string

const (
	ImageManifestKindImage       ImageManifestKind = "image"
	ImageManifestKindAttestation ImageManifestKind = "attestation"
	ImageManifestKindUnknown     ImageManifestKind = "unknown"
)

type ImageManifestSummary struct {
	ID string `json:"Id"`
	Descriptor ocispec.Descriptor `json:"Descriptor"`
	Available bool `json:"Available"`
	ContentSize int64 `json:"ContentSize"`

	Kind ImageManifestKind `json:"Kind"`
	// Fields below are specific to the kind of the image manifest.

	// Present only if Kind == ImageManifestKindImage.
	ImageData ImageProperties `json:"ImageData,omitempty"`

	// Present only if Kind == ImageManifestKindAttestation.
	AttestationData AttestationProperties `json:"AttestationData,omitempty"`
}

type ImageProperties struct {
	Platform ocispec.Platform `json:"Platform"`
	UnpackedSize int64 `json:"UnpackedSize"`
	Containers int64 `json:"Containers"`
}

type AttestationProperties struct {
	// For - the digest of the image manifest that this attestation is for.
	For digest.Digest `json:"For"`
}

This would also leave the gate open for adding any other manifests types in future.

WDYT?

thaJeztah

overall LGTM

I left one comment about sizes; wondering if you (and others) have thoughts on that. Also still a bit sad about the attestations bit (and we need to double-check how signing will work if we don't want the enum for "Kind" to become an unbounded list 🙈 )

api/swagger.yaml

thaJeztah · 2024-07-30T11:51:36Z

api/swagger.yaml

+      AvailableContentSize:
+        description: |
+          The size of the available distributable (possibly compressed) image content
+          in bytes.
+        type: "integer"


~~This does not include the unpacked content, correct?~~ never mind; that's indeed further down.

One thing I was considering; would it make sense to combine these? Thinking;

Size - packed - unpacked

With the potential to add (e.g.) size of the manifest itself.

My thinking there is that it could (possibly) allow formatting "size" as either a combined output (including a total), or as detailed, but encapsulate all of that in a combined struct about "size".

I think it's fine, but that would have to go into ImageData:

The top "manifest" level shouldn't do too much interpretation. That's why there's only raw content size.

The unpacked size doesn't make sense for all manifests - attestations can't be unpacked.

With the potential to add (e.g.) size of the manifest itself.

Manifest size is already available in the Descriptor.

Grouped under Size but actually didn't add the packed one - it would be the same as AvailableContentSize.

Added a Size group to the top-level manifest that contains:

Content - size of the locally available content in the content store

Total - total size of the content AND all the kind-specific sizes (Total-Content = size remaining in the XData struct).

Does that LGTY?

@thaJeztah PTAL

thaJeztah · 2024-07-30T11:59:44Z

api/types/image/manifest.go

+	// Present only if Kind == ImageManifestKindAttestation.
+	AttestationData AttestationProperties `json:"AttestationData,omitempty"`


I'm also somewhat preparing for other things like signing etc.

Note to self; we need to check what plans are in that area, and how such information would be made available.

thaJeztah · 2024-07-30T12:02:51Z

api/types/image/manifest.go

+	// Platform is the OCI platform object describing the platform of
+	//
+	// Required: true
+	Platform ocispec.Platform `json:"Platform"`


Ugh. Forgot about those 😞

ISTR there was still a discussion ongoing about some of that (i.e., badly built cross-compiled images that may have non-matching platforms in both). I'd have to dig back where that was.

thaJeztah · 2024-07-30T12:06:49Z

api/swagger.yaml

+              The number of containers that are using this specific platform image.
+            type: "integer"
+            format: "int64"
+            example: 2


Just slightly worried about list becoming an inspect 😅.

Yeah, I can see where you're coming from. I think it may be fine to use a list for this here. Or at least, it makes the content probably a lot more useful than just a number 🤔 (but happy to discuss)

api/types/image/manifest.go

integration/image/list_test.go

thaJeztah

Thanks! This starts to look good; I left some suggestions on naming, and some other comments; happy to discuss

api/swagger.yaml

api/types/image/manifest.go

api/types/image/summary.go

client/image_list.go

thaJeztah · 2024-08-07T09:51:18Z

daemon/containerd/image_list.go

+				"digest":   target.Digest,
+				"isPseudo": isPseudo,
+			}).Debug("pseudo image check failed")
+			return nil


I notice that here we're returning nil, but for the contentSize, err := img.Size(ctx) error we're logging and continuing; is that intentional?

Perhaps this line may need a comment why we don't (or can't) continue in this case. And why we can continue if we get a NotFound error

Yes, added a comment. Also the error checking below should also be ignored (so we don't abort the whole list operation just in case there's some issue with one image).

thaJeztah · 2024-08-07T09:56:37Z

daemon/containerd/image_list.go

+		if isPseudo {
+			if img.IsAttestation() {


Perhaps not for this PR, but perhaps some of these may be good to extract to a separate function at some point. I think it may make the flow clearer as there's some early returns happening that are easy to miss because they're a bit buried in the code to construct the types.

Basically thinking along the lines of

determine the Kind

calling function for Kind to propagate the relevant data

Yep, it's a bit complex at this point, but splitting it into functions isn't great either as there's a lot of shared state (totalSize, allChainsIDs, containersCount, best).

I can extract it to a separate struct in a follow up (or later, after we decide to settle for this approach).

Yup indeed. Perhaps there's not better solution, and definitely OK with me to look at in follow-ups

I'll definitely try to improve it, but in this case we might need to sacrifice some readability over performance because it's a hot path 😞

vvoland · 2024-08-07T11:22:23Z

api/types/image/manifest.go

+		// Content is the size (in bytes) of all the locally present
+		// content in the content store (e.g. image config, layers)
+		// referenced by this manifest and its children.
+		// This only includes blobs in the content store.
+		Content int64 `json:"Content"`
+
+		// Total is the total size (in bytes) of all the locally present
+		// data (both distributable and non-distributable) that's related to
+		// this manifest and its children.
+		// This equal to the sum of [Content] size AND all the sizes in the
+		// [Size] struct present in the Kind-specific data struct.
+		// For example, for an image kind (Kind == ManifestKindImage),
+		// this would include the size of the image content and unpacked
+		// image snapshots ([Size.Content] + [ImageData.Size.Unpacked]).
+		Total int64 `json:"Total"`


Wondering if these should be nilable in cases where we fail to determine the size? So we can differentiate between 0 and 🤷🏻 .

Oh, that's a good one; I know we used -1 for other situations, but those were -1 means "we didn't check".

Using omitempty also has its own downsides of course (w.r.t. other tools trying to use the JSON as-is) 🤔

Would we have cases where "one of these" failed? Or would they more likely "both" fail?

(slightly considering if omitting all of Size would work for that), but .. tricky).

We could also consider adding a Warnings[] array somewhere for non-fatal, but still useful failures

-1 is a bit meh to me, as it's really easy to get unnoticed, especially if you only add/subtract it.
Yeah I'd be against making it omitempty actually - not sure how other languages could handle it, but in pure JSON I think it's fine to just use explicit null instead of having us use the omitempty?

Yeah the Warnings would be nice too IMO.

LOL, we'll just make it a string and put "NaN" in it. Problem solved

thaJeztah

"LGTM"

I left some last few nits/notes, and wasn't sure if you were planning to squash the last touch-up commits.

Otherwise, I think this one should be good to get in from my side ❤️

hack/generate-swagger-api.sh

thaJeztah · 2024-08-07T11:13:12Z

api/swagger.yaml

+  ImageManifestSummary:
+    description: |
+      ImageManifestSummary represents a summary of an image manifest.


I think we can either rename the definition here, or add a x-go-name: "ManifestSummary"

Went with the x-go-name

thaJeztah · 2024-08-07T11:18:09Z

api/swagger.yaml

+      Kind:
+        description: |
+          The kind of the manifest.
+        type: "string"
+        example: "image"
+        enum:
+          - "image"
+          - "attestation"


It's probably fine to touch-up in a follow-up, but if you're planning to still document these, I think we can either use a bullet-list, like this one (but that one should have a white-space before the list 😂)

moby/api/swagger.yaml

Lines 6484 to 6489 in 1dd102e

description: |

The published state of the volume.

* `pending-publish` The volume should be published to this node, but the call to the controller plugin to do so has not yet been successfully completed.

* `published` The volume is published successfully to the node.

* `pending-node-unpublish` The volume should be unpublished from the node, and the manager is awaiting confirmation from the worker that it has done so.

* `pending-controller-unpublish` The volume is successfully unpublished from the node, but has not yet been successfully unpublished on the controller.

Or even a small table, like

moby/api/swagger.yaml

Lines 4302 to 4310 in 1dd102e

node attribute | matches | example

---------------------|--------------------------------|-----------------------------------------------

`node.id` | Node ID | `node.id==2ivku8v2gvtg4`

`node.hostname` | Node hostname | `node.hostname!=node-2`

`node.role` | Node role (`manager`/`worker`) | `node.role==manager`

`node.platform.os` | Node operating system | `node.platform.os==windows`

`node.platform.arch` | Node architecture | `node.platform.arch==x86_64`

`node.labels` | User-defined node labels | `node.labels.security==high`

`engine.labels` | Docker Engine's labels | `engine.labels.operatingsystem==ubuntu-24.04`

thaJeztah · 2024-08-07T11:21:43Z

api/swagger.yaml

+          Platform:
+            $ref: "#/definitions/OCIPlatform"


Also fine for a follow-up, and I THINK this is a limitation of the options we have in swagger, but wondering if we can somehow document how this Platform differs from the Descriptor.Platform (one from the descriptor, the other from the image config).

Not sure if it's possible (I don't think swagger accepts combination of description and $ref

Yup, it's not rendered. However, I think it doesn't seem like it's an error to actually include the description?

I went ahead and added it regardless of whether it's rendered or not 😅

thaJeztah · 2024-08-07T11:22:50Z

api/swagger.yaml

+        properties:
+          Platform:


Also still contemplating if we want to add image ID (digest of the image-config) somewhere, but that one is probably fine for a follow-up discussion as well.

I don't think it has any use currently (without exposing the content store to allow to fetch it), so I think we can add it later if needed.

Our version of go-swagger doesn't handle the `omitempty` correctly for the new field. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

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

Use a helper from `ImageManifest` which reads the config instead. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

Add `Manifests` field to `ImageSummary` which exposes all image manifests (which includes other blobs using the image media type, like buildkit attestations). There's also a new `manifests` query field that needs to be set in order for the response to contain the new information. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

thaJeztah

LGTM

vvoland added this to the 27.0.0 milestone Mar 7, 2024

vvoland self-assigned this Mar 7, 2024

vvoland force-pushed the c8d-list-multiplatform branch 2 times, most recently from 9d68e75 to 3507b41 Compare March 7, 2024 19:56

vvoland force-pushed the c8d-list-multiplatform branch 4 times, most recently from fc27649 to f7c6a83 Compare April 4, 2024 14:18

vvoland mentioned this pull request Apr 4, 2024

image/list: Add --tree flag docker/cli#4982

Merged

vvoland changed the title ~~[WIP] c8d: Multi-platform image list~~ c8d: Multi-platform image list Apr 4, 2024

vvoland commented Apr 4, 2024

View reviewed changes

vvoland mentioned this pull request Apr 4, 2024

containerd integration: Platform selection #47680

Closed

18 tasks

vvoland force-pushed the c8d-list-multiplatform branch from f7c6a83 to ea31ce0 Compare April 4, 2024 15:34

vvoland modified the milestones: 26.1.0, 27.0.0 Apr 10, 2024

vvoland mentioned this pull request May 13, 2024

Dockerfile: Use upstream go-swagger #47827

Closed

vvoland force-pushed the c8d-list-multiplatform branch 3 times, most recently from 7a67112 to 5eb892a Compare May 14, 2024 12:49

AkihiroSuda mentioned this pull request May 18, 2024

nerdctl images displays the wrong image id containerd/nerdctl#3011

Open

vvoland force-pushed the c8d-list-multiplatform branch from 5eb892a to 1db8418 Compare May 20, 2024 14:54

vvoland force-pushed the c8d-list-multiplatform branch 2 times, most recently from e1e2554 to 3adc348 Compare June 5, 2024 08:09

cpuguy83 approved these changes Jul 29, 2024

View reviewed changes

vvoland force-pushed the c8d-list-multiplatform branch from 02ca88a to eab3e7b Compare July 30, 2024 12:00

thaJeztah reviewed Jul 30, 2024

View reviewed changes

vvoland force-pushed the c8d-list-multiplatform branch from eab3e7b to da1fa10 Compare July 30, 2024 13:47

vvoland requested a review from thaJeztah July 30, 2024 13:48

vvoland force-pushed the c8d-list-multiplatform branch 4 times, most recently from c05ec81 to 49851df Compare August 5, 2024 10:37

thaJeztah reviewed Aug 7, 2024

View reviewed changes

vvoland force-pushed the c8d-list-multiplatform branch 3 times, most recently from 9a4749b to 1b685f4 Compare August 7, 2024 11:15

vvoland commented Aug 7, 2024

View reviewed changes

thaJeztah approved these changes Aug 7, 2024

View reviewed changes

vvoland added 4 commits August 7, 2024 13:48

swagger: Disable ImageSummary model generation

efb3c50

Our version of go-swagger doesn't handle the `omitempty` correctly for the new field. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

api: Bump default version to 1.47

85e9102

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

c8d/list: Use ReadConfig helper

f8b638b

Use a helper from `ImageManifest` which reads the config instead. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the c8d-list-multiplatform branch from 1b685f4 to 050afe1 Compare August 7, 2024 11:49

thaJeztah approved these changes Aug 7, 2024

View reviewed changes

vvoland merged commit f3cf935 into moby:master Aug 8, 2024

This was referenced Aug 8, 2024

vendor: github.com/docker/docker master (f3cf9359bdf6) docker/cli#5330

Merged

c8d/list: Fix Total size calculation #48330

Merged

This was referenced Aug 16, 2024

c8d/list: Don't require opts.ContainerCount for manifest containers #48345

Merged

[27.x backport] c8d: Multi-platform image list #48346

Merged

[27.x backport] image/list: Add --tree flag docker/cli#5353

Merged

thaJeztah mentioned this pull request Oct 8, 2024

Return container count on ImageList #48607

Open

This was referenced Oct 20, 2024

api: GET /images/json: preserve original manifest order #48701

Merged

[27.x backport] api: GET /images/json: preserve original manifest order #48712

Merged

		// Present only if Kind == ImageManifestKindAttestation.
		AttestationData AttestationProperties `json:"AttestationData,omitempty"`

	description: \|
	The published state of the volume.
	* `pending-publish` The volume should be published to this node, but the call to the controller plugin to do so has not yet been successfully completed.
	* `published` The volume is published successfully to the node.
	* `pending-node-unpublish` The volume should be unpublished from the node, and the manager is awaiting confirmation from the worker that it has done so.
	* `pending-controller-unpublish` The volume is successfully unpublished from the node, but has not yet been successfully unpublished on the controller.

	node attribute \| matches \| example
	---------------------\|--------------------------------\|-----------------------------------------------
	`node.id` \| Node ID \| `node.id==2ivku8v2gvtg4`
	`node.hostname` \| Node hostname \| `node.hostname!=node-2`
	`node.role` \| Node role (`manager`/`worker`) \| `node.role==manager`
	`node.platform.os` \| Node operating system \| `node.platform.os==windows`
	`node.platform.arch` \| Node architecture \| `node.platform.arch==x86_64`
	`node.labels` \| User-defined node labels \| `node.labels.security==high`
	`engine.labels` \| Docker Engine's labels \| `engine.labels.operatingsystem==ubuntu-24.04`

Conversation

vvoland commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 commented May 15, 2024

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

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 Jul 30, 2024 • 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!

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!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

vvoland Aug 7, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland Aug 7, 2024 • 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!

thaJeztah left a comment

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 commented Mar 7, 2024 •

edited

Loading

vvoland Jul 30, 2024 •

edited

Loading

vvoland Aug 7, 2024 •

edited

Loading

vvoland Aug 7, 2024 •

edited

Loading