Remove platform field by hqhq · Pull Request #850 · opencontainers/runtime-spec

hqhq · 2017-05-24T02:06:55Z

Close: #725

See discussion in #830 and dev-weekly-meeting discussion
in http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2017/opencontainers.2017-06-14-21.03.log.txt

We don't use this field in runc, and mostly only
image-spec cares about this, we can assume tools know
what specific platform this spec is built for.

Signed-off-by: Qiang Huang h.huangqiang@huawei.com

lowenna · 2017-05-24T02:13:31Z

- Values for **`arch`** SHOULD use, and runtimes SHOULD understand, **`arch`** entries listed in the Go Language document for [`GOARCH`][go-environment].
- If an architecture is not included in the `GOARCH` documentation, it SHOULD be submitted to this specification for standardization.
+**`platform`** (string, REQUIRED) specifies the configuration's target platform.
+ The value MUST be a slug from [the platform list](spec.md#platforms).


Not sure I like the word slug... Item? Entry?

Updated, thanks.

lowenna · 2017-05-24T02:14:46Z

- This MAY be set if **`platform.os`** is `linux` and MUST NOT be set otherwise.
+ This MAY be set if **`platform`** is `linux` and MUST NOT be set otherwise.
 * **`windows`** (object, OPTIONAL) [Windows-specific configuration](config-windows.md).
- This MAY be set if **`platform.os`** is `windows` and MUST NOT be set otherwise.


Actually, this MUST be set if the platform is windows. A container isn't going to run at all without it. Not sure whether this applies to other platforms though.

Maybe we can open a new PR to fix this? It's non-relevant to this PR.

Maybe we can open a new PR to fix this?
#828 is covering this change, so we can leave this as MAY for this PR.

Now that #828 has landed, this PR will need a rebase.

tianon · 2017-05-24T15:33:27Z

Since this appears to map platform to image-spec's platform.os, would it make sense to instead call this os or platformOs (and perhaps even reference image-spec)?

IMO dropping our platform object down to just os is a decent compromise (rather than removing platform entirely), but I'd love to hear the opinions of other @opencontainers/runtime-spec-maintainers.

wking · 2017-05-24T16:16:16Z

On Wed, May 24, 2017 at 08:33:29AM -0700, Tianon Gravi wrote: … and perhaps even reference image-spec…

There's no need for this, because the values come from the runtime-spec platform list [1]. [1]: https://github.com/opencontainers/runtime-spec/blob/2ec18c02474dd49ce20aa30236959a8a80763042/spec.md#platforms

stevvooe · 2017-05-24T21:04:37Z

This seems like a step backwards.

The only change needed to make it compatible with the image spec is arch -> architecture.

hqhq · 2017-06-02T03:11:12Z

@opencontainers/runtime-spec-maintainers More comments on this? I see many different suggestions and not sure which way to go.

wking · 2017-06-02T03:42:37Z

On Thu, Jun 01, 2017 at 08:11:13PM -0700, Qiang Huang wrote: More comments on this? I see many different suggestions and not sure which way to go.

I'm not a maintainer, but I think we want to gut the current platform (like you're doing in dc88c9e) unless someone wants support for the “I'm not sure if my runtime will execute this container on my current host or some other platform…” use case [1]. I haven't heard anyone advocating for that. We'll need some way to declare your compliance track [2], and for that I'm fine with either of: a. Your ‘platform’ property where the value “MUST be an item from the platform list [2]”. b. @crosbymichael's ‘--platform’ option [3], with the same compliance-track values [2]. I don't see much difference between a or b myself, but @crosbymichael sees his proposal as more robust [3]. You could swing something more complicated like @tianon's proposal [4], but I feel like using the platform-specific sections to both select the compliance track *and* hold some subset of the platform-specific properties is too overloaded [5]. [1]: #830 (comment) [2]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/spec.md#platforms [3]: #830 (comment) [4]: #830 (comment) [5]: #830 (comment)

lowenna · 2017-06-02T03:47:54Z

Actually, just to throw one out there.... Windows will imminently be able to run both Windows and Linux containers. So it might matter.

wking · 2017-06-02T03:53:07Z

On Thu, Jun 01, 2017 at 08:47:56PM -0700, John Howard wrote: Windows will imminently be able to run both Windows and Linux containers. So it might matter.

The arch will matter? I think all the proposals include some way to specify the compliance track [1], so you can still send an explicitly-Linux config into a runtime on a Windows host. Will the Windows runtime need any more information than “this is a windows config” or “this is a linux config”? [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/spec.md#platforms

lowenna · 2017-06-02T03:55:39Z

I'm still figuring that out. It will need bits of the Windows and Linux sections - for example the LayerFolders in Windows are going to be required for the runtime, but much of the Linux section will need to be populated as that's where the Linux settings are. For 1.0, I'd probably ignore my comment, it was just in passing. However, there will definitely be quite a few spec updates required for multi-platform on a single OS.

wking · 2017-06-02T03:59:38Z

On Thu, Jun 01, 2017 at 08:55:41PM -0700, John Howard wrote: It will need bits of the Windows and Linux sections - for example the LayerFolders in Windows are going to be required for the runtime, but much of the Linux section will need to be populated as that's where the Linux settings are.

*shudder* Some of these host-vs.-container issues have come up in #405, although I don't think compliance testing plans for those VM settings have been worked out yet.

For 1.0, I'd probably ignore my comment…

Ok, so we're back to “nobody is asking for anything more than the compliance track at the moment”.

wking · 2017-06-06T14:17:34Z

-
-* **`os`** (string, REQUIRED) specifies the operating system family of the container configuration's specified [`root`](#root) file system bundle.
- The runtime MUST generate an error if it does not support the specified **`os`**.
- Bundles SHOULD use, and runtimes SHOULD understand, **`os`** entries listed in the Go Language document for [`GOOS`][go-environment].


You'll want to drop the reference at the ebd of this file too, since you remove the only consumers.

crosbymichael · 2017-06-14T21:18:46Z

We discussed this on the call today and feel that it would be better to fully remove it from the spec. The tool that interprets the spec knows how/what platform it is built for or what platform it should use in the case of windows or VMs.

@hqhq can you update this PR to remove the platform field fully? Validation tools will be told what they are validating a specific spec against.

hqhq · 2017-06-15T17:00:45Z

@crosbymichael Updated.

wking · 2017-06-15T17:14:03Z

 [procfs_2]: https://www.kernel.org/doc/Documentation/filesystems/proc.txt
-[semver-v2.0.0]: http://semver.org/spec/v2.0.0.html
-[go-environment]: https://golang.org/doc/install/source#environment
+[semver-v3.0.0]: http://semver.org/spec/v2.0.0.html


I don't think you want to bump semver-v2.0.0 → semver-v3.0.0 (especially since 2.0.0 is still the most recent version ;).

Indeed -- minus this change, the rest of this PR LGTM now. 👍

Oops, no idea where that change comes from, updated.

crosbymichael · 2017-06-15T17:19:21Z

platform changes

LGTM

Close: opencontainers#725 See discussion in opencontainers#830 and dev-weekly-meeting discussion in http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2017/opencontainers.2017-06-14-21.03.log.txt We don't use this field in runc, and mostly only image-spec cares about this, we can assume tools know what specific platform this spec is built for. Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>

Mashimiao · 2017-06-16T07:22:03Z

Sorry for asking so late. I have a question.
If we set more than one of linux, windows and solaris, how can runtime distinguish which platform is the target platform?

wking · 2017-06-16T14:18:00Z

If we set more than one of linux, windows and solaris, how can runtime distinguish which platform is the target platform?

The runtime will have a compiled-in favorite. For example, a Linux build of runc will assume a Linux config, regardless of which properties are set.

Tools that can handle configs for multiple platforms (e.g. runtime-tools) will need a new --platform option.

tianon · 2017-06-16T18:21:14Z

LGTM

crosbymichael · 2017-06-16T18:36:13Z

LGTM

Through 63b7c6c (Merge pull request opencontainers#850 from hqhq/simply_platform, 2017-06-16). Signed-off-by: W. Trevor King <wking@tremily.us>

hqhq mentioned this pull request May 24, 2017
Expand platform to be more sophisticated #830
Closed

lowenna reviewed May 24, 2017
View reviewed changes

hqhq force-pushed the simply_platform branch 2 times, most recently from 3cc7816 to dc88c9eCompare May 24, 2017 02:22

mrunalp added this to the 1.0.0 milestone Jun 1, 2017

wking mentioned this pull request Jun 6, 2017
platform.arch != GOARCH #872
Closed

wking reviewed Jun 6, 2017
View reviewed changes

hqhq force-pushed the simply_platform branch from dc88c9e to 166581eCompare June 15, 2017 16:57

hqhq changed the title ~~Simply platform field~~Remove platform fieldJun 15, 2017

wking reviewed Jun 15, 2017
View reviewed changes

hqhq force-pushed the simply_platform branch from 166581e to af0e225Compare June 16, 2017 07:11

crosbymichael merged commit 63b7c6c into opencontainers:masterJun 16, 2017

This was referenced Jun 16, 2017
config: Bump Hyper-V condition from root.path to root itself #838
Merged
config: Do not allow runtimes to ignore properties defined by the spec #680
Open
validate: fix platform check opencontainers/runtime-tools#347
Closed

hqhq deleted the simply_platform branch June 17, 2017 12:10

vbatts mentioned this pull request Jul 5, 2017
bump to v1.0.0-rc6 #882
Merged

vishvananda mentioned this pull request Oct 20, 2017
Platform should be optional oracle/railcar#27
Closed

Uh oh!

Conversation

hqhq commented May 24, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lowennaMay 24, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tianon commented May 24, 2017

Uh oh!

wking commented May 24, 2017 via email

Uh oh!

stevvooe commented May 24, 2017

Uh oh!

hqhq commented Jun 2, 2017

Uh oh!

wking commented Jun 2, 2017 via email

Uh oh!

lowenna commented Jun 2, 2017

Uh oh!

wking commented Jun 2, 2017 via email

Uh oh!

lowenna commented Jun 2, 2017

Uh oh!

wking commented Jun 2, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented Jun 14, 2017

Uh oh!

hqhq commented Jun 15, 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!

crosbymichael commented Jun 15, 2017• edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mashimiao commented Jun 16, 2017

Uh oh!

wking commented Jun 16, 2017

Uh oh!

tianon commented Jun 16, 2017• edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crosbymichael commented Jun 16, 2017• edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hqhq commented May 24, 2017•
edited
Loading

lowennaMay 24, 2017•
edited
Loading

crosbymichael commented Jun 15, 2017•
edited by caniszczyk
Loading

tianon commented Jun 16, 2017•
edited by caniszczyk
Loading

crosbymichael commented Jun 16, 2017•
edited by caniszczyk
Loading