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 `os`, `architecture`, `variant`, `osversion`, `osfeatures` parameters (fields of OCI Platform) that allow 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?

This adds the common helper functions used by the recent multiplatform-related PRs. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

Translate os.ErrNotExist into cerrdefs.ErrNotExists Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

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

Add utility that allows to construct an image with the specified platforms. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

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.

vvoland · 2024-05-10T12:28:51Z

daemon/containerd/image_push_test.go

+		{
+			name:  "none requested on v7 daemon, v7 not available",
+			check: singleManifestSelected(linuxArmv5), // Should it fail, because v5 can't be pushed?
+
+			indexPlatforms:  []platforms.Platform{linuxArm64, linuxArmv7, linuxArmv5},
+			deletePlatforms: []ocispec.Platform{linuxArmv7},
+			daemonPlatform:  &linuxArmv7,
+			requestPlatform: nil,
+		},


It's not immediately obvious to me what this is testing/emulating, sorry 😅

Isn't linuxArmv7 being in indexPlatforms meaning that the image does support v7? What is deletePlatforms?

deletePlatforms are the platforms that are missing locally. So the case is:

Image index supports: linux/arm64, linux/arm/v5 and linux/arm/v7.

Daemon is running on: linux/arm/v7

User has pulled: linux/arm64, linux/arm/v5 (image index minus the deletePlatforms).

Should the arm/v5 be pushed (as the daemon can run this image), even though it's not the native variant which the original image supports?

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 <pawel.gronowski@docker.com>

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

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 added 2 commits May 10, 2024 13:56

c8d/image_manifest: Add helper functions

58b7e64

This adds the common helper functions used by the recent multiplatform-related PRs. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

c8d/blobsDirContentStore: Return ErrNotExists

3275679

Translate os.ErrNotExist into cerrdefs.ErrNotExists Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland added 2 commits May 10, 2024 13:56

c8d/ImageManifest: Cache image platform

8320045

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

testutils/specialimage: Add MultiPlatform

a281060

Add utility that allows to construct an image with the specified platforms. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

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 2 times, most recently from 9db1dc3 to e8d00ce Compare May 10, 2024 12:23

vvoland commented May 10, 2024

View reviewed changes

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

c8d/push: Support platform selection

dea3651

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 <pawel.gronowski@docker.com>

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

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 10, 2024

tianon May 14, 2024

vvoland May 15, 2024

vvoland May 14, 2024

tianon May 14, 2024

vvoland May 15, 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

Choose a reason for hiding this comment

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

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

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

vvoland commented Apr 4, 2024 •

edited

vvoland May 15, 2024 •

edited