runtime.md: Require 'create' to fail if config.json asks for the impossible #559

wking · 2016-09-08T22:12:39Z

We don't want to silently ignore settings that we understand but cannot implement (we do want to ignore settings that we don't understand, but that's a separate issue).

This raises a slightly sticky certification issue. If a runtime always exits create with an error:

func create() err {
  return fmt.Errorf("nope, I cannot create that container either.")
}

it would be neither complaint nor non-compliant. It would not fail any MUSTs, but availing itself of the "cannot create the maintainer" option specified in this commit would mean the test suite could not test the deeper requirements around the config properties themselves.

So with this change, making Microsoft certifiable will still need an explicit weakening around root.path. The easiest way to do that might be to have separate annotations for whether a setting is optional for config authors and whether it's optional for runtime authors (supported):

* **`readonly`** (bool, config:optional, support:optional) ...

But I'll leave hashing that out to #476 or other PR. Regardless of the certification impact, we want to be clear that silently ignoring known parameters is wrong.

tianon · 2016-09-15T15:34:04Z

LGTM

I'm kind of surprised that we don't have something simple like "a runtime MUST be capable of creating containers", but IMO that's splitting hairs pretty badly -- in the real world, I can't imagine what would be gained by creating a runtime that errors out on create unconditionally, especially since it wouldn't last very long with its users.

wking · 2016-09-15T15:44:13Z

On Thu, Sep 15, 2016 at 08:34:05AM -0700, Tianon Gravi wrote:

I'm kind of surprised that we don't have something simple like "a
runtime MUST be capable of creating containers", but IMO that's
splitting hairs pretty badly…

It's not that black and white. What if you implement almost all of
your MUSTs, but use this one to avoid implementing some other MUSTs?
That's what @RobDolinMS was initially looking for in #476 (although he
was putting in the safety valve on the setting, and here I've put it
on ‘create’ and require an error). With this PR (as it stands in
10b731e), I'm opening the possibility of runtimes that are neither
complaint (they don't pass all their MUSTs) nor non-compliant (they
don't fail any MUSTs). I'm ok with that, although I'd also be ok
making compliant “passes all MUSTs” and non-compliant “is not
compliant”, in which case the always-error runtime and others that
don't completely cover their requirements would be non-compliant.

jonboulle · 2016-09-16T12:31:08Z

runtime.md

@@ -87,6 +87,7 @@ This operation MUST generate an error if it is not provided a path to the bundle
 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 and a new container MUST not be created.
 Using the data in [`config.json`](config.md), this operation MUST create a new container.
 This means that all of the resources associated with the container MUST be created, however, the user-specified code MUST NOT be run at this time.
+If the runtime cannot create the container as specified in `config.md`, it MUST generate an error and a new container MUST not be created.


driveby grammar, this and line 87 should be MUST NOT not MUST not

or SHOULD I say they MUST be MUST NOT

…ssible We don't want to silently ignore settings that we understand but cannot implement [1] (we *do* want to ignore settings that we don't understand [2], but that's a separate issue). This raises a slightly sticky certification issue. If a runtime *always* exits 'create' with an error: func create() err { return fmt.Errorf("nope, I cannot create that container either.") } it would be neither complaint nor non-compliant. It would not fail any MUSTs, but availing itself of the "cannot create the maintainer" option specified in this commit would mean the test suite could not test the deeper requirements around the config properties themselves. So with this change, making Microsoft certifiable will still need an explicit weakening around root.path. The easiest way to do that might be to have separate annotations for whether a setting is optional for config authors and whether it's optional for runtime authors (supported): * **`readonly`** (bool, config:optional, support:optional) ... But I'll leave hashing that out to a later commit. Regardless of the certification impact, we want to be clear that silently ignoring known parameters is wrong. [1]: /~https://github.com/opencontainers/runtime-spec/pull/476/files/9b8e21826cc9887f51f095604120cfbb788078b2#r65400731 Subject: [ Config | Root Config ] Clarify readonly [2]: opencontainers#510 Subject: Add text about extensions Signed-off-by: W. Trevor King <wking@tremily.us>

wking · 2016-09-16T15:11:31Z

Pushed 10b731e → 766abd6 fixing some “MUST not” → “MUST NOT” typos
1.

tianon · 2016-09-16T21:12:43Z

LGTM

mrunalp · 2016-09-20T21:29:55Z

LGTM

From 766abd6 (runtime.md: Require 'create' to fail if config.json asks for the impossible, 2016-09-08, opencontainers#559). Signed-off-by: W. Trevor King <wking@tremily.us>

Otherwise a runtime could silently ignore a property it didn't want to implement, which would be confusing for runtime callers [1]. This closes a potential loophole in the restriction from 766abd6 (runtime.md: Require 'create' to fail if config.json asks for the impossible, 2016-09-08, opencontainers#559). [1]: opencontainers#472 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

In 718f9f3 (minor narrative cleanup regarding config compatibility, 2017-01-30, opencontainers#673) we got: Implementations MUST error out when invalid values are encountered and MUST generate an error message and error out when encountering valid values it chooses to not support In c763e64 (config: Move valid-value rules to their own section, 2017-02-07, opencontainers#681), I'd moved that out into the current "Valid values" section with the line I'm removing in this commit. However, giving runtimes a blanket clause to ignore valid values makes it harder to use runtimes, because you can't be sure an OCI-compliant runtime supports the spec-defined value you need [1]. There have been concerns about requiring runtimes to support values which are not supported by the host system [2]. But since 766abd6 (runtime.md: Require 'create' to fail if config.json asks for the impossible, 2016-09-08, opencontainers#559) we've had runtime.md wording that gives the runtime the ability to compliantly die in those cases. That text had a wording tweak in 72e8062 (runtime: Explicitly make process.* timing implementation-defined, 2017-02-27, opencontainers#700), and is now: If the runtime cannot apply a property as specified in the configuration, it MUST generate an error and a new container MUST NOT be created. With this line removed, consumers will be able to rely on valid-value support in compliant runtimes, although many properties could use clearer runtimes-MUST-support wording for those values. However, we can sort those out on a per-property basis. And runtimes are still allowed to support extention values not defined in the spec (e.g. new capability types, or mount options, or whatever). Like all extentions, it is up to the runtime and runtime-caller to negotiate support in those cases. [1]: opencontainers#813 (comment) [2]: opencontainers#673 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

In 718f9f3 (minor narrative cleanup regarding config compatibility, 2017-01-30, opencontainers#673) we got: Implementations MUST error out when invalid values are encountered and MUST generate an error message and error out when encountering valid values it chooses to not support In c763e64 (config: Move valid-value rules to their own section, 2017-02-07, opencontainers#681), I'd moved that out into the current "Valid values" section with the line I'm removing in this commit. However, giving runtimes a blanket clause to ignore valid values makes it harder to use runtimes, because you can't be sure an OCI-compliant runtime supports the spec-defined value you need [1]. There have been concerns about requiring runtimes to support values which are not supported by the host system [2]. But since 766abd6 (runtime.md: Require 'create' to fail if config.json asks for the impossible, 2016-09-08, opencontainers#559) we've had runtime.md wording that gives the runtime the ability to compliantly die in those cases. That text had a wording tweak in 72e8062 (runtime: Explicitly make process.* timing implementation-defined, 2017-02-27, opencontainers#700), and is now: If the runtime cannot apply a property as specified in the configuration, it MUST generate an error and a new container MUST NOT be created. With this commit, consumers will be able to rely on valid-value support in compliant runtimes. Many properties could use clearer runtimes-MUST-support wording for those values, but we can sort those out on a per-property basis in follow-up work. And runtimes are still allowed to support extention values not defined in the spec (e.g. new capability types, or mount options, or whatever). Like all extentions, it is up to the runtime and runtime-caller to negotiate support in those cases. [1]: opencontainers#813 (comment) [2]: opencontainers#673 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

In 718f9f3 (minor narrative cleanup regarding config compatibility, 2017-01-30, opencontainers#673) we got: Implementations MUST error out when invalid values are encountered and MUST generate an error message and error out when encountering valid values it chooses to not support In c763e64 (config: Move valid-value rules to their own section, 2017-02-07, opencontainers#681), I'd moved that out into the current "Valid values" section with the line I'm removing in this commit. However, giving runtimes a blanket clause to ignore valid values makes it harder to use runtimes, because you can't be sure an OCI-compliant runtime supports the spec-defined value you need [1]. There have been concerns about requiring runtimes to support values which are not supported by the host system [2]. But since 766abd6 (runtime.md: Require 'create' to fail if config.json asks for the impossible, 2016-09-08, opencontainers#559) we've had runtime.md wording that gives the runtime the ability to compliantly die in those cases. That text had a wording tweak in 72e8062 (runtime: Explicitly make process.* timing implementation-defined, 2017-02-27, opencontainers#700), and is now: If the runtime cannot apply a property as specified in the configuration, it MUST generate an error and a new container MUST NOT be created. As it stands both before and after this commit, a runtime can *still* die in 'create' because it cannot apply values supported by the host. This commit is just a step towards requiring runtimes to support as many values as the host supports; it doesn't get us all the way there. Many properties could use clearer runtimes-MUST-support wording for those values, but we can sort those out on a per-property basis in follow-up work. And runtimes are still allowed to support extention values not defined in the spec (e.g. new capability types, or mount options, or whatever). Like all extentions, it is up to the runtime and runtime-caller to negotiate support in those cases. [1]: opencontainers#813 (comment) [2]: opencontainers#673 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

wking mentioned this pull request Sep 8, 2016

[ Config | Root Config ] Clarify readonly #476

Closed

jonboulle reviewed Sep 16, 2016

View reviewed changes

wking force-pushed the create-bail-for-unsupported-config branch from 10b731e to 766abd6 Compare September 16, 2016 15:08

mrunalp merged commit 313f40b into opencontainers:master Sep 20, 2016

wking deleted the create-bail-for-unsupported-config branch September 20, 2016 21:37

wking mentioned this pull request Nov 4, 2016

[Config | Hooks] Add "if supported by the platform" #472

Closed

vbatts mentioned this pull request Dec 7, 2016

Version bump for v1.0.0-rc3 #642

Merged

wking mentioned this pull request Dec 7, 2016

runtime: Fix config.md -> config.json typo #643

Merged

wking mentioned this pull request Feb 7, 2017

config: Do not allow runtimes to ignore properties defined by the spec #680

Open

wking mentioned this pull request Oct 4, 2017

config: Drop "subset of the valid values" line #927

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime.md: Require 'create' to fail if config.json asks for the impossible #559

runtime.md: Require 'create' to fail if config.json asks for the impossible #559

wking commented Sep 8, 2016

tianon commented Sep 15, 2016 •

edited by caniszczyk

Loading

wking commented Sep 15, 2016

jonboulle Sep 16, 2016

jonboulle Sep 16, 2016

wking commented Sep 16, 2016

tianon commented Sep 16, 2016 •

edited by caniszczyk

Loading

mrunalp commented Sep 20, 2016 •

edited by caniszczyk

Loading

runtime.md: Require 'create' to fail if config.json asks for the impossible #559

runtime.md: Require 'create' to fail if config.json asks for the impossible #559

Conversation

wking commented Sep 8, 2016

tianon commented Sep 15, 2016 • edited by caniszczyk Loading

wking commented Sep 15, 2016

jonboulle Sep 16, 2016

Choose a reason for hiding this comment

jonboulle Sep 16, 2016

Choose a reason for hiding this comment

wking commented Sep 16, 2016

tianon commented Sep 16, 2016 • edited by caniszczyk Loading

mrunalp commented Sep 20, 2016 • edited by caniszczyk Loading

tianon commented Sep 15, 2016 •

edited by caniszczyk

Loading

tianon commented Sep 16, 2016 •

edited by caniszczyk

Loading

mrunalp commented Sep 20, 2016 •

edited by caniszczyk

Loading