New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LCOW: API: Add `platform` to /images/create and /build #34642

Merged
merged 2 commits into from Oct 7, 2017

Conversation

@jhowardmsft
Contributor

jhowardmsft commented Aug 27, 2017

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.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 27, 2017

Contributor

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>
Contributor

jhowardmsft commented Aug 27, 2017

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>
@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 27, 2017

Contributor

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>
Contributor

jhowardmsft commented Aug 27, 2017

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>

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

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 27, 2017

Contributor

@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.

Contributor

jhowardmsft commented Aug 27, 2017

@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.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 27, 2017

Contributor

@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.

Contributor

jhowardmsft commented Aug 27, 2017

@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.

@jhowardmsft jhowardmsft requested review from mlaventure, stevvooe and vieux and removed request for cpuguy83 Aug 27, 2017

Show outdated Hide outdated api/server/router/image/image_routes.go Outdated
@@ -40,5 +40,5 @@ type GetImageAndLayerOptions struct {
PullOption PullOption
AuthConfig map[string]types.AuthConfig
Output io.Writer
Platform string
OS string

This comment has been minimized.

@mlaventure

mlaventure Aug 28, 2017

Contributor

Why the rename?

@mlaventure

mlaventure Aug 28, 2017

Contributor

Why the rename?

This comment has been minimized.

@jhowardmsft

jhowardmsft Aug 30, 2017

Contributor

Same response :)

@jhowardmsft

jhowardmsft Aug 30, 2017

Contributor

Same response :)

Show outdated Hide outdated api/types/types.go Outdated
Show outdated Hide outdated builder/dockerfile/containerbackend.go Outdated
Show outdated Hide outdated builder/dockerfile/dispatchers.go Outdated
Show outdated Hide outdated builder/dockerfile/imagecontext.go Outdated
@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 30, 2017

Contributor

@mlaventure comments addressed and rebased.

Contributor

jhowardmsft commented Aug 30, 2017

@mlaventure comments addressed and rebased.

@yoonnnnn

This comment has been minimized.

Show comment
Hide comment
@yoonnnnn

yoonnnnn Aug 30, 2017

yoonnnnn commented Aug 30, 2017

Show outdated Hide outdated api/swagger.yaml Outdated
@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 30, 2017

Contributor

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

Contributor

stevvooe commented Aug 30, 2017

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

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 31, 2017

Contributor

@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.

Contributor

jhowardmsft commented Aug 31, 2017

@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.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Aug 31, 2017

Member

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.

This option is the original os[/arch[/variant]] right? I actually think that's the best option. The API needs to validate the input anyway, it can't just trust what it receives.

Member

dnephin commented Aug 31, 2017

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.

This option is the original os[/arch[/variant]] right? I actually think that's the best option. The API needs to validate the input anyway, it can't just trust what it receives.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 31, 2017

Contributor

You mean as in POST /endpoint?foo=bar&platform=.....? That already was ruled out from a previous comment.....

Which comment?

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.

Ok, so let's block this on having that merged. We should use that format throughout the ecosystem to specify platforms. Using the OCI specification is fine, but that ties things to OCI (however, I'd like to see specifiers in OCI).

Contributor

stevvooe commented Aug 31, 2017

You mean as in POST /endpoint?foo=bar&platform=.....? That already was ruled out from a previous comment.....

Which comment?

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.

Ok, so let's block this on having that merged. We should use that format throughout the ecosystem to specify platforms. Using the OCI specification is fine, but that ties things to OCI (however, I'd like to see specifiers in OCI).

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 5, 2017

Contributor

Which comment?

@stevvooe - It comes from the discussion in #34617 (comment)

Contributor

jhowardmsft commented Sep 5, 2017

Which comment?

@stevvooe - It comes from the discussion in #34617 (comment)

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 12, 2017

Contributor

@stevvooe - Can you comment on the above? I'm trying to determine how you want the API to look in terms of passing the parameter. From conversations offline from @johnstep, it sounds like you do NOT want the JSON.

Which takes me back to what I originally had, as an HTTP API parameter such as POST /function?platform=os/arch for example. Can you confirm that?

Contributor

jhowardmsft commented Sep 12, 2017

@stevvooe - Can you comment on the above? I'm trying to determine how you want the API to look in terms of passing the parameter. From conversations offline from @johnstep, it sounds like you do NOT want the JSON.

Which takes me back to what I originally had, as an HTTP API parameter such as POST /function?platform=os/arch for example. Can you confirm that?

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 18, 2017

Contributor

Rebased.

Note the janky error is unrelated - is failing for all PRs currently. Now fixed

Contributor

jhowardmsft commented Sep 18, 2017

Rebased.

Note the janky error is unrelated - is failing for all PRs currently. Now fixed

jhowardmsft added a commit to jhowardmsft/cli that referenced this pull request Sep 18, 2017

HACK: Vendor docker/docker for API changes
Signed-off-by: John Howard <jhoward@microsoft.com>

This is a temporary hack - depends on moby/moby#34642
to be merged.
@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Sep 18, 2017

Contributor

@jhowardmsft I think you should only support os, as I said above, since vendoring the new containerd is going to create other problems.

Contributor

stevvooe commented Sep 18, 2017

@jhowardmsft I think you should only support os, as I said above, since vendoring the new containerd is going to create other problems.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 18, 2017

Contributor

@stevvooe - OK, that's a trivial change. I'll make it shortly.

Contributor

jhowardmsft commented Sep 18, 2017

@stevvooe - OK, that's a trivial change. I'll make it shortly.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 18, 2017

Contributor

@stevvooe - Done. (See ValidatePlatform in pkg\system\lcow.go)

Contributor

jhowardmsft commented Sep 18, 2017

@stevvooe - Done. (See ValidatePlatform in pkg\system\lcow.go)

jhowardmsft added a commit to jhowardmsft/cli that referenced this pull request Sep 19, 2017

HACK: Vendor docker/docker for API changes
Signed-off-by: John Howard <jhoward@microsoft.com>

This is a temporary hack - depends on moby/moby#34642
to be merged.
@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 20, 2017

Contributor

Rebased after some builder refactoring was merged recently

Contributor

jhowardmsft commented Sep 20, 2017

Rebased after some builder refactoring was merged recently

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 20, 2017

Contributor

Resolved CI failures. All green.

Contributor

jhowardmsft commented Sep 20, 2017

Resolved CI failures. All green.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 22, 2017

Contributor

Rebased on master

Contributor

jhowardmsft commented Sep 22, 2017

Rebased on master

// for a pull it is not an error if no auth was given
// to increase compatibility with the existing api it is defaulting to be empty
authConfig = &types.AuthConfig{}
if err == nil {

This comment has been minimized.

@stevvooe

stevvooe Sep 22, 2017

Contributor

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

@stevvooe

stevvooe Sep 22, 2017

Contributor

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

This comment has been minimized.

@jhowardmsft

jhowardmsft Sep 22, 2017

Contributor

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.

@jhowardmsft

jhowardmsft Sep 22, 2017

Contributor

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.

if err := system.ValidatePlatform(p); err != nil {
return nil, validationError{fmt.Errorf("invalid platform: %s", err)}
}
options.Platform = p.OS

This comment has been minimized.

@stevvooe

stevvooe Sep 22, 2017

Contributor

👍

@stevvooe

stevvooe Sep 22, 2017

Contributor

👍

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Sep 25, 2017

Contributor

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

Contributor

jhowardmsft commented Sep 25, 2017

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

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Sep 29, 2017

Contributor

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

Contributor

stevvooe commented Sep 29, 2017

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

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Oct 2, 2017

Contributor

@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.

Contributor

jhowardmsft commented Oct 2, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Oct 2, 2017

Contributor

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

Contributor

stevvooe commented Oct 2, 2017

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

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Oct 2, 2017

Contributor

@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 != "" {
...
Contributor

jhowardmsft commented Oct 2, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Oct 2, 2017

Contributor

LGTM

My that is awkward code.

Contributor

stevvooe commented Oct 2, 2017

LGTM

My that is awkward code.

@johnstep

LGTM

os = img.OS
}
} else {
// This mean scratch. On Windows, we can safely assume that this is a linux

This comment has been minimized.

@johnstep

johnstep Oct 4, 2017

Contributor

s/mean/means/

@johnstep

johnstep Oct 4, 2017

Contributor

s/mean/means/

return nil, fmt.Errorf("cannot create a %s container from a %s image", params.Platform, img.Platform())
}
} else {
if runtime.GOOS == "windows" {

This comment has been minimized.

@johnstep

johnstep Oct 4, 2017

Contributor

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

@johnstep

johnstep Oct 4, 2017

Contributor

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

@@ -605,6 +613,12 @@ func (p *v2Puller) pullSchema2(ctx context.Context, ref reference.Named, mfst *s
return "", "", errRootFSMismatch
}
// Early bath if the requested OS doesn't match that of the configuration.

This comment has been minimized.

@johnstep

johnstep Oct 4, 2017

Contributor

bath? Is that short for bailout path?

@johnstep

johnstep Oct 4, 2017

Contributor

bath? Is that short for bailout path?

var lcowSupported = false
// InitLCOW sets whether LCOW is supported or not
// TODO @jhowardmsft.
// 1. Replace with RS3 RTM build number.

This comment has been minimized.

@johnstep

johnstep Oct 4, 2017

Contributor

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

@johnstep

johnstep Oct 4, 2017

Contributor

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

}
if len(elements) >= 1 {
p.OS = elements[0]
}

This comment has been minimized.

@johnstep

johnstep Oct 4, 2017

Contributor

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.

@johnstep

johnstep Oct 4, 2017

Contributor

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.

LCOW: API: Add platform to /images/create and /build
Signed-off-by: John Howard <jhoward@microsoft.com>

This PR has the API changes described in #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.
LCOW: API change JSON header to string POST parameter
Signed-off-by: John Howard <jhoward@microsoft.com>
@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Oct 6, 2017

Contributor

@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)

Contributor

jhowardmsft commented Oct 6, 2017

@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)

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

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37262 has succeeded
Details
janky Jenkins build Docker-PRs 45942 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6329 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17507 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6128 has succeeded
Details
@agronholm

This comment has been minimized.

Show comment
Hide comment
@agronholm

agronholm Jan 12, 2018

Is this still necessary, even with the new containerd? I would very much like to pull arm images on my amd64 laptop.

agronholm commented on pkg/system/lcow.go in d98ecf2 Jan 12, 2018

Is this still necessary, even with the new containerd? I would very much like to pull arm images on my amd64 laptop.

@KevM KevM referenced this pull request Mar 20, 2018

Closed

Add platform switch to docker run, image, create commands #33

0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment