c8d: Add `--platform` flag to history, save and load #5331

vvoland · 2024-08-08T11:22:13Z

needs: c8d: Add platform parameter to history, save and load moby/moby#48295
needs: vendor: github.com/docker/docker 164cae56ed95 (master, v-next) #5428

Add --platform flag to history, save and load.

- Description for the changelog

containerd image store: `docker load` and `docker save` now support a `--platform` flag allowing to choose a specific single-platform image to load/save if the image is multi-platform
containerd image store: `docker history` now supports a `--platform` flag allowing to choose a specific platform to show the history for.

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

dvdksn · 2024-09-24T13:01:23Z

cli/command/image/history.go

+If the platform is not specified, the host platform is preferred if it's available, otherwise any available platform is used.
+
+Format: os[/arch[/variant]]
+Example: "linux/amd64"`)


How about including the full cmd?

Suggested change

Example: "linux/amd64"`)

Example: "docker image history --platform linux/amd64"`)

I (somewhat) assume that this was done to allow defining a global const for these flags and for them to be shared (similar to what we did for --format 🤔), but I need to check if they differ in other ways

dvdksn · 2024-09-24T13:04:39Z

cli/command/image/load.go

+		`Pick a single-platform to be loaded if the image is multi-platform.
+Full multi-platform image will be load if not specified.
+
+Format: os[/arch[/variant]]
+Example: "linux/amd64"`)


Suggested change

`Pick a single-platform to be loaded if the image is multi-platform.

Full multi-platform image will be load if not specified.

Format: os[/arch[/variant]]

Example: "linux/amd64"`)

`Specify a platform from a multi-platform image to load.

If a platform is not specified, and the image is a multi-platform image, all platforms are loaded.

Format: os[/arch[/variant]]

Example: "docker image load --platform linux/amd64"`)

dvdksn · 2024-09-24T13:05:49Z

cli/command/image/save.go

+		`Pick a single-platform to be saved if the image is multi-platform.
+Full multi-platform image will be saved if not specified.
+
+Format: os[/arch[/variant]]
+Example: "linux/amd64"`)


Suggested change

`Pick a single-platform to be saved if the image is multi-platform.

Full multi-platform image will be saved if not specified.

Format: os[/arch[/variant]]

Example: "linux/amd64"`)

`Specify a platform from a multi-platform image to save.

If a platform is not specified, and the image is a multi-platform image, all platform variants are saved.

Format: os[/arch[/variant]]

Example: "docker image save --platform linux/amd64"`)

codecov-commenter · 2024-09-24T14:34:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.67%. Comparing base (6a78e92) to head (d085e24).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5331      +/-   ##
==========================================
+ Coverage   60.63%   60.67%   +0.04%     
==========================================
  Files         345      345              
  Lines       23462    23488      +26     
==========================================
+ Hits        14226    14252      +26     
  Misses       8263     8263              
  Partials      973      973

thaJeztah · 2024-09-24T15:23:36Z

cli/command/image/history.go

+		`Specify a platform from a multi-platform image to show the history for.
+If the platform is not specified, the host platform is preferred if it's available, otherwise any available platform is used.


@dvdksn I applied your suggestions; I'm slightly wondering if we should / could condense the information a bit; for example;

Specify a platform -> Platform from a multi-platform image ...

(I don't think we generally add Specify .... as that's a bit implicit for options?)

The "format" could possibly be included in the first line, something like;

Platform from a multi-platform image to show the history for (format: "os[/arch[/variant]]") By default, the daemon's native platform is preferred, otherwise any available platform is used.

(wondering if the host platform is ambiguous, and if we need to emphasise daemon platform)

i also think we should probably change "any available platform is used" to something more precise.. what does "any" mean in this context? the first available platform in alphabetical order?

@thaJeztah your suggestion sgtm. Coming back to this I realize that multi-line flag descriptions are kinda exotic - we don't use them a lot. Maybe we should instead create sections in the respective .md files for the platform flag, like we usually do, to show examples.

IMO, if there is a specific behavior we've decide is part of the contract "i.e. it's always the first alphabetical one", then we should make that explicit. On the other hand, if we explicitly don't want to make any future guarantees about which platform is used, then any is fine. I honestly kind of would rather just return an error if there's no platform for the given image, but 🤷‍♀️ it's not a big deal either way, especially for history.

laurazard · 2024-10-04T12:11:20Z

I had a thought while looking at this again today – I think it would be really nice if we had a main .md file/doc with a general explanation about multi-platform images – i.e. "this is a new feature available on docker if the containerd store integration is enabled, it's kind of like the old images but now each image is actually an index and may contain multiple "images" for different platforms, etc.,". We could link there from a lot of other places, and we could also have a little table with the different commands that take a parameter to specify a platform if relevant, and I think it'd tie a lot of things together that we're kind of leaving implied/are spread out through a lot of different places/docs right now. Kind of like the multi-platform build doc for Build. cc @thaJeztah @dvdksn

dvdksn · 2024-10-04T18:32:24Z

@laurazard the multi-platform builds page has some of that. I agree it would be nice with a general "what's an image, actually" primer. We should probably consider OCI the typical format by now — buildx will default to them more often than not, and with containerd becoming the default storage, it feels like the transition is coming sooner rather than later. Single-platform manifests is a legacy format in my world.

thaJeztah · 2024-10-07T14:37:52Z

@dvdksn @laurazard I rewrote the PR to have a short description for the flags, and move the description to the docs.

There's still some issues/bugs to fix on the daemon side (as I found some issues while working on the docs; moby/moby#48589)

thaJeztah · 2024-10-07T16:00:06Z

Oh! Should mention that I didn't squash / cleanup commits yet; will do so before merging if this looks good.

krissetto

overall LGTM, just left a few nits in the long-form docs

krissetto · 2024-10-08T13:41:10Z

docs/reference/commandline/image_history.md

+The `--platform` option allows you to specify which platform-variant to show
+history for if multiple platforms are present. By default, `docker history`
+shows the history for the daemon's native platform or if not present, the
+first available platform.


the first available platform found in alphabetical order?

Yeah, I think this relates a bit to the other comments above; I kept it somewhat "on the surface here" because I also didn't want to document it as a strict behavior. We need to define some of that in more details; "first available" may also be "best match", which may not be "alphabetical", i.e., if amd64 and arm64 are present, this should pick arm64 as a default (on arm), and amd64 (on x86).

So perhaps even "first available" is wrong already.

Sounds good to me if we want to define this a bit better somewhere else (or leave the road open to a better definition at a later point, without adding to user expectations)

krissetto · 2024-10-08T13:41:47Z

docs/reference/commandline/image_history.md

+`--platform` option selects which variant to show the history for. An error
+is produced if the given platform is not present in the local image cache.
+
+The platform option takes the `os[/arch[/variant]]` format, for example,


nit

Suggested change

The platform option takes the `os[/arch[/variant]]` format, for example,

The platform option takes the `os[/arch[/variant]]` format, for example

Let me check with the grammar police (@dvdksn 🙏 ), but I think it's recommended to always follow for example with a comma. Perhaps it should start with a new sentence here though (format. For example, ...)

the word man! anyway these were just my 2 cents, so feel free to merge whenever 😄

And, answering myself, because I noticed I copy/pasted parts of the flag descriptions and used e.g. without a comma; https://english.stackexchange.com/a/16173

E.g. (exempli gratia in Latin, meaning “for example”) should be generally followed by a list of examples. Thus, adhering to proper English style usually requires commas to follow e.g. to delimit the beginning of that list.

If the "For example" starts the sentence, I agree with having the comma after it. As is, when reading the sentence it feel strange to pause both before and after "for example", because it makes it sound like the example came before (causing some confusion, at least to myself 😄).

Definitely minutia though

Yeah; these are hard. I basically tried to avoid ending up with "3 word sentences" because that also felt a bit silly (which is why I used the comma before example). I updated to use a semicolon instead, but not sure if that's 100% grammatically correct either 🥹

krissetto · 2024-10-08T13:48:01Z

docs/reference/commandline/image_history.md

+The following examples pulls an `alpine` image for the daemon's native platform,
+and shows the history, then provides a custom platform, which fails until the
+given platform-variant is pulled;


nit:

Suggested change

The following examples pulls an `alpine` image for the daemon's native platform,

and shows the history, then provides a custom platform, which fails until the

given platform-variant is pulled;

The following example:

- pulls an `alpine` image for the daemon's native platform (because `--platform` is omitted);

- shows the history (also for the default platform, because `--platform` is omitted);

- tries to show the history for the image while providing a custom `--platform` value, which fails until the given platform-variant is pulled;

Oh! Forgot to reply on these; I've been going back and forth a bit if this approach would be making things better (much more in depth) or if it would be "too much".

Happy to hear from others though. Also happy to make changes along these lines in a follow up.

docs/reference/commandline/image_load.md

docs/reference/commandline/image_save.md

krissetto · 2024-10-08T13:53:25Z

docs/reference/commandline/image_save.md

+The following examples pulls an `alpine` image for the daemon's native platform,
+then provides a custom platform, which fails until the given platform-variant
+is pulled;


nit:

Suggested change

The following examples pulls an `alpine` image for the daemon's native platform,

then provides a custom platform, which fails until the given platform-variant

is pulled;

The following example:

- pulls an `alpine` image for the daemon's native platform (because `--platform` is omitted);

- tries to save the image while providing a custom `--platform` value, which fails until the given platform-variant is pulled;

thaJeztah · 2024-10-08T15:48:12Z

let me start cleaning up the commit history on this one to bring it in a mergable state

thaJeztah · 2024-10-08T16:45:58Z

cli/command/image/load.go

+			_, _ = fmt.Fprintf(dockerCli.Err(), "Invalid platform %s", opts.platform)
+			return err


I just realised that this isn't correct; it tries to both print something as well as returning an error. The printed message is without a newline. But also, because the error is returned, both the printed message and the error get printed;

docker image save -o image-amd64.tar --platform in:valid/foo:bar alpine:latest Invalid platform in:valid/foo:bar"in:valid" is an invalid OS component of "in:valid/foo:bar": OSAndVersion specifier component must match "^([A-Za-z0-9_-]+)(?:\$([A-Za-z0-9_.-]*)\$)?$": invalid argument

I wondered if this was perhaps done for cases where no output file is specified (so the tar output would be redirected), but the same happens in that case;

docker image save --platform in:valid/foo:bar alpine:latest > file.tar Invalid platform in:valid/foo:bar"in:valid" is an invalid OS component of "in:valid/foo:bar": OSAndVersion specifier component must match "^([A-Za-z0-9_-]+)(?:\$([A-Za-z0-9_.-]*)\$)?$": invalid argument

So I think this should just wrap the error and return it.

Updated these to just return the error

thaJeztah · 2024-10-08T17:47:51Z

LOL, and there I thought "let me add some basic unit-tests" and scratching my head "why doesn't it fail??"

cli/command/image: fix TestNewSaveCommandSuccess to actually test #5520

thaJeztah · 2024-10-08T18:38:53Z

OK; this should probably be ready for review / merge now;

Squashed / cleaned up commits
Changed the fmt.FPrint to just return the error instead; c8d: Add --platform flag to history, save and load #5331 (comment)
Added some basic unit tests

docs/reference/commandline/image_history.md

docs/reference/commandline/image_load.md

docs/reference/commandline/image_save.md

laurazard · 2024-10-10T12:42:53Z

Oops, meant to comment/hit the other button. Just had found some typos/suggestions, otherwise it looks good.

thaJeztah · 2024-10-10T12:59:53Z

Updated! I think I got them all, @laurazard

laurazard

LGTM

docs/reference/commandline/image_save.md

docs/reference/commandline/image_load.md

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

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

dvdksn

LGTM 🚀

vvoland added impact/changelog area/api area/ux containerd-integration Issues and PRs related to containerd integration labels Aug 8, 2024

vvoland added this to the 28.0.0 milestone Aug 8, 2024

vvoland requested review from a team and dvdksn August 8, 2024 11:22

vvoland force-pushed the c8d-saveload-platform branch from 2ee583e to b3139f5 Compare August 8, 2024 13:45

vvoland self-assigned this Sep 6, 2024

thaJeztah mentioned this pull request Sep 12, 2024

vendor: github.com/docker/docker 164cae56ed95 (master, v-next) #5428

Merged

thaJeztah force-pushed the c8d-saveload-platform branch from b3139f5 to 8f20d2e Compare September 24, 2024 11:48

dvdksn reviewed Sep 24, 2024

View reviewed changes

thaJeztah force-pushed the c8d-saveload-platform branch from 8f20d2e to 400f5f0 Compare September 24, 2024 14:32

thaJeztah force-pushed the c8d-saveload-platform branch from 400f5f0 to 8ebf4cf Compare September 24, 2024 15:19

thaJeztah marked this pull request as ready for review September 24, 2024 15:19

thaJeztah requested a review from a team as a code owner September 24, 2024 15:19

thaJeztah reviewed Sep 24, 2024

View reviewed changes

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. #5476

Open

dvdksn mentioned this pull request Sep 27, 2024

add Docker Desktop 4.35.0 release notes docker/docs#20909

Merged

thaJeztah force-pushed the c8d-saveload-platform branch from 8ebf4cf to 3b97d52 Compare October 4, 2024 11:53

This was referenced Oct 7, 2024

api: postImagesLoad: fix API version for platform moby/moby#48588

Merged

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

Open

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

Closed

thaJeztah force-pushed the c8d-saveload-platform branch from 3b97d52 to 7731583 Compare October 7, 2024 13:37

thaJeztah mentioned this pull request Oct 8, 2024

add shell-completion for --platform flags #5516

Merged

thaJeztah force-pushed the c8d-saveload-platform branch from 7731583 to 5412e96 Compare October 8, 2024 13:36

krissetto approved these changes Oct 8, 2024

View reviewed changes

thaJeztah force-pushed the c8d-saveload-platform branch from 5412e96 to d30d61a Compare October 8, 2024 14:48

Benehiko approved these changes Oct 8, 2024

View reviewed changes

thaJeztah force-pushed the c8d-saveload-platform branch from d30d61a to c656e2e Compare October 8, 2024 14:54

thaJeztah reviewed Oct 8, 2024

View reviewed changes

thaJeztah force-pushed the c8d-saveload-platform branch from c656e2e to c7cc278 Compare October 8, 2024 18:37

Benehiko approved these changes Oct 9, 2024

View reviewed changes

thaJeztah mentioned this pull request Oct 9, 2024

With containerd, I pulled images debian, ubuntu, nginx all latest, using --platform=linux/arm64/v8 and multiple strange observations. #5523

Open

laurazard requested changes Oct 10, 2024

View reviewed changes

thaJeztah force-pushed the c8d-saveload-platform branch from c7cc278 to 6acec20 Compare October 10, 2024 12:59

laurazard approved these changes Oct 10, 2024

View reviewed changes

dvdksn reviewed Oct 10, 2024

View reviewed changes

docs/reference/commandline/image_save.md Outdated Show resolved Hide resolved

docs/reference/commandline/image_save.md Outdated Show resolved Hide resolved

docs/reference/commandline/image_load.md Outdated Show resolved Hide resolved

vvoland added 3 commits October 10, 2024 16:35

image/save: Add --platform

a20eb45

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

image/load: Add --platform

b0bb4ba

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

image/history: Add --platform flag

d085e24

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

thaJeztah force-pushed the c8d-saveload-platform branch from 6acec20 to d085e24 Compare October 10, 2024 15:00

dvdksn approved these changes Oct 10, 2024

View reviewed changes

thaJeztah merged commit f483aac into docker:master Oct 10, 2024
89 checks passed

thaJeztah mentioned this pull request Oct 10, 2024

Touch-up some errors for missing platforms moby/moby#48631

Merged

vvoland mentioned this pull request Oct 18, 2024

[27.x backport] Touch-up some errors for missing platforms moby/moby#48691

Merged

This was referenced Oct 25, 2024

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

Merged

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

Closed

	Example: "linux/amd64"`)
	Example: "docker image history --platform linux/amd64"`)

		`Specify a platform from a multi-platform image to show the history for.
		If the platform is not specified, the host platform is preferred if it's available, otherwise any available platform is used.

	The platform option takes the `os[/arch[/variant]]` format, for example,
	The platform option takes the `os[/arch[/variant]]` format, for example

-The following examples pulls an `alpine` image for the daemon's native platform,
-and shows the history, then provides a custom platform, which fails until the
-given platform-variant is pulled;
+The following example:
+- pulls an `alpine` image for the daemon's native platform (because `--platform` is omitted);
+- shows the history (also for the default platform, because `--platform` is omitted);
+- tries to show the history for the image while providing a custom `--platform` value, which fails until the given platform-variant is pulled;

		_, _ = fmt.Fprintf(dockerCli.Err(), "Invalid platform %s", opts.platform)
		return err

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

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

Conversation

vvoland commented Aug 8, 2024 • edited by thaJeztah Loading

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

codecov-commenter commented Sep 24, 2024 • edited Loading

Codecov Report

thaJeztah Sep 24, 2024 • edited Loading

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

laurazard commented Oct 4, 2024 • edited Loading

dvdksn commented Oct 4, 2024

thaJeztah commented Oct 7, 2024

thaJeztah commented Oct 7, 2024

krissetto left a 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

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

thaJeztah commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Oct 8, 2024

thaJeztah commented Oct 8, 2024

laurazard commented Oct 10, 2024

thaJeztah commented Oct 10, 2024

laurazard left a comment

Choose a reason for hiding this comment

dvdksn left a comment

Choose a reason for hiding this comment

c8d: Add `--platform` flag to history, save and load #5331

c8d: Add `--platform` flag to history, save and load #5331

vvoland commented Aug 8, 2024 •

edited by thaJeztah

Loading

codecov-commenter commented Sep 24, 2024 •

edited

Loading

thaJeztah Sep 24, 2024 •

edited

Loading

laurazard commented Oct 4, 2024 •

edited

Loading