c8d: Add `platform` parameter to history, save and load by vvoland · Pull Request #48295 · moby/moby

vvoland · 2024-08-06T16:16:43Z

- What I did
Add the ability to select a specific platform for docker history, docker save and docker load.

- How I did it
See commits.

- How to verify it

- Description for the changelog

`GET /images/{name}/history` now supports a `platform` parameter (JSON encoded OCI Platform type) that allows to specify a platform to show the history of.
`POST /images/{name}/load` and `GET /images/{name}/get` now support a `platform` parameter (JSON encoded OCI Platform type) that allows to specify a platform to load/save. Not passing this parameter will result in loading/saving the full multi-platform image.

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

client/interface.go

vvoland · 2024-08-07T08:06:36Z

Unrelated failure, looks flaky, but haven't seen that one yet.

=== FAIL: github.com/docker/docker/container TestStateTimeoutWait (0.22s)
    state_test.go:147: Stop callback doesn't fire in 200 milliseconds

thaJeztah

Did a pass, but happy to hear your thoughts on some of them.

client/image_load_test.go

thaJeztah · 2024-09-09T10:07:50Z

client/image_load.go

+
+		p, err := json.Marshal(*opts.Platform)
+		if err != nil {
+			return image.LoadResponse{}, err


Should we return a errdefs error here? Also probably would be good to wrap the error as we did for the other one (invalid platform:)

Not sure - other usages of json's Marshal/Unmarshal/NewDecoder/NewDecoder in client package also return the raw error directly and don't wrap it in any way so I went with the same approach.

Seems fine not to wrap it.

Yeah, we could probably make a pass at typing those errors.

As to the wrapping; it was because I noticed that for ImageHistory we add a prefix;

return nil, fmt.Errorf("invalid platform: %v", err)

Without that, the error may not give a clue where the problem is (just a plain JSON marshal error).

thaJeztah · 2024-09-09T10:10:39Z

client/image_save.go

+
+		p, err := json.Marshal(*opts.Platform)
+		if err != nil {
+			return nil, fmt.Errorf("invalid platform: %v", err)


Same here for considering to use an errdefs.

☝️ Actually starting to wonder if it would make sense to have a utility for this; yes, it would be minimal (just json marshalling and handling errors), but would help producing the right errors and being consistent WDYT?

☝️ And I just realise we have httputils.DecodePlatform, so to some extent it would be nice if we had the reverse to that, but I guess httputils is part of api/server, so if we do, we should look where a suitable location would be if we want to put them together 🤔

same here for errdefs 😄

daemon/containerd/image_exporter.go

daemon/images/image_exporter.go

thaJeztah · 2024-09-09T10:20:32Z

daemon/images/image_exporter.go

+	if platform != nil {
+		return errdefs.NotImplemented(errors.New("platform parameter is not supported with the graphdriver image store"))
+	}


Same here

☝️ ❓ wondering now - could we somehow actually implement this? i.e. if a platform is provided and it matches then proceed - if it doesn't matches, then produce the same error as we do if we have multi-arch support (containerd image store) enabled?

Hmm actually we can. Updated.

integration/build/build_squash_test.go

integration/image/save_test.go

internal/testutils/specialimage/load.go

api/server/router/image/backend.go

laurazard

LGTM (left a couple nits, and +1 on @thaJeztah's comments)

api/swagger.yaml

cpuguy83 · 2024-09-10T19:56:13Z

image/tarexport/tarexport.go

+	lss             layer.Store
+	rs              refstore.Store
+	loggerImgEvent  LogImageEvent
+	platform        *platforms.Platform


Not sure platform is needed here since we have the matcher.

It's used in error message:

moby/image/tarexport/save.go

Line 171 in c25d20b

return errors.New("no suitable export target found for platform " + platforms.FormatAll(*l.platform))

Yeah, would be great if matchers would expose more information about what they're matching and/or perhaps return an error instead of only have a boolean.

api/common.go

thaJeztah

LGTM

some minor comments for consideration

api/swagger.yaml

thaJeztah · 2024-09-11T15:27:16Z

client/image_load.go

+
+		p, err := json.Marshal(*opts.Platform)
+		if err != nil {
+			return image.LoadResponse{}, err


Yeah, we could probably make a pass at typing those errors.

As to the wrapping; it was because I noticed that for ImageHistory we add a prefix;

return nil, fmt.Errorf("invalid platform: %v", err)

Without that, the error may not give a clue where the problem is (just a plain JSON marshal error).

thaJeztah · 2024-09-11T15:30:33Z

image/tarexport/save.go

+	"github.com/containerd/platforms"
+
 	"github.com/containerd/containerd/images"


🙈 stray empty line

Ugh, thanks! I really need to sort out my Goland settings 🙈

thaJeztah · 2024-09-11T15:32:42Z

image/tarexport/tarexport.go

+	lss             layer.Store
+	rs              refstore.Store
+	loggerImgEvent  LogImageEvent
+	platform        *platforms.Platform


Yeah, would be great if matchers would expose more information about what they're matching and/or perhaps return an error instead of only have a boolean.

Add `Platform` parameter that allows to select a specific platform to show the history for. This is a breaking change to the Go client as it changes the signature of `ImageHistory`. Signed-off-by: Paweł Gronowski <[email protected]>

Add `Platform` parameter that allows to select a specific platform to save/load. This is a breaking change to the Go client as it changes the signatures of `ImageLoad` and `ImageSave`. Signed-off-by: Paweł Gronowski <[email protected]>

Allows load to filter image manifests to load based on their platform. For save, verify that the image platform matches the requested platform, otherwise error out. Signed-off-by: Paweł Gronowski <[email protected]>

vvoland added status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/images Image Distribution containerd-integration Issues and PRs related to containerd integration labels Aug 6, 2024

vvoland added this to the 28.0.0 milestone Aug 6, 2024

vvoland self-assigned this Aug 6, 2024

vvoland commented Aug 6, 2024

View reviewed changes

client/interface.go Outdated Show resolved Hide resolved

vvoland force-pushed the c8d-saveload-platform branch 5 times, most recently from 7ae06b0 to 4d31b8c Compare August 8, 2024 11:02

vvoland requested review from laurazard and thaJeztah August 8, 2024 11:14

vvoland mentioned this pull request Aug 8, 2024

c8d: Add --platform flag to history, save and load docker/cli#5331

Merged

2 tasks

vvoland force-pushed the c8d-saveload-platform branch 4 times, most recently from f145088 to aba3908 Compare August 8, 2024 16:08

vvoland marked this pull request as draft August 8, 2024 16:09

vvoland mentioned this pull request Aug 21, 2024

api/swagger: Improve description for platform in images/push #48357

Merged

vvoland force-pushed the c8d-saveload-platform branch from aba3908 to 3275617 Compare August 23, 2024 13:34

vvoland added area/go-sdk impact/go-sdk Noteworthy (compatibility changes) in the Go SDK impact/changelog labels Aug 23, 2024

vvoland force-pushed the c8d-saveload-platform branch 4 times, most recently from 3d5c3ef to e5c6cb0 Compare August 23, 2024 15:11

vvoland force-pushed the c8d-saveload-platform branch from 033d0f0 to d0b8814 Compare September 6, 2024 14:05

vvoland marked this pull request as ready for review September 6, 2024 17:57

thaJeztah requested changes Sep 9, 2024

View reviewed changes

laurazard approved these changes Sep 9, 2024

View reviewed changes

api/swagger.yaml Outdated Show resolved Hide resolved

api/swagger.yaml Outdated Show resolved Hide resolved

vvoland force-pushed the c8d-saveload-platform branch from d0b8814 to c25d20b Compare September 9, 2024 16:31

cpuguy83 requested changes Sep 10, 2024

View reviewed changes

vvoland force-pushed the c8d-saveload-platform branch 2 times, most recently from 319064b to ead3ee3 Compare September 11, 2024 07:55

thaJeztah reviewed Sep 11, 2024

View reviewed changes

api/common.go Outdated Show resolved Hide resolved

vvoland force-pushed the c8d-saveload-platform branch from 2709581 to 0d1cff7 Compare September 11, 2024 10:32

thaJeztah approved these changes Sep 11, 2024

View reviewed changes

vvoland added 3 commits September 11, 2024 19:44

image/history: Support Platform parameter

ab075ec

Add `Platform` parameter that allows to select a specific platform to show the history for. This is a breaking change to the Go client as it changes the signature of `ImageHistory`. Signed-off-by: Paweł Gronowski <[email protected]>

image/save&load: Support Platform parameter

f143f4e

Add `Platform` parameter that allows to select a specific platform to save/load. This is a breaking change to the Go client as it changes the signatures of `ImageLoad` and `ImageSave`. Signed-off-by: Paweł Gronowski <[email protected]>

images: Support platform selection on graphdrivers

521fa83

Allows load to filter image manifests to load based on their platform. For save, verify that the image platform matches the requested platform, otherwise error out. Signed-off-by: Paweł Gronowski <[email protected]>

vvoland force-pushed the c8d-saveload-platform branch from 0d1cff7 to 521fa83 Compare September 11, 2024 17:44

thaJeztah requested a review from cpuguy83 September 12, 2024 03:48

cpuguy83 approved these changes Sep 12, 2024

View reviewed changes

thaJeztah merged commit 2936af4 into moby:master Sep 12, 2024

thaJeztah mentioned this pull request Sep 24, 2024

Docker Containerd snapshotter Overlayfs, docker save fails for image pulled using --platform=linux/arm64 on amd64 host. docker/cli#5476

Open

This was referenced Oct 7, 2024

api: postImagesLoad: fix API version for platform #48588

Merged

c8d: image load does not produce error if given platform doesn't exist #48589

Closed

c8d: ImageService.LoadImage: don't ignore missing platform when requested #48590

Closed

This was referenced Oct 25, 2024

c8d: update image save/load to accept multiple platforms #48759

Closed

c8d/load: Don't ignore missing platform when requested #48718

Merged

[27.x backport] c8d/load: Don't ignore missing platform when requested #48763

Closed

vvoland mentioned this pull request Nov 6, 2024

[27.x backport] api/swagger: Improve description for platform in images/push #48829

Merged

thaJeztah mentioned this pull request Nov 19, 2024

client: support multiple platforms on save and load #48902

Merged

vvoland mentioned this pull request Jan 27, 2025

containerd integration: Platform selection #47680

Closed

18 tasks

kristianheljas mentioned this pull request Apr 1, 2025

Add missing platform parameter to ImageGet api docs and add it under Image tag #49726

Merged

vvoland mentioned this pull request Sep 9, 2025

Unable to load docker containerd managed images to cluster kubernetes-sigs/kind#3795

Open

		"github.com/containerd/platforms"

		"github.com/containerd/containerd/images"

Conversation

vvoland commented Aug 6, 2024

Uh oh!

Uh oh!

vvoland commented Aug 7, 2024

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!

vvoland Sep 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vvoland Sep 9, 2024 •

edited

Loading