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

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 <[email protected]>

Signed-off-by: Paweł Gronowski <[email protected]>

Use a helper from `ImageManifest` which reads the config instead. Signed-off-by: Paweł Gronowski <[email protected]>

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 <[email protected]>

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

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 <[email protected]>

api: Bump default version to 1.47

85e9102

Signed-off-by: Paweł Gronowski <[email protected]>

c8d/list: Use ReadConfig helper

f8b638b

Use a helper from `ImageManifest` which reads the config instead. Signed-off-by: Paweł Gronowski <[email protected]>

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

vvoland mentioned this pull request Aug 8, 2024

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

Merged

vvoland added the process/cherry-pick/27.2 label Aug 14, 2024

vvoland added process/cherry-picked and removed process/cherry-pick/27.2 labels Aug 16, 2024

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

	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!

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!

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!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vvoland commented Mar 7, 2024 •

edited

Loading

vvoland Aug 7, 2024 •

edited

Loading

vvoland Aug 7, 2024 •

edited

Loading