Expand platform to be more sophisticated

[hqhq]

Currently support is not sufficient for many platforms,
such as ARM, we need more information to identify a
specific platform.

Image-spec already had these entries, so I just picked
them up and applied to runtime-spec.

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

[AkihiroSuda]

This is being removed opencontainers/image-spec#672

[stevvooe]

Yes, please remove this.

[hqhq]

I see why you remove this field, it's a fair point to me, I'll update. Thanks.

[wking]

On Mon, May 15, 2017 at 11:02:28PM -0700, Qiang Huang wrote: Image-spec already had these entries, so I just picked them up and applied to runtime-spec.

I'm not excited about having these properties documented in two different repositories (with an intention to keep them synchronized?). Do you file a new runtime-spec PR if/when opencontainers/image-spec#652 lands? Do we need to synchronize releases so that there is at least one image-spec release with the same language as a given runtime-spec release (and vice versa)? The image and runtime use-cases are a bit different. For image-spec, this is registry information, allowing folks to say: I want a version of image FOO that can run on PLATFORM. For runtime-spec, the use case is presumably: I'm not sure if my runtime will execute this container on my current host or some other platform (e.g. via an internal HTTP API). If the runtime chooses to launch the container on another host, I need it to pick one like this. You probably need the same metadata for both use-cases, but it's not clear to me if we need to support that runtime-spec use-case out of the box. Maybe we should drop ‘platform’ instead and point folks who need that workflow towards ‘annotations’.

[stevvooe]

Should runtime-spec adopt the platform specification from the image-spec? Are these even used in the runtime spec? What would be the impact of making that change? I think the only addition is to have architecture instead of arch.

[crosbymichael]

Maybe we remove this from runtime spec as we do not consume it anywhere nor intended to when we initially added the fields? We could leave this as an image-spec concern.

[wking]

Maybe we remove this from runtime spec as we do not consume it anywhere…

We currently consume it for validation, which is one instance of “the config you're looking at may target a different platform than your current host”. Without platform.os, you don't know if platform.apparmorProfile is legal or not. But as long as there is some way to select your compliance track, I'm fine dropping the rest of platform.

[tianon]

I'd definitely be +1 to punting the platform object to the image-spec -- maintaining that list in one place makes a lot of sense (and it's definitely more relevant to the image-spec).

[hqhq]

Are we talking about keep Platform as is or remove it from runtime-spec?

[crosbymichael]

Ya, talking about removing it. I remember when we added it we were never going to use it in the runtime, it was an informational field. But this was before the image spec. If we don't have any real usecases for a runtime to consume it, its worth considering removing it.

[cyphar]

At the very least we should make it an annotation.

[mattspencer-arm]

I agree with the sentiment of having this defined in one place. Currently the image-spec references the runtime spec for the definition of platform.architecture and platform.os. I would suggest as above removing platform from the runtime-spec and having the image spec being the canonical definition.

[dqminh]

Ya, talking about removing it. I remember when we added it we were never going to use it in the runtime, it was an informational field. But this was before the image spec. If we don't have any real usecases for a runtime to consume it, its worth considering removing it.

platform.os should at least be useful in config validation though ? Like the current way we are using / wording it

[**`platform.os`**](#platform) is used to lookup further platform-specific configuration.

[wking]

On Tue, May 23, 2017 at 08:01:41AM -0700, Daniel, Dao Quang Minh wrote: [**`platform.os`**](#platform) is used to lookup further platform-specific configuration.

It's used for more than lookup though. For example, both whether root.path is \ or / delimited and whether ‘proc’ has a chance at being is a valid mounts[].type depend on the platform. I suggest we handle this (and close off #725) with something like: * `platform` (string, REQUIRED) the platform targetted by this configuration. The value MUST be a slug from [the platform list](spec.md#platforms).

[crosbymichael]

@dqminh either that or on validation you just pass in the platform you want to validate for.

runtime-tools validate --platform linux <config.json>

It would actually be more robust for validation that you validate what you expect, not what the spec says it is.

@cyphar they are annotations, you can put whatever you want there, that's way its a map[string]string

[wking]

… on validation you just pass in the platform you want to validate for.

I don't see a robustness difference, but I'm fine with this approach (and completely dropping platform from the config) too.

[lowenna]

I'd pick a more up to date build. And it's actually a quad-tuple these days (it was a triplet). eg 10.0.14393.1198 is the current fully-patched Windows Server 2016 RS1 build number.

[lowenna]

(Historical - 10586 was a pre-release, IIRC Technical Preview 4 of Windows Server 2016)

[wking]

I'd pick a more up to date build.

My take on the discussion is that runtime-spec will likely drop platform or keep only platform.os. Either way, this line is unlikely to land in runtime-spec, so we probably don't need to worry about it.

And the image-spec analog got a quad-tuple in the recently-landed opencontainers/image-spec#651.

[hqhq]

Thanks for all your comments, I think we have conclusion not to expand platform and sync with image-spec, I opened a new PR to simply it in #850 , I'll close this PR.

[stevvooe]

I think we have conclusion not to expand platform and sync with image-spec,

How did we arrive at this conclusion? #850 is pretty insufficient.

I would favor just removing this altogether rather than make this just a string.

[tianon]

@stevvooe and then use the per-platform objects to differentiate for validation? (and update them to all be mutually-exclusive directly, rather than mutually-exclusive based on the value of platform.os)

ie, if the windows property is set, the linux and solaris properties MUST NOT be, etc (rather than the current language which hinges around platform.os)

[wking]

On Wed, May 24, 2017 at 02:07:27PM -0700, Tianon Gravi wrote: ie, if the `windows` property is set, the `linux` and `solaris` properties MUST NOT be, etc (rather than the current language which hinges around `platform.os`)

Currently the ‘linux’ and ‘solaris’ properties are OPTIONAL. The approach you're floating would make one of windows/linux/solaris REQUIRED, and folks who didn't need anything from inside ‘linux’ (for example) would have to set ‘"linux": {}’. Not that that's much more difficult from a config-author side than requiring them to set ‘"platform": "linux"’, but it is more complicated from a validation-tooling side (because you have to look at multiple properties to make sure the MUST NOT is satisfied.

[tianon]

If I don't need any of the platform-specific properties for a given bundle, isn't that like saying that the bundle doesn't care which platform it runs on? It seems safe enough to me, especially since we already don't make any guarantees about the "runnability" of the binaries in the rootfs (since that's really more of an image-spec / distribution concern). I don't see any reason to need to require one of them, but making them mutually exclusive is probably a good idea.

[wking]

On Wed, May 24, 2017 at 03:00:27PM -0700, Tianon Gravi wrote: If I don't need any of the platform-specific properties for a given bundle, isn't that like saying that the bundle doesn't care which platform it runs on?

You might still need platform-specific properties (e.g. process.user.uid and soon mounts[].type, #854) without needing any of the platform-specific properties under ‘linux’. If ‘linux’ is not set (with your approach, or ‘"platform": "linux" is not set with the #850 approach, or ‘--platform linux’ is not set with @crosbymichael's approach [1]), then the validation tooling has no way to validate: { "ociVersion": "1.0.0", "root": { "path": "rootfs" }, "process": { "cwd": "/", "args": [ "sh" ], "user": { "uid": 0, "gid": 0 } } } Is that a valid Windows config (which happens to set the undefined-for-Windows process.user.uid and process.user.gid)? Is that a valid Linux/Solaris config? I think it's better to require some explicit declaration of the intended compliance track [2] to avoid the complication of guessing based on which properties are set and what they're set to. [1]: #830/#issuecomment-303463569 [2]: https://github.com/opencontainers/runtime-spec/blob/2ec18c02474dd49ce20aa30236959a8a80763042/spec.md#platforms

[wking]

On Wed, May 24, 2017 at 03:11:33PM -0700, W. Trevor King wrote: Is that a valid Windows config (which happens to set the undefined-for-Windows process.user.uid and process.user.gid)?

This will no longer be an option once #828 lands and starts requiring ‘windows’ for all Windows configs [1]: This MUST be set if **`platform.os`** is `windows` and MUST NOT be set otherwise. In that case, the issue will be distinguishing between Solaris and Linux compliance tracks, and there's not a lot of separation there at the moment outside of the ‘linux’ and ‘solaris’ prorperties. However, that seems like a brittle condition to hang sane compliance testing on. For example, it would be impossible to violate the new-in-#828 ‘windows’ condition, even if you *intended* to target ‘windows’ but forgot to set that property. An explicit compliance track declaration is harder to muff. [1]: https://github.com/opencontainers/runtime-spec/pull/828/files#diff-c9c91c29b41257aea3a3403cc606ad99L331