config: Make process optional #701
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since be59415 (Split create and start, 2016-04-01, opencontainers#384), it's possible for a container process to never execute user-specified code (e.g. you can call 'create', 'kill', 'delete' without calling 'start'). For folks who expect to do that, there's no reason to define process.args. The only other process property required for all platforms is 'cwd', but the runtime's idler code isn't specified in sufficient detail for the configuration author to have an opinion about what its working directory should be. On Linux and Solaris, 'user' is also required for 'uid' and 'gid'. My preferred approach here is to make those optional and define defaults [1,2]: If unset, the runtime will not attempt to manipulate the user ID (e.g. not calling setuid(2) or similar). But the maintainer consensus is that they want those to be explicitly required properties [3,4,5]. With the current spec, one option could be to make process optional (with the idler's working directory unspecified) for OSes besides Linux and Solaris. On Windows, username is optional, but that was likely accidental [6]. So an unspecified 'process' would leave process.cwd and process.user unset. What that means for the implementation-defined container process between 'create' and 'start' is unclear, but clarifying how that is handled is a separate issue [7] independent of whether 'process' is optional or not. [1]: opencontainers#417 (comment) [2]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/DWdystx5X3A Subject: Exposing platform defaults Date: Thu, 14 Jan 2016 15:36:26 -0800 Message-ID: <[email protected]> [3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-04-17.00.log.html#l-44 [4]: opencontainers#417 (comment) [5]: opencontainers#417 (comment) [6]: opencontainers#618 (comment) [7]: opencontainers#700 Signed-off-by: W. Trevor King <[email protected]>
|
I would like it if we had some language in runC doesn't do this now, but I would like some language to make it clear that this is meant to be a supported workflow. |
|
On Mon, Feb 27, 2017 at 02:00:08PM -0800, Aleksa Sarai wrote:
I would like it if we had some language in `create` that states that
`process.args` should only be "checked" when you hit `start`.
Runtime validation at create-time is currently completely optional
[1]. And it is clear that subsequent changes to the configuration do
not effect the container [2].
I'm guessing that you're interested in runC's (still current?)
create-time lookup for the initial config arg [3], but as far as I can
tell “an executable at process.args[0] MUST exist at create-time is
not a spec requirement. We do require an executable at
process.args[0] by start-time [4] (which may never happen), but that
shouldn't require a create-time check.
runC doesn't do this now, but I would like some language to make it
clear that this is meant to be a supported workflow.
#489, #620, and now this PR have been my attempts at
clarifying/streamlining our support for the “I'm never going to call
‘start’” workflow. Are you missing some language for that workflow in
this PR? If so, please tell me what you want added.
Or are you looking for language to cover the “there's nothing at
process.args[0] now, but I'll have it in place by the time I call
‘start’”? If so, that seems like a separate issue.
[1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc4/runtime.md#L97
[2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc4/runtime.md#L100
[3]: opencontainers/runc#827 (comment)
[4]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc4/config.md#L130
|
|
LGTM |
1 similar comment
|
LGTM |
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
May 11, 2017
It's optional since c41ea83 (config: Make process optional, 2017-02-27, opencontainers#701) which landed yesterday. Signed-off-by: W. Trevor King <[email protected]>
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
May 11, 2017
It's optional since c41ea83 (config: Make process optional, 2017-02-27, opencontainers#701) which landed yesterday. Mrunal wanted to continue testing a config which has enough for a 'start' invocation [1], so I've kept the old JSON as minimal-for-start.json (washing it through 'make -C schema fmt' to adjust the args indenting). [1]: opencontainers#805 (comment) Signed-off-by: W. Trevor King <[email protected]>
Merged
wking
added a commit
to wking/cri-o
that referenced
this pull request
Jan 19, 2018
OCI runtime callers (like CRI-O) are allowed to leave process unset [1] for containers that they do not intend to 'start'. When we don't have any process.args, we *must* leave process unset (because process.args is required [2]). My personal preference would have been to have both process and process.args optional [3], which would have allowed for these settings to be decoupled, but that's not where the spec ended up. When we have no args and are clearing Process, we need to ensure that we don't re-create an args-less structure later on by populating process.user or similar. This commit collects later process-creating calls (e.g. setupContainerUser, which populates process.user) into the "we have some args" branch. This commit leaves earlier process-creating calls (e.g. SetProcessTerminal) where they were. Anything they do inside Process will be clobbered later if we nil it, but that's fine. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/config.md#L145 [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/config.md#L157 [3]: opencontainers/runtime-spec#701 (comment) Signed-off-by: W. Trevor King <[email protected]>
wking
added a commit
to wking/cri-o
that referenced
this pull request
Jan 19, 2018
OCI runtime callers (like CRI-O) are allowed to leave process unset [1] for containers that they do not intend to 'start'. When we don't have any process.args, we *must* leave process unset (because process.args is required [2]). My personal preference would have been to have both process and process.args optional [3], which would have allowed for these settings to be decoupled, but that's not where the spec ended up. When we have no args and are clearing Process, we need to ensure that we don't re-create an args-less structure later on by populating process.user or similar. This commit collects later process-creating calls (e.g. setupContainerUser, which populates process.user) into the "we have some args" branch. This commit leaves earlier process-creating calls (e.g. SetProcessTerminal) where they were. Anything they do inside Process will be clobbered later if we nil it, but that's fine. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/config.md#L145 [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/config.md#L157 [3]: opencontainers/runtime-spec#701 (comment) Signed-off-by: W. Trevor King <[email protected]>
wking
added a commit
to wking/cri-o
that referenced
this pull request
Jan 19, 2018
OCI runtime callers (like CRI-O) are allowed to leave process unset [1] for containers that they do not intend to 'start'. When we don't have any process.args, we *must* leave process unset (because process.args is required [2]). My personal preference would have been to have both process and process.args optional [3], which would have allowed for these settings to be decoupled, but that's not where the spec ended up. When we have no args and are clearing Process, we need to ensure that we don't re-create an args-less structure later on by populating process.user or similar. This commit collects later process-creating calls (e.g. setupContainerUser, which populates process.user) into the "we have some args" branch. This commit leaves earlier process-creating calls (e.g. SetProcessTerminal) where they were. Anything they do inside Process will be clobbered later if we nil it, but that's fine. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/config.md#L145 [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/config.md#L157 [3]: opencontainers/runtime-spec#701 (comment) Signed-off-by: W. Trevor King <[email protected]>
wking
added a commit
to wking/cri-o
that referenced
this pull request
Jan 24, 2018
OCI runtime callers (like CRI-O) are allowed to leave process unset [1] for containers that they do not intend to 'start'. When we don't have any process.args, we *must* leave process unset (because process.args is required [2]). My personal preference would have been to have both process and process.args optional [3], which would have allowed for these settings to be decoupled, but that's not where the spec ended up. When we have no args and are clearing Process, we need to ensure that we don't re-create an args-less structure later on by populating process.user or similar. This commit collects later process-creating calls (e.g. setupContainerUser, which populates process.user) into the "we have some args" branch. This commit leaves earlier process-creating calls (e.g. SetProcessTerminal) where they were. Anything they do inside Process will be clobbered later if we nil it, but that's fine. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/config.md#L145 [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/config.md#L157 [3]: opencontainers/runtime-spec#701 (comment) Signed-off-by: W. Trevor King <[email protected]>
wking
added a commit
to wking/runc
that referenced
this pull request
Feb 20, 2018
As it can be since opencontainers/runtime-spec@c41ea83d (config: Make process optional, 2017-02-27, opencontainers/runtime-spec#701). Signed-off-by: W. Trevor King <[email protected]>
wking
added a commit
to wking/runc
that referenced
this pull request
Feb 20, 2018
As it can be since opencontainers/runtime-spec@c41ea83d (config: Make process optional, 2017-02-27, opencontainers/runtime-spec#701). Signed-off-by: W. Trevor King <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since #384, it's possible for a container process to never execute user-specified code (e.g. you can call
create,kill,deletewithout callingstart). For folks who expect to do that, there's no reason to defineprocess.args.The narrow solution to this would be to make
process.argsoptional, but that approach was rejected in favor of this all-or-nothing approach toprocesshandling. What an unsetprocessmeans for the implementation-defined container process betweencreateandstartis unclear, but clarifying how that is handled is a separate issue (#700) independent of whetherprocessis optional or not.