Skip to content
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

runtime.md: only required properties are MUST applied #935

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hqhq
Copy link
Contributor

@hqhq hqhq commented Nov 9, 2017

Address the issue mentioned in:
#405 (comment)

We already doing this in runc as we can run a container
with a config which has windows entry. And this is
the right way to handle it.

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

@wking
Copy link
Contributor

wking commented Nov 9, 2017 via email

Address the issue mentioned in:
opencontainers#405 (comment)

We already doing this in runc as we can run a container
with a config which has `windows` entry. And this is
the right way to handle it.

Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>
@hqhq
Copy link
Contributor Author

hqhq commented Nov 9, 2017

@wking I've updated this PR to allow ignore platform specific configuration, now vm based container has to figure out how they can be ignored by other runtimes :( @sameo

Copy link
Contributor

@wking wking left a comment

Choose a reason for hiding this comment

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

... now vm based container has to figure out how they can be ignored by other runtimes :(

I think the way to do that is to be declarative:

Would you need anything else?

@@ -100,9 +100,10 @@ This operation MUST [generate an error](#errors) if it is not provided a path to
If the ID provided is not unique across all containers within the scope of the runtime, or is not valid in any other way, the implementation MUST [generate an error](#errors) and a new container MUST NOT be created.
This operation MUST create a new container.

All of the properties configured in [`config.json`](config.md) except for [`process`](config.md#process) MUST be applied.
All of the properties configured in [`config.json`](config.md) except for [`process`](config.md#process) and [Platform](spec.md#platforms)-specific configuration beyond the target platform MUST be applied.
Copy link
Contributor

@wking wking Nov 9, 2017

Choose a reason for hiding this comment

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

I feel like this is the idea that the extensibility section was trying to convey,and which I was trying to tighten up in #680. Instead of attempting to repeat the condition here, I would rather update that section to cover the non-ignore-ability of properties defined by this spec for that target platform (even ones like process.rlimits, which lie outside of linux). Text here that references that section would be great, though. Maybe something like:

All of the [recognized properties](config.md#extensibility) configured in [`config.json`](config.md) except for [`process`](config.md#process) MUST be applied.

with wording in the extensibility section and/or glossary to more formally define "recognized properties"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wking I don't think the extensibility section conveys this. In my understanding, unknown property means the property is not defined in spec, not the property that runtime doesn't support (if we have different understanding, maybe glossary for unknown property is needed?).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the extensibility section conveys this.

Not yet (#680 is me trying to tighten part of it down). I'm staying that the extensibility section is where we already attempt to cover this sort of thing, not that it already covers the hole you've identified here.

I'd rather tighten up the extensibility section to cover all of the “which properties can the runtime ignore?” conditions, because if it covers a subset, and runtime.md adds a few more, then config authors might read the extensibility section, think they understand what the runtime can ignore, and then be surprised when it also ignores something that was added in runtime.md. It's easier to understand if we collect all the conditions in one place.

@sameo
Copy link

sameo commented Nov 9, 2017

Declare a vm platform (I would have preferred "protocol", see #570).

I don't think a VM is a platform. They're almost orthogonal concepts, given that all existing OCI platforms support hardware virtualization, i.e. you could potentially have a vm platform and a {linux,solaris,windows} platform.

@wking
Copy link
Contributor

wking commented Nov 9, 2017

I don't think a VM is a platform.

Which is why I prefer "protocol", but the maintainers wanted "platform".

... you could potentially have a vm platform and a {linux,solaris,windows} platform.

Which is why I used vm,linux in my example.

And taking a step back, this feels like we're gradually circling back to #15.

@vbatts
Copy link
Member

vbatts commented Dec 17, 2019

house keeping: this use of MAY would not clear up the situation in the future where platform supportable features overlap (i.e. Windows Subsystem for Linux (WSL) possibly running linux container tech on a windows host). VM is not a platform, but this language should not loosen edge-cases.
I'm inclined to close this issue for now.

@h-vetinari h-vetinari mentioned this pull request Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants