narrative cleanup in support of Base Config Compatibility #303 #673
Conversation
|
@RobDolinMS please review in the context of #303. It seems a lot of other changes have come in to establish config compatibility, this just cleans up a few nits. |
config.md
Outdated
|
|
||
| The container's top-level directory MUST contain a configuration file called `config.json`. | ||
| The canonical schema is defined in this document, but there is a JSON Schema in [`schema/config-schema.json`](schema/config-schema.json) and Go bindings in [`specs-go/config.go`](specs-go/config.go). | ||
| The canonical schema is defined in this document, but there is a JSON Schema in [`schema/config-schema.json`](schema/config-schema.json) and Go bindings in [`specs-go/config.go`](specs-go/config.go). [**`Platform-specific`**](#platform) configuration schema are defined in platform-specific documents. |
There was a problem hiding this comment.
There is already some documentation for this idea here. If you think it needs a mention in the top of this file, I'd consider just adding a link to the local #platform-specific-configuration (which actually links to the platform-specific documentation).
There was a problem hiding this comment.
That's what I intended to do, typo. Will fix, thanks.
There was a problem hiding this comment.
I also broke the 'one sentence per line' rule here, will fix.
| **`process`** (object, REQUIRED) configures the container process. | ||
|
|
||
| * **`terminal`** (bool, OPTIONAL) specifies whether a terminal is attached to that process, defaults to false. | ||
| On Linux, a pseudoterminal pair is allocated for the container process and the pseudoterminal slave is duplicated on the container process's [standard streams][stdin.3]. |
There was a problem hiding this comment.
Ok thanks for pointing that out. It seemed like an implementation note more than anything, so I just dropped it. I'll put it back for now; I'm aware of #663.
config.md
Outdated
| * **`type`** (string, REQUIRED) The filesystem type of the filesystem to be mounted. | ||
| Linux: *filesystemtype* argument supported by the kernel are listed in */proc/filesystems* (e.g., "minix", "ext2", "ext3", "jfs", "xfs", "reiserfs", "msdos", "proc", "nfs", "iso9660"). | ||
| Windows: ntfs. | ||
| Windows: corresponds to the type of file system on the volume, e.g. 'ntfs'. |
There was a problem hiding this comment.
I would be much happier if we could link to some Windows docs listing allowed types here. “e.g. ntfs” doesn't help with compliance testing (which types should get checked before a runtime is deemed compliant?).
There was a problem hiding this comment.
I thought using an example rather than implying only 'ntfs' would be better. I pinged irc for supported file systems but no reply yet. It's possible that only 'ntfs' is supported, but I thought we might see 'refs' eventually.
@RobDolinMS could you advise what should go here?
config.md
Outdated
| * **`path`** (string, REQUIRED) Specifies the path to the root filesystem for the container. | ||
| The path can be an absolute path (starting with /) or a relative path (not starting with /), which is relative to the bundle. | ||
| For example (Linux), with a bundle at `/to/bundle` and a root filesystem at `/to/bundle/rootfs`, the `path` value can be either `/to/bundle/rootfs` or `rootfs`. | ||
| The path can is either an absolute path or a relative path to the bundle. On Linux for example, with a bundle at `/to/bundle` and a root filesystem at `/to/bundle/rootfs`, the `path` value can be either `/to/bundle/rootfs` or `rootfs`. |
config.md
Outdated
|
|
||
| * **`capabilities`** (array of strings, OPTIONAL) capabilities is an array that specifies Linux capabilities that can be provided to the process inside the container. | ||
| Valid values are the strings for capabilities defined in [the man page](http://man7.org/linux/man-pages/man7/capabilities.7.html). | ||
| * **`capabilities`** (array of strings, OPTIONAL) is an array that specifies the set of capabilities of the process(es) inside the container. Valid values are platform-specific. For example, valid values for Linux are defined in [the man page](http://man7.org/linux/man-pages/man7/capabilities.7.html). |
There was a problem hiding this comment.
I think you need to say something like “OPTIONAL on Linux and Solaris, undefined on Windows” or something to get that idea across. Also, if other platforms besides Linux support process.capabilities we need to link to their documentation for allowed values, otherwise compliance testing is difficult.
There was a problem hiding this comment.
On the wording, is there a reason to label this as explicitly undefined? If it's OPTIONAL, implementations may leave it out of their scope.
As far as documentation, on reading this spec I really take the man page references to be just example references for clarity. We have various places where documentation is not explicitly referred to. If that should be part of this (or another PR), I think it would extend to more than this field. Thoughts?
There was a problem hiding this comment.
If it's OPTIONAL, implementations may leave it out of their scope.
Right, but when a Linux config sets capabilities, the runtime has to apply them in a particular way. And when a Windows config sets capabilities, the runtime doesn't have to do anything.
As far as documentation, on reading this spec I really take the man page references to be just example references for clarity. We have various places where documentation is not explicitly referred to. If that should be part of this (or another PR), I think it would extend to more than this field.
I agree that this is a bigger issue than we want to handle in this PR. I just think we need to have clearly-specified values (otherwise how do the compliance tests or folks writing a config know what values their generic OCI-compliant runtime must support?). And while I don't think we need to strengthen all of that in this PR, I'd rather not weaken anything here ;).
There was a problem hiding this comment.
For this PR, how would you feel about a generic "valid values for other platforms are as documented for that specific platform" or similar type of statement? Without some abstraction, we should probably refer to specific documentation for all platforms.
The abstraction proofs the spec against platform-specific documentation changes (content or location), which seems like an additional benefit.
config.md
Outdated
| * **`soft`** (uint64, REQUIRED) - the value that the kernel enforces for the corresponding resource. | ||
| * **`hard`** (uint64, REQUIRED) - the ceiling for the soft limit that could be set by an unprivileged process. | ||
| Only privileged process (under Linux: one with the CAP_SYS_RESOURCE capability) can raise a hard limit. | ||
| * **`type`** (string, REQUIRED) - the platform resource being limited, for example as defined in [the man page](http://man7.org/linux/man-pages/man2/setrlimit.2.html). |
There was a problem hiding this comment.
This is not “for example” on Linux. The values defined in setrlimit(2) MUST be supported by compliant runtimes. For other platforms, we need new links to their supported values, not a weakening of the Linux values.
There was a problem hiding this comment.
Again, I took this to be an example. I don't see how this infers that the field is not required nor how it weakens the spec. It's just that the specific man page reference is specific only to Linux, it serves as an example as to what can be valid for a particular platform.
This loops back to previous comment, if this spec should have references to all documentation for all configuration options on all supported platforms, it seems more work than these specific fields.
|
@opencontainers/runtime-spec-maintainers @RobDolinMS @jhowardmsft PTAL, thank you. Based on this week's discussion on the call and in IRC, I've made changes to clarify what constitutes a valid value for a platform-specific property. Note the reference at the top of the file to the platform-specific scope below. I am hoping that this abstraction is enough to allow for implementors to feel free to support a platform as they see fit, while maintaining the spec's strength. There are various nits which improve consistency and readability, I'm hoping it's simpler to review taken as a whole, rather than breaking out nits into separate PRs. |
|
|
||
| * **`capabilities`** (array of strings, OPTIONAL) capabilities is an array that specifies Linux capabilities that can be provided to the process inside the container. | ||
| Valid values are the strings for capabilities defined in [the man page](http://man7.org/linux/man-pages/man7/capabilities.7.html). | ||
| * **`capabilities`** (array of strings, OPTIONAL) is an array that specifies the set of capabilities of the process(es) inside the container. Valid values are platform-specific. For example, valid values for Linux are defined in the [CAPABILITIES(7)](http://man7.org/linux/man-pages/man7/capabilities.7.html) man page. |
There was a problem hiding this comment.
I think this is still too wishy. MUST runtimes support all those valid values? E.g. is a runtime that errors on CAP_AUDIT_READ still compliant?. What are valid values on Solaris? On Windows (now that you've removed the earlier "For Linux-based systems the process structure supports the following process specific fields:" guard)?
There was a problem hiding this comment.
I don't see that it's any different from before, only that Linux isn't the only thing referenced, and the man page isn't an implied extension of the spec. I have attempted to clarify with statements in the 'Platform-specific configuration' section. Applying those rules for all fields, not just 'capabilities', the answers to your questions are yes, valid values for Solaris are documented with the Solaris doc set, same for Windows. A runtime may support any combination of valid values for that platform, and there are rules for what it must do if it chooses to not do so.
There was a problem hiding this comment.
A runtime may support any combination of valid values for that platform, and there are rules for what it must do if it chooses to not do so.
But then you could have a “compliant” no-op runtime, which always errors out with “sorry, I don't support that”. I think it makes sense to set a minimum bar for what you have to implement before you are compliant (e.g. “Linux runtimes MUST implement at least these capabilities: …”. I'm ok with that minimum bar being bounded by the local platform (e.g. a compliant runtime can error on CAP_X if and only if the kernel doesn't support CAP_X), but I don't have a good way to word that requirement generically.
There was a problem hiding this comment.
I think we might be getting a bit contrived with a runtime that implements nothing... for example, no file system types for mounts are supported by choice. That said, this isn't meant to be a design document. I would say that as long as a clear error message is returned for any valid value for a field, yes it's spec-compliant.
There was a problem hiding this comment.
I would say that as long as a clear error message is returned for any valid value for a field, yes it's spec-compliant.
That's an internally consistent approach, although it makes “OCI runtime-spec compliant” a pretty cheap badge, and means the only way to know that a runtime will be able to handle a given config will be “try to use the runtime to launch that config”. If the maintainers want to go in this direction, they may want to re-evaluate the effort currently being put into runtime-tools' compliance testing.
There was a problem hiding this comment.
I think this is fine. Again, if further linting is required by an application to ensure a host has the expected capabilities, that is not going to be defined here.
There was a problem hiding this comment.
What @vbatts said. You cannot base compliance on system values @wking and you cannot just make up some minimum. It can support as many values as the underlying host supports for fields like this. Just because you use a runtime on an older kernel that doesn't have a CAP does not mean its no longer a compliant runtime.
LGTM @jlbutler
There was a problem hiding this comment.
Just because you use a runtime on an older kernel that doesn't have a CAP does not mean its no longer a compliant runtime.
That's not the situation I'm concerned about. I'm concerned about the situation where the host does support a particular CAP_* (or whatever) but the runtime does not. Instead of “it can support…”, I'd rather have “it MUST support values from {some values} for values supported by the host platform”. That bases compliance on system values (which you don't like), but means that you can actually test runtimes for meaningful compliance (e.g. the no-op runtime would be non-compliant).
There was a problem hiding this comment.
@wking then the runtime errors out. end-users investigate. complain to the runtime developers or write a fix. That kind-of linting and validation needs to be secondary to this spec.
There was a problem hiding this comment.
then the runtime errors out. end-users investigate. complain to the runtime developers or write a fix. That kind-of linting and validation needs to be secondary to this spec.
So what kind of CAP_* values should runtime compliance testing use? Currently it sets these, and fails runtimes which die in create (currently start, because runtime-tools hasn't been ported around #384, opencontainers/runtime-tools#285) or run without a matching set. Will we update that to say that any runtime is compliant as long as it:
- Dies in
createor - Succeeds in
createand has a container environment matching the configuration
And then test all combinations (or some sampling) of CAP_* values regardless of platform support?
|
Good stuff, so far. I'm still reviewing it tho. |
config.md
Outdated
|
|
||
| The container's top-level directory MUST contain a configuration file called `config.json`. | ||
| The canonical schema is defined in this document, but there is a JSON Schema in [`schema/config-schema.json`](schema/config-schema.json) and Go bindings in [`specs-go/config.go`](specs-go/config.go). | ||
| Platform-specific configuration schema are defined in [**`platform-specific documents`**](#platform-specific-configuration) linked below. |
There was a problem hiding this comment.
We probably don't need backticks or bold around “platform-specific documents”. The usual link styling makes the link-ness obvious enough.
| Windows: ntfs. | ||
| Solaris: corresponds to "type" of the fs resource in zonecfg(8). | ||
| * Linux: valid *filesystemtype* supported by the kernel as listed in */proc/filesystems* (e.g., "minix", "ext2", "ext3", "jfs", "xfs", "reiserfs", "msdos", "proc", "nfs", "iso9660"). | ||
| * Windows: the type of file system on the volume, e.g. "ntfs". |
There was a problem hiding this comment.
yea. This allows for better future-proofing.
|
|
||
| * **`capabilities`** (array of strings, OPTIONAL) capabilities is an array that specifies Linux capabilities that can be provided to the process inside the container. | ||
| Valid values are the strings for capabilities defined in [the man page](http://man7.org/linux/man-pages/man7/capabilities.7.html). | ||
| * **`capabilities`** (array of strings, OPTIONAL) is an array that specifies the set of capabilities of the process(es) inside the container. Valid values are platform-specific. For example, valid values for Linux are defined in the [CAPABILITIES(7)](http://man7.org/linux/man-pages/man7/capabilities.7.html) man page. |
There was a problem hiding this comment.
I think this is fine. Again, if further linting is required by an application to ensure a host has the expected capabilities, that is not going to be defined here.
config.md
Outdated
| [**`platform.os`**](#platform) is used to lookup further platform-specific configuration. | ||
| [**`platform.os`**](#platform) is used to specify platform-specific configuration. | ||
| Runtime implementations MAY support any valid values for platform-specific fields as part of this configuration. | ||
| Implementations MUST generate a clear error message for valid values it chooses to not support and MUST generate an error when invalid values are encountered. |
There was a problem hiding this comment.
does "a clear error message" mean that the implementation would not continue running and would exit non-zero? Just curious.
There was a problem hiding this comment.
does "a clear error message" mean that the implementation would not continue running and would exit non-zero?
Exiting non-zero on create errors is covered by the in-flight #513. The currently landed error language has “as if the operation were never attempted” wording and “MUST generate an error and a new container MUST NOT be created” wording, which seems to cover everything but the exit-code (which only makes sense in the context of the #513 API).
config.md
Outdated
| [**`platform.os`**](#platform) is used to lookup further platform-specific configuration. | ||
| [**`platform.os`**](#platform) is used to specify platform-specific configuration. | ||
| Runtime implementations MAY support any valid values for platform-specific fields as part of this configuration. | ||
| Implementations MUST error out when invalid values are encountered and MUST generate a clear error message and exit non-zero when encountering valid values it chooses to not support. |
There was a problem hiding this comment.
I think we want to drop “exit non-zero”, because config.md should be independent of the API used to invoke the runtime. “generate a clear error message” is generic, and the relationship between error generation and exit-code should be setup in the API docs for APIs which use exit-codes.
And “clear” is hard to test. Maybe make it “MUST generate an error”? Then we'd trust on runtime authors to make it clear. If we don't trust runtime authors, we should say something enforceable, but that's hard to do when we don't even require the runtime to expose error to the user.
There was a problem hiding this comment.
I believe I've covered these concerns in the latest commit. Thanks!
Signed-off-by: Jesse Butler <[email protected]>
|
LGTM |
1 similar comment
|
LGTM |
Some minor narrative nits cleaned up in support of resolving issue #303. Most changes attempt to make it clear that multiple platforms are supported but keep the Linux examples and references in place. In the Process section, Linux-only options were left under the original header, anything which is supported by at least one other platform was pulled out.