c8d/push: Support `--platform` switch #47679

vvoland · 2024-04-04T15:09:20Z

Add OCI platform fields as a parameters to the POST /images/{id}/push that allow to specify a single-platform manifest to be pushed instead of the whole image index.

- What I did

- How I did it

- How to verify it

- Description for the changelog

- containerd image store: `POST /images/{name}/push` now supports a `platform` parameters (JSON encoded OCI Platform type) that allows selecting a specific platform-manifest from the multi-platform image.

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

cpuguy83 · 2024-04-08T16:44:01Z

Noting what we discussed on zoom:

I think the platform option needs to support something like platform=default so the client doesn't have to know the platform of the daemon.
I think buildkit already has an option like this we can just reuse.

buildx also supports --platform=local which would be the platform of the client.

api/server/router/image/image_routes.go

vvoland · 2024-04-09T15:57:52Z

I added the local and remote special values.

remote is handled on the server (this PR), the local is handled in the CLI PR: docker/cli#4984.

api/server/router/image/image_routes.go

daemon/containerd/image_manifest.go

daemon/containerd/image_push.go

daemon/containerd/image_manifest.go

cpuguy83 · 2024-04-13T16:59:44Z

client/image_push.go

@@ -36,6 +37,14 @@ func (cli *Client) ImagePush(ctx context.Context, image string, options image.Pu
 }
 }

+ if options.Platform != "" {
+ if options.Platform == "local" {
+ query.Set("platform", platforms.DefaultString())


cc @tianon

More platform strings :)

oof, and generated in client code to pass to the daemon 😭

local in the client is not local to the daemon 😭

Made local mean the platform local to the client for consistency with buildx --platform local.

That's why there's also remote (although I think I'd prefer to call it host) that is handled on the daemon side and defaults to the daemon platform:

moby/api/server/router/image/image_routes.go

Lines 554 to 559 in dfe5a6c

func parsePlatform(platformStr string) (ocispec.Platform, error) {

if platformStr == "remote" {

return platforms.DefaultSpec(), nil

}

return platforms.Parse(platformStr)

}

Happy to hear your suggestions if you have better ideas 😄

Definitely creeping into "build" vs "host" vs "target" confusion territory here -- what's the use case for building for the client-local host's platform when talking to a different daemon?

(Some of the context for this discussion is containerd/platforms#6, which actually makes platforms.DefaultString() unsafe to pass across process/machine boundaries like this, but I think the issue here is larger.)

Probably because I'd mentioned it in an earlier review, but more "the client shouldn't have to know the daemon's platform... also should we do the buildx thing for client platform?"
It may well be not needed.

what's the use case for building for the client-local host's platform when talking to a different daemon?

Personally, --platform local was useful to me for extracting binaries out of docker images to use on my host system, like:

echo 'FROM moby/moby-bin:master' | docker buildx build --platform local -o type=local,dest=asdadsad -

So, mostly thinking of remote daemon or, a more down-to-earth case: DD on non-Linux platforms - in the above case I really don't care about the VM, I just want to get the image for my host.

However, local might be less useful than the remote/host/daemon for the interactive case, because most of the time I know what platform my client host is running on. Still, it could be useful in scripts.

Also, in case of a remote Docker host - users might want to push the image for the platform of their host:

$ docker -c beef tag <some-image> myregistry.local/some-image $ docker -c beef push --platform local myregistry.local/some-image $ docker run myregistry.local/some-image

But still, I think that's mostly useful for scripts, as in the interactive case it's highly likely that user would pass the platform explicitly anyway.

Anyway, I'm fine with dropping these for now. We can always add them later if there's a real demand for it.

I would advise against using a single string field to represent the platform in the API. The first issue is highlighted here that it is too tempting to override it with special values. The second issue is that the string format is now a part of the API which it is not intended for. In containerd we only use the string for CLI or config, but the API always uses the structured form (https://github.com/containerd/containerd/blob/main/api/types/platform.proto) or its embedded in json in the original form (https://github.com/opencontainers/image-spec/blob/main/specs-go/v1/descriptor.go#L53).

The point of this change is to make the API endpoint behavior more explicit which filling in a variable "remote" value doesn't really accomplish. The way the current API behaves is already "--platform remote" and I worry by adding this flag here we end up changing the API to default to only push all platforms and end up recommending users to add "--platform remote". I think its best to keep the default to push whatever what pulled for that same image, so the equivalent of "--platform remote" if only a single platform was pulled or push all platforms if we can support pull all platforms. The case of pushing a single platform which differs from the remote after pulling multiple platforms seems like an edge case and is likely better handled with a separate command to edit an image with a new manifest.

Removed the single string and added parameters that correspond to the fields of ocispec.Platform.

cpuguy83

Can we get an integration test for this?

vvoland · 2024-05-10T11:58:53Z

I added unit tests for the descriptor candidate selection code. Also tweaked the default behavior (no explicit platform selection) to choose a single-platform manifest in some cases.

daemon/containerd/image_push_test.go

vvoland · 2024-05-14T11:35:30Z

api/server/httputils/form.go

+ p.OSVersion = form.Get("osversion")
+ p.OSFeatures = form["osfeatures"]


Wondering if we should include these... They seem to be ignored by default containerd matcher though: https://github.com/containerd/platforms/blob/c1438e911ac7596426105350652fe267d0fb8a03/platforms.go#L154-L159

On the other hand, I can image an index having multiple images for different OSVersions/Features and user wanting to push only a selected one.

So IMO, we should either:

Drop these parameters from the API

Use a custom platform matcher that also checks these

WDYT?

Would it be horrible to use the canonical JSON representation in a single field instead of multiple fields?

IMO, yes, because we pass the parameters by encoding them in url query, not as a json-encoded body 🙈

That doesn't really solve the OSVersion and OSFeatures issue though :P

Could we use the string serialisation as was defined in containerd/platforms#6 ? (at least that would keep the option open to pass multiple as well I guess

I would not be against JSON in url-encoding though if parsing that makes things less ambiguous (we already do this for filters, which admitted are a bit ugly, but in the end, API is not for humans)

I think we specifically wanted to not use that string format in the API.

That was my first implementation, see: #47679 (comment)

Changed to JSON encoding

vvoland · 2024-05-20T13:27:22Z

Current state:

If docker pull would also pull manifests for all platforms, pushing a full index (without all data locally) would also just work with the cross-repo mount:

And would fallback to the single-manifest if cross-repo mounts are not possible:

daemon/containerd/image_push.go

This adds the common helper functions used by the recent multiplatform-related PRs. Signed-off-by: Paweł Gronowski <[email protected]>

Translate os.ErrNotExist into cerrdefs.ErrNotExists Signed-off-by: Paweł Gronowski <[email protected]>

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

Add utility that allows to construct an image with the specified platforms. Signed-off-by: Paweł Gronowski <[email protected]>

Add a OCI platform fields as parameters to the `POST /images/{id}/push` that allow to specify a specific-platform manifest to be pushed instead of the whole image index. Signed-off-by: Paweł Gronowski <[email protected]>

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

When no platform was requested and pushing whole index failed, fallback to pushing a platform-specific manifest with a best candidate (if it's possible to choose one). Signed-off-by: Paweł Gronowski <[email protected]>

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

vvoland added area/api impact/api impact/changelog area/images area/ux labels Apr 4, 2024

vvoland self-assigned this Apr 4, 2024

This was referenced Apr 4, 2024

cli/push: Add platform switch docker/cli#4984

Open

WIP: containerd integration: Multi-platform images UX #47680

Open

vvoland force-pushed the c8d-multiplatform-push branch 2 times, most recently from 83357b3 to 489ca74 Compare April 4, 2024 18:58

vvoland marked this pull request as ready for review April 5, 2024 11:49

vvoland added this to the v-future milestone Apr 5, 2024

cpuguy83 reviewed Apr 8, 2024

View reviewed changes

api/server/router/image/image_routes.go Outdated Show resolved Hide resolved

vvoland force-pushed the c8d-multiplatform-push branch from 489ca74 to fc5fa64 Compare April 9, 2024 15:56

vvoland requested review from cpuguy83, rumpl and thaJeztah April 9, 2024 15:57

rumpl reviewed Apr 10, 2024

View reviewed changes

api/server/router/image/image_routes.go Outdated Show resolved Hide resolved

rumpl reviewed Apr 10, 2024

View reviewed changes

daemon/containerd/image_manifest.go Outdated Show resolved Hide resolved

rumpl reviewed Apr 10, 2024

View reviewed changes

daemon/containerd/image_push.go Outdated Show resolved Hide resolved

rumpl reviewed Apr 10, 2024

View reviewed changes

daemon/containerd/image_manifest.go Outdated Show resolved Hide resolved

vvoland force-pushed the c8d-multiplatform-push branch from fc5fa64 to dfe5a6c Compare April 12, 2024 11:10

cpuguy83 reviewed Apr 13, 2024

View reviewed changes

cpuguy83 requested changes Apr 13, 2024

View reviewed changes

cpuguy83 mentioned this pull request Apr 14, 2024

Docker push for single arch not work with containerd storage docker/cli#4959

Open

tianon mentioned this pull request Apr 30, 2024

Add format for platform string containerd/platforms#6

Merged

vvoland force-pushed the c8d-multiplatform-push branch from dfe5a6c to c63b42e Compare May 10, 2024 11:57

vvoland force-pushed the c8d-multiplatform-push branch from 9db1dc3 to e8d00ce Compare May 10, 2024 12:23

vvoland commented May 10, 2024

View reviewed changes

daemon/containerd/image_push_test.go Outdated Show resolved Hide resolved

vvoland requested review from cpuguy83, dmcgowan, tianon and rumpl May 10, 2024 12:38

vvoland force-pushed the c8d-multiplatform-push branch from e8d00ce to dea3651 Compare May 10, 2024 12:57

vvoland commented May 14, 2024

View reviewed changes

vvoland added the status/2-code-review label May 14, 2024

vvoland modified the milestones: v-future, 27.0.0 May 14, 2024

vvoland force-pushed the c8d-multiplatform-push branch 3 times, most recently from 9e5950a to 827f61a Compare May 20, 2024 13:20

dmcgowan reviewed May 21, 2024

View reviewed changes

daemon/containerd/image_push.go Outdated Show resolved Hide resolved

dmcgowan reviewed May 21, 2024

View reviewed changes

daemon/containerd/image_push.go Outdated Show resolved Hide resolved

dmcgowan reviewed May 21, 2024

View reviewed changes

daemon/containerd/image_push.go Outdated Show resolved Hide resolved

vvoland added 11 commits May 21, 2024 12:04

c8d/image_manifest: Add helper functions

042a22f

This adds the common helper functions used by the recent multiplatform-related PRs. Signed-off-by: Paweł Gronowski <[email protected]>

c8d/blobsDirContentStore: Return ErrNotExists

e194b19

Translate os.ErrNotExist into cerrdefs.ErrNotExists Signed-off-by: Paweł Gronowski <[email protected]>

c8d/ImageManifest: Cache image platform

959cc6d

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

testutils/specialimage: Add MultiPlatform

95cf5cc

Add utility that allows to construct an image with the specified platforms. Signed-off-by: Paweł Gronowski <[email protected]>

c8d/push: Support platform selection

b204ab6

Add a OCI platform fields as parameters to the `POST /images/{id}/push` that allow to specify a specific-platform manifest to be pushed instead of the whole image index. Signed-off-by: Paweł Gronowski <[email protected]>

api/push: Use JSON encoding for platform

a339647

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

c8d/push_test: Change removePlatforms into availablePlatforms

88db481

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

c8d/push_test: Move check below

5068be6

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

c8d/push: Retry with a single manifest if no platform requested

987f311

When no platform was requested and pushing whole index failed, fallback to pushing a platform-specific manifest with a best candidate (if it's possible to choose one). Signed-off-by: Paweł Gronowski <[email protected]>

c8d/push: Make sure the push candidate is different for retry

dccd836

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

c8d/push: Handle error for non-index types

bc0bbaf

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

vvoland force-pushed the c8d-multiplatform-push branch from 827f61a to bc0bbaf Compare May 21, 2024 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

c8d/push: Support `--platform` switch #47679

c8d/push: Support `--platform` switch #47679

vvoland commented Apr 4, 2024 •

edited

cpuguy83 commented Apr 8, 2024

vvoland commented Apr 9, 2024

cpuguy83 Apr 13, 2024

tianon Apr 15, 2024

vvoland Apr 16, 2024

tianon Apr 16, 2024

cpuguy83 Apr 16, 2024

vvoland Apr 17, 2024

vvoland Apr 17, 2024

dmcgowan Apr 30, 2024

vvoland May 10, 2024

cpuguy83 left a comment

vvoland commented May 10, 2024

vvoland May 14, 2024

tianon May 14, 2024

vvoland May 15, 2024 •

edited

thaJeztah May 16, 2024

thaJeztah May 16, 2024

cpuguy83 May 16, 2024

vvoland May 16, 2024

vvoland May 17, 2024

vvoland commented May 20, 2024 •

edited

	func parsePlatform(platformStr string) (ocispec.Platform, error) {
	if platformStr == "remote" {
	return platforms.DefaultSpec(), nil
	}
	return platforms.Parse(platformStr)
	}

		p.OSVersion = form.Get("osversion")
		p.OSFeatures = form["osfeatures"]

c8d/push: Support --platform switch #47679

Are you sure you want to change the base?

c8d/push: Support --platform switch #47679

Conversation

vvoland commented Apr 4, 2024 • edited

cpuguy83 commented Apr 8, 2024

vvoland commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

vvoland commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvoland May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvoland commented May 20, 2024 • edited

c8d/push: Support `--platform` switch #47679

c8d/push: Support `--platform` switch #47679

vvoland commented Apr 4, 2024 •

edited

vvoland May 15, 2024 •

edited

vvoland commented May 20, 2024 •

edited