LCOW: API: Add `platform` to /images/create and /build by lowenna · Pull Request #34642 · moby/moby

lowenna · 2017-08-27T00:56:32Z

Signed-off-by: John Howard jhoward@microsoft.com

This PR has the API changes described in #34617.

Update 9/13 based on feedback:
Specifically, it adds an HTTP header "X-Requested-Platform" which is a JSON-encoded
OCI Image-spec Platform structure to /images/create and /build
Specifically, it adds a query parameter which is a simple string such as platform=linux/amd64 to /images/create and /build.

In addition, it renames (almost all) uses of the string variable platform (and associated
methods/functions) to os, or operatingSystem where it clashes with the os import. This makes it much clearer to disambiguate with the swarm "platform" which is really os/arch. This is a stepping stone to getting the daemon towards fully multi-platform/arch-aware, and makes it clear when "operating system" is being referred to rather than "platform" which is misleadingly used - sometimes in the swarm
meaning, but more often as just the operating system.

lowenna · 2017-08-27T01:18:32Z

This is what the behaviour with this change (and a CLI updated to support the --platform flag) looks like:

This is where the image does not exist locally, and no platform flag has been passed. The engine will default to the host OS which is Windows, but no Windows busybox exists in the registry.

PS E:\> docker run --rm busybox echo hello
Unable to find image 'busybox:latest' locally
latest: Pulling from library/busybox
e:\go\src\github.com\docker\cli\binary\docker.exe: cannot download image with operating system "linux" when requesting "windows".
See 'e:\go\src\github.com\docker\cli\binary\docker.exe run --help'.
PS E:\>

Now doing the same adding the CLI/API flag. This succeeds.

PS E:\> docker run --rm --platform=linux busybox echo hello
Unable to find image 'busybox:latest' locally
latest: Pulling from library/busybox
add3ddb21ede: Pull complete
Digest: sha256:b82b5740006c1ab823596d2c07f081084ecdb32fd258072707b99f52a3cb8692
Status: Downloaded newer image for busybox:latest
hello

Now the image exists locally, there's no need to pass the API/CLI flag to run an image based off it:

PS E:\> docker run --rm busybox echo hello
hello

Delete the image for the next step

PS E:\> docker rmi busybox
Untagged: busybox:latest
Untagged: busybox@sha256:b82b5740006c1ab823596d2c07f081084ecdb32fd258072707b99f52a3cb8692
Deleted: sha256:d20ae45477cbc89863fff11d01cdccf28e4ff06ce2eb2f0206ef971b14eaf6c0
Deleted: sha256:6a749002dd6a65988a6696ca4d0c4cbe87145df74e3bf6feae4025ab28f420f2

Attempt to pull without an API/CLI flag. This also fails as there is no busybox image for Windows in the registry

PS E:\> docker pull busybox
Using default tag: latest
latest: Pulling from library/busybox
cannot download image with operating system "linux" when requesting "windows"

So adding the flag

PS E:\> docker pull --platform=linux busybox
Using default tag: latest
latest: Pulling from library/busybox
add3ddb21ede: Pull complete
Digest: sha256:b82b5740006c1ab823596d2c07f081084ecdb32fd258072707b99f52a3cb8692
Status: Downloaded newer image for busybox:latest

And the same for create. busybox doesn't exist locally first. So the first one will fail:

PS E:\docker\build\lcow> docker create busybox /bin/echo hello
Unable to find image 'busybox:latest' locally
latest: Pulling from library/busybox
cannot download image with operating system "linux" when requesting "windows"

So add the CLI/API flag:

PS E:\docker\build\lcow> docker create --platform=linux/amd64 busybox /bin/echo hello
Unable to find image 'busybox:latest' locally
latest: Pulling from library/busybox
Digest: sha256:b82b5740006c1ab823596d2c07f081084ecdb32fd258072707b99f52a3cb8692
Status: Downloaded newer image for busybox:latest
6cd0ad51750d3bf72eea042cfb10fe7335f633a7108324bebd8a7b82cd3a0c1d

Start a container from the new image

PS E:\docker\build\lcow> docker start 6
6

And query the output

PS E:\docker\build\lcow> docker logs 6cd
hello
PS E:\docker\build\lcow>

lowenna · 2017-08-27T01:37:04Z

And showing the same build step from #34401, which this PR replaces, with the API changes, demonstrated via the CLI:

PS E:\docker\build\lcow> docker build --no-cache -f test3 --platform=linux .
Sending build context to Docker daemon  19.93MB
Step 1/3 : FROM busybox
latest: Pulling from library/busybox
add3ddb21ede: Pull complete
Digest: sha256:b82b5740006c1ab823596d2c07f081084ecdb32fd258072707b99f52a3cb8692
Status: Downloaded newer image for busybox:latest
 ---> d20ae45477cb
Step 2/3 : RUN mkdir /foo
 ---> Running in b83d7de0c946
 ---> 4755c8944ab4
Removing intermediate container b83d7de0c946
Step 3/3 : RUN ls -l /
 ---> Running in 5458cc315cd3
total 36
drwxr-xr-x    2 root     root         12288 Aug 23 00:00 bin
drwxr-xr-x    5 root     root           340 Aug 27 01:34 dev
drwxr-xr-x    1 root     root            60 Aug 27 01:34 etc
drwxr-xr-x    2 root     root          4096 Aug 27 01:34 foo
drwxr-xr-x    2 nobody   nogroup       4096 Aug 23 00:00 home
dr-xr-xr-x  118 root     root             0 Aug 27 01:34 proc
drwxr-xr-x    2 root     root          4096 Aug 23 00:00 root
dr-xr-xr-x   12 root     root             0 Aug 27 01:34 sys
drwxrwxrwt    2 root     root          4096 Aug 23 00:00 tmp
drwxr-xr-x    3 root     root          4096 Aug 23 00:00 usr
drwxr-xr-x    4 root     root          4096 Aug 23 00:00 var
 ---> eaa4cf7ef226
Removing intermediate container 5458cc315cd3
Successfully built eaa4cf7ef226
PS E:\docker\build\lcow>

lowenna · 2017-08-27T02:07:29Z

@tonistiigi @dmcgowan @johnstep @mlaventure @stevvooe @vieux @friism PTAL.

This is now ready for review, and can be tested in conjunction with the matching (somewhat hacked as it's dependent on this PR) CLI changes in docker/cli#474. Manual testing results above.

After this, the next step is re-coalescing the image store so that there is a single one in the engine. With that, the Windows daemon should be able to run in dual-LCOW & WCOW modes again, and the environment variable LCOW_SUPPORTED can be removed. I'm working on that next step now.

@dnephin @simonferquel @jstarks @darrenstahlmsft @PatrickLang @thaJeztah @crosbymichael FYI.

lowenna · 2017-08-27T02:19:10Z

@stevvooe Note the 3 functions I've added in pkg\system\lcow.go which do the translation/parsing/validation of a platform flag passed from the CLI as os[/arch[/variant]] to an OCI image-spec Platform. I've switched the engine to use the OCI image-spec Platform structure as far as reasonably possible, without implementing the full multi-platform/architecture awareness the engine needs, which, as discussed, is out of scope of just the LCOW work.

I think I will need similar functionality in the containerd package in containerd/containerd#1403 to support these same three functions. However, we can migrate to those at a later date. I've put some pretty clear comments in that the existing functions I've introduced are temporary, and will migrate in the future.

mlaventure · 2017-08-28T15:07:04Z

api/server/router/image/image_routes.go

nit: Can we avoid this extra indent?

For instance, we could name the function return in which case the following defer should work:

defer func() { if err != nil { if !output.Flushed() { return } output.Write(streamformatter.FormatError(err)) } err = nil }()

I'm not sure we can that simply. The code can't go down the pull/import path if GetRequestedPlatform fails. Unless I'm missing something obvious, which is very possible.

mlaventure · 2017-08-28T15:08:35Z

api/types/backend/build.go

Why the rename?

Same response :)

mlaventure · 2017-08-28T15:09:25Z

api/types/types.go

Why the rename?

@mlaventure It's due to this that I put at the top:

In addition, it renames (almost all) uses of the string variable platform (and associated
methods/functions) to os, or operatingSystem where it clashes with the os import. This makes it much clearer to disambiguate with the swarm "platform" which is really os/arch. This is a stepping stone to getting the daemon towards fully multi-platform/arch-aware, and makes it clear when "operating system" is being referred to rather than "platform" which is misleadingly used - sometimes in the swarm
meaning, but more often as just the operating system.

In this case, shouldn't we use an OCI platform spec?

Ah, yes, indeed. Platform was added much earlier in the LCOW work. This PR simply renamed it to OS. But in the light of the discussions about the OCI spec and specifiers, I agree, this should be the OCI image-spec Platform. Will update.

@stevvooe - That's updated now. For the time being, only the OS field is populated as that the only field in the Container object currently. As the daemon is moved to be fully multi-platform/arch (etc) aware, as per the F2F discussions, this field can be extended.

mlaventure · 2017-08-28T15:14:08Z

builder/dockerfile/containerbackend.go

Oops. Yes, will fix

mlaventure · 2017-08-28T15:22:26Z

builder/dockerfile/dispatchers.go

nit: how about something like:

imageImage := &image.Image{} imageImage.OS = runtime.GOOS if runtime.GOOS == "windows" { switch b.options.Platform.OS { case "windows": return nil, errors.New("Windows does not support FROM scratch") case "linux": if !system.LCOWSupported() { return nil, errors.New("LCOW is not supported on this system") } imageImage.OS = "linux" default: return nil, errors.Errorf("OS %s is not supported", b.options.Platform.OS) } } return builder.Image(imageImage), nil

Sure, will update

mlaventure · 2017-08-28T15:23:30Z

builder/dockerfile/imagecontext.go

Shouldn't we just pass the platform here?

Actually, the same comment as before. When things are fully multi-platform/arch aware throughout the daemon, I agree. However, for now, the only bit which is actually used is the operating system, hence I'm making that obvious. (Clearly as mentioned before, full multi-platform/arch is out of scope of LCOW directly)

lowenna · 2017-08-30T02:07:56Z

@mlaventure comments addressed and rebased.

stevvooe · 2017-08-30T22:45:08Z

api/swagger.yaml


            Only the registry domain name (and port if not the default 443) are required. However, for legacy reasons, the Docker Hub registry must be specified with both a `https://` prefix and a `/v1/` suffix even though Docker will prefer to use the v2 registry API.
          type: "string"
+        - name: "X-Requested-Platform"


Why a header with json in it?

Answered in larger comment below.

stevvooe · 2017-08-30T22:45:58Z

@jhowardmsft Rather than use a header with json, let's define a parameter using the platform selector syntax in containerd/containerd#1403.

lowenna · 2017-08-31T05:06:54Z

@stevvooe. Am I missing what you mean by "as a parameter"? You mean as in POST /endpoint?foo=bar&platform=.....? That already was ruled out from a previous comment.....

Otherwise there's only two ways to send "structured data" from the CLI through the API to the daemon. That being either an HTTP header, or JSON encoded in the body.

I can't use the body for this as it build uses the body for the build context. Hence the reason for adding it as a header, avoiding a breaking change.

As for encoding, there's really only a handful of choices here.

a) Pass an un-parsed string and handle it server side. That simply isn't clean.
b) Parse it (and validate) it client side and pass structured data to the server. In terms of encoding structured data, the real two choices open are JSON or base64. JSON IMO seems better here.

I simply went for b). That seems a far better option.

As for using containerd/containerd#1403, as I have commented in the code, the parsing functions are to be updated once that is merged. It isn't yet. However, that PR still bases the underlying platform structure on the OCI Image specs Platform definition, which is exactly what I have used here. I don't think it is unreasonable to use that in the API.

stevvooe · 2017-09-22T17:50:22Z

api/server/router/image/image_routes.go

What err is this? This doesn't look like it gets set.

Good catch, thanks. It was a copy/paste error on my part into the if block above where the platform validation could fail. In this case, we still need to flush the output at the end of this function, but obviously not do the PullImage or ImportImage calls into the backend. Fixed, pushing it shortly.

stevvooe · 2017-09-22T17:51:27Z

api/server/router/build/build_routes.go

lowenna · 2017-09-25T23:30:44Z

Hey @stevvooe - any other issues you can see in here? Thanks :)

stevvooe · 2017-09-29T23:25:33Z

@jhowardmsft I think you need to remove that err check still.

lowenna · 2017-10-02T16:19:29Z

@stevvooe Ah, I see why you say that now. Came back to it with fresh eyes this morning. The intent was that the output was flushed/written, but the backend wasn't called in the case the platform passed in the API was invalid. I've now fixed that, but it requires keeping the error check in.

stevvooe · 2017-10-02T21:51:13Z

@jhowardmsft What can set that error? Based on the current code, it is always nil, unless I'm missing something.

lowenna · 2017-10-02T22:14:23Z

@stevvooe It's set in the block immediately above it where it's validating what is passed in via the API:

...
		platform = system.ParsePlatform(apiPlatform)
		if err = system.ValidatePlatform(platform); err != nil {
			err = fmt.Errorf("invalid platform: %s", err)
		}
	}

	if err == nil {
		if image != "" {
...

stevvooe · 2017-10-02T22:53:40Z

LGTM

My that is awkward code.

johnstep

LGTM

johnstep · 2017-10-04T05:45:53Z

daemon/create.go

s/mean/means/

johnstep · 2017-10-04T05:47:35Z

daemon/create.go

The earlier scratch case checks for system.LCOWSupported(). Is that required here, or unnecessary there?

johnstep · 2017-10-04T05:53:58Z

distribution/pull_v2.go

bath? Is that short for bailout path?

johnstep · 2017-10-04T05:59:13Z

pkg/system/init_windows.go

16299.15? Is the UBR (15, for example) important, and is there a way to deal with it?

johnstep · 2017-10-04T06:00:35Z

pkg/system/lcow.go

No need to change this, since it is temporary and should be short-lived, but I would have checked for >= 1 first and then nested >=2 in that, and nested == 3 in that.

Signed-off-by: John Howard <jhoward@microsoft.com> This PR has the API changes described in moby#34617. Specifically, it adds an HTTP header "X-Requested-Platform" which is a JSON-encoded OCI Image-spec `Platform` structure. In addition, it renames (almost all) uses of a string variable platform (and associated) methods/functions to os. This makes it much clearer to disambiguate with the swarm "platform" which is really os/arch. This is a stepping stone to getting the daemon towards fully multi-platform/arch-aware, and makes it clear when "operating system" is being referred to rather than "platform" which is misleadingly used - sometimes in the swarm meaning, but more often as just the operating system.

Signed-off-by: John Howard <jhoward@microsoft.com>

lowenna · 2017-10-06T22:29:26Z

@johnstep (@dmcgowen) - rebased after #35090 was merged and re-verified. Note, I've left the regression introduced by #35090 (comment) in there for you guys to come up with a spot fix (with the further caveat I still have reservations about the approach 35090 takes)

lowenna requested review from cpuguy83, dnephin, johnstep and tonistiigi as code owners August 27, 2017 00:56

GordonTheTurtle added the status/0-triage label Aug 27, 2017

lowenna force-pushed the jjh/add-platform-to-api branch 2 times, most recently from 5f2907a to 66b10da Compare August 27, 2017 01:13

lowenna force-pushed the jjh/add-platform-to-api branch 2 times, most recently from 287dc3f to 8fae079 Compare August 27, 2017 01:29

lowenna mentioned this pull request Aug 27, 2017

LCOW: Builder platform API #34401

Closed

lowenna force-pushed the jjh/add-platform-to-api branch from 8fae079 to e65babf Compare August 27, 2017 01:39

This was referenced Aug 27, 2017

[WIP pending moby/moby] - LCOW: Add --platform to docker build docker/cli#416

Closed

LCOW: CLI changes to add platform flag - pull, run, create and build docker/cli#474

Merged

lowenna changed the title ~~WIP - *IGNORE FOR NOW* - LCOW: API: Add platform to /images/create and /build~~ LCOW: API: Add platform to /images/create and /build Aug 27, 2017

lowenna added status/2-code-review and removed status/0-triage labels Aug 27, 2017

lowenna requested review from mlaventure, stevvooe and vieux and removed request for cpuguy83 August 27, 2017 03:26

mlaventure suggested changes Aug 28, 2017

View reviewed changes

lowenna force-pushed the jjh/add-platform-to-api branch from e65babf to d3d7ce3 Compare August 30, 2017 02:07

stevvooe reviewed Aug 30, 2017

View reviewed changes

lowenna force-pushed the jjh/add-platform-to-api branch from 0b4b8f4 to 5a2f85a Compare September 22, 2017 17:38

stevvooe reviewed Sep 22, 2017

View reviewed changes

api/server/router/build/build_routes.go Outdated

Copy link

Contributor

stevvooe Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

lowenna force-pushed the jjh/add-platform-to-api branch 2 times, most recently from ee92aea to 4b914b8 Compare September 22, 2017 19:23

johnstep approved these changes Oct 4, 2017

View reviewed changes

johnstep added status/4-merge and removed status/2-code-review labels Oct 5, 2017

lowenna mentioned this pull request Oct 6, 2017

Add support for Windows version filtering on pull #35090

Merged

LCOW: API change JSON header to string POST parameter

d98ecf2

Signed-off-by: John Howard <jhoward@microsoft.com>

lowenna force-pushed the jjh/add-platform-to-api branch from 4b914b8 to d98ecf2 Compare October 6, 2017 22:28

johnstep merged commit a3efe97 into moby:master Oct 7, 2017

lowenna mentioned this pull request Oct 9, 2017

Fix regression on inspect #35144

Merged

carlfischer1 mentioned this pull request Jan 9, 2018

docker-compose build failed on LCOW mode docker/compose#5548

Closed

KevM mentioned this pull request Mar 20, 2018

Add platform switch to docker run, image, create commands MihaMarkic/Cake.Docker#33

Closed

3 tasks

thaJeztah added the area/lcow Issues and PR's related to the experimental LCOW feature label Oct 30, 2018

thaJeztah mentioned this pull request Jun 3, 2020

doc: server API Correct ImagesCreate - platform parameter added in 1.32 #41060

Merged

This was referenced Dec 22, 2021

docker import does not honor the --platform flag #42813

Closed

Add support for platform (os and architecture) on image import #43103

Merged

thaJeztah mentioned this pull request Jan 25, 2022

Remove various leftover LCOW checks #43187

Merged

thaJeztah mentioned this pull request Mar 11, 2022

Remove LCOW (step 9): layer: remove layerstore.setOS(), layerstore.getOS() #43362

Merged

mtrmac mentioned this pull request Dec 5, 2022

Error : panic: inconsistency - windowsfilter graphdriver should not be used when in LCOW mode containers/storage#1125

Closed

Conversation

lowenna commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lowenna commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lowenna commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lowenna commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lowenna commented Aug 27, 2017 • 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!

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!

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!

lowenna commented Aug 30, 2017 • 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!

stevvooe commented Aug 30, 2017

Uh oh!

lowenna commented Aug 31, 2017

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!

lowenna commented Sep 25, 2017

Uh oh!

stevvooe commented Sep 29, 2017

Uh oh!

lowenna commented Oct 2, 2017

Uh oh!

stevvooe commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lowenna commented Oct 2, 2017

Uh oh!

stevvooe commented Oct 2, 2017

Uh oh!

johnstep left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lowenna commented Aug 27, 2017 •

edited

Loading

lowenna commented Aug 27, 2017 •

edited

Loading

lowenna commented Aug 27, 2017 •

edited

Loading

lowenna commented Aug 27, 2017 •

edited

Loading

lowenna commented Aug 27, 2017 •

edited

Loading

lowenna commented Aug 30, 2017 •

edited

Loading

stevvooe commented Oct 2, 2017 •

edited

Loading

lowenna commented Oct 6, 2017 •

edited

Loading