config: Make 'process.args' optional #620

wking · 2016-11-11T05:08:31Z

Since #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, theres no reason to define process.args`.

The main point of #489 without the surrounding cleanup, since most of the cleanup has since landed via:

The one-sentence-per-line fixes in Rewrite mounts description in config.md #443.
The line calling out process as required from config: Synchronize comments between Markdown and Go #525.

There was some negative review of this change in #489, although @mrunalp acknowledged support for the “never calling start” workflow. If there is a maintainer consensus around that workflow being supported, requiring process.args in all cases seems like needless hoop-jumping. I'm fine with validation tooling warning users “you might have forgotten to put something meaningful in process.args, so start calls will fail”. But I'd rather not have this spec require dummy process.args from folks who will never call start.

WeiZhang555 · 2016-11-14T09:09:21Z

Well, I think if

If process.args was not configured, the runtime MUST generate an error.

Then it should make more sense to keep it REQUIRED.

although @mrunalp acknowledged support for the “never calling start” workflow.

If the user code will never be started, you can specify anything in args such as 'args: ["dummyCode"]', I don't see any benefits from removing the required args field from spec. On the other hand, we should detect missing args and report error earlier to user.

so -1 on this.

wking · 2016-11-14T15:21:59Z

On Mon, Nov 14, 2016 at 01:09:22AM -0800, zhangwei_cs wrote:

although @mrunalp acknowledged support for the “never calling
start” workflow.

If the user code will never be started, you can specify anything in
args such as 'args: ["dummyCode"]'…

That's not legal, because we currently require the executable to exist
1. If you have an empty rootfs, you'd have to use "/" 2.

On the other hand, we should detect missing args and report error
earlier to user.

I'm fine having validation tooling that warns (or errors, as long as
it's not the compliance-testing invocation) on missing process.args.
I just don't see a point to requiring:

{"process": {"args": ["/"], …}, …}

for folks who will never call ‘start’.

mikebrow · 2017-01-10T17:34:37Z

runtime.md

 Attempting to start a container that does not exist MUST generate an error.
 Attempting to start an already started container MUST have no effect on the container and MUST generate an error.
 This operation MUST run the user-specified code as specified by [`process`](config.md#process-configuration).
+If `process.args` was not configured, the runtime MUST generate an error.


If the process.args array is not specified or if the user-specified code cannot be found or started the runtime MUST generate an error.

I'd rather not make that change. The execution will usually be by a call to execve(2) or similar, and while that returns errors like ENOENT, ENOEXEC, etc. on failure, there's no way to get it to return something on success (because by that point the process will be running the user-specified program). And start is just signalling an external process to execute code (e.g. see opencontainers/runc#886, where the start process drains a fifo to trigger an execve in the container process). So I don't see a way to block on potential execve errors in the start process short of ptrace(2) monitoring, and that seems like too high a cost.

I'd rather have start error out if it failed to trigger the start attempt (which the container process can confirm with “start received. I'm about to execve, wish me luck”). Folks who are curious about whether that execve succeeded can either ptrace the container process themselves, or monitor state, etc.

Yeah, I wasn't looking for failures in the process started, I'm looking for failure to specify a runnable executable as arg0 of the process.args array. At least change it to:

If the process.args array does not at least include a first argument specifying the user-specified code the runtime MUST generate an error.

Otherwise we need some sort of default.. description. No?

That sounds like a subset of the current process.args requirements, so it would fall under the “MAY validate” portion of create. Do we need a “MUST validate process” (or just process.args?) clause in start? My personal preference would be to leave validation up to the caller, who can run bundle-validation tools whenever they want. I'm happy to make that an explicit part of the start spec, if folks think it would help. I'll grudgingly make a MUST-validate-whatever clause a part of the start spec if that's the maintainer consensus ;).

Otherwise we need some sort of default.. description.

With optional validation, runtimes that choose not to validate in start will have this sort of miss-configuration result in the same sort of error-workflow that applies to execve errors. I don't see a need for a default, and am not sure what sort of default you'd pick in any case.

Yeah I didn't see a good default either... certainly would not have picked "/" :)

I thought the point was start does not have to be called.

Yes.

… thus we only have this MUST when start is called.

That's what I was going for with an optional process.args which still contains a MUST for an initial argument. That requirement (if process.args is set, it MUST contain that leading entry) applies to all configs, not just when start is called.

changing is not configured to is not set is more understandable to me.
also maybe we should use the same set of phrases above ? maybe This operation MUST generate an error if process.args is not set

maybe This operation MUST generate an error if process.args is not set

Done (and rebased onto master) with 35ee63b → 7071706, although I used “was” instead of “is” because what matters is what was in the configuration at create-time.

mikebrow · 2017-01-11T18:33:18Z

runtime.md

 Attempting to start a container that does not exist MUST generate an error.
 Attempting to start an already started container MUST have no effect on the container and MUST generate an error.
 This operation MUST run the user-specified program as specified by [`process`](config.md#process).
+This operation MUST generate an error if `process.args` was not set.


The process.args array is currently "OPTIONAL", not REQUIRED. So if it's optional we need to explain why/when its REQUIRED. I know you know why, but it does not explicitly say why / when. The MUST requirement being in the START command paragraph "implies" that the process.args array (first element) is REQUIRED only when START is used but is not explicit or explained. The word "set" does not express explicitly what you mean. I only mention this because I believe MUST language should only have one interpretation.

After some discussion on IRC, @mikebrow recommended the following addition to the process.args docs:

When start is used process.args is no longer OPTIONAL and MUST be set.

There's also a similar conditional requirement in the device spec, but “REQUIRED unless you won't call start” and “OPTIONAL unless you'll call start in which case it's REQUIRED” both feel too awkward.

With 7071706 → e45ddba I've added:

When start will be called, this property is REQUIRED.

which gets the cross-reference to the start command. It replaces @mikebrow's “is no longer OPTIONAL and MUST be set” with “is REQUIRED”, because I like “REQUIRED” better than “MUST be set”. I think “is no longer OPTIONAL” is fairly clear from the context, but I'm ok adding that back in if folks feel like it's necessary. And I've replaced “is used” with “will be called” because you need to set this field at create time in order to use it when start is called.

mikebrow · 2017-01-11T22:20:31Z

Looks good to me.

dqminh · 2017-01-11T22:26:03Z

config.md

-* **`args`** (array of strings, REQUIRED) with similar semantics to [IEEE Std 1003.1-2001 `execvp`'s *argv*][ieee-1003.1-2001-xsh-exec].
+* **`args`** (array of strings, OPTIONAL) with similar semantics to [IEEE Std 1003.1-2001 `execvp`'s *argv*][ieee-1003.1-2001-xsh-exec].
  This specification extends the IEEE standard in that at least one entry is REQUIRED, and that entry is used with the same semantics as `execvp`'s *file*.
+  When [`start`](runtime.md#start) will be called, this property is REQUIRED.


This property is required when start is called seems better for me.

Done with e45ddba → bebdd29.

when works..

RobDolinMS · 2017-01-11T22:31:13Z

@opencontainers/runtime-spec-maintainers PTAL

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 it's not clear how intentional that was [6]. [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 Signed-off-by: W. Trevor King <[email protected]>

Now that d43fc42 (config-linux: Lift no-tweaking namespace restriction, 2017-01-11, opencontainers#649) allows us to get into this sort of situation. This sort of ownership may also apply to other resources (cgroups?), but we can handle them in follow-up commits. Also drop "Configuration" from the root header. Everything in that file is a configuration. container-namespace3 (instead of container-namespace) supports the single-page, Pandoc-generated file (see e7be40f, Cleanup the spec a bit to remove WG/git text that's not really part of the spec, 2016-11-14, opencontainers#626). Using an informative suggestion was recommended by Dao Quang Minh [1]. I've made the config JSON as small as possible while keeping it valid, but there's still an unfortunate amount of boilerplate there. There is in-flight work to let us at least drop process.args [2]. [1]: opencontainers#651 [2]: opencontainers#620 Signed-off-by: W. Trevor King <[email protected]>

Now that d43fc42 (config-linux: Lift no-tweaking namespace restriction, 2017-01-11, opencontainers#649) allows us to get into this sort of situation. This sort of ownership may also apply to other resources (cgroups?), but we can handle them in follow-up commits. Using an informative suggestion was recommended by Dao Quang Minh [1]. I've made the config JSON as small as possible while keeping it valid, but there's still an unfortunate amount of boilerplate there. There is in-flight work to let us at least drop process.args [2]. The new mount namespace in the UTS example avoids pivoting the host namespace's root. Also drop "Configuration" from the root header. Everything in that file is a configuration. container-namespace3 (instead of container-namespace) supports the single-page, Pandoc-generated file (see e7be40f, Cleanup the spec a bit to remove WG/git text that's not really part of the spec, 2016-11-14, opencontainers#626). [1]: opencontainers#651 [2]: opencontainers#620 Signed-off-by: W. Trevor King <[email protected]>

crosbymichael · 2017-02-02T21:51:02Z

If we want to be pedantic basically all of the process is not needed if you don't specify args or do start.

wking · 2017-02-02T22:22:01Z

If we want to be pedantic basically all of the process is not needed if you don't specify args or do start.

But there are affects to not specifying process even if you do not call start. For example, process.user has no default so it's unclear what user the runtime's pre-start container process would be running as. I'm fine defining a default user to cover that case, but there has been resistance to that in the past.

However the process.user discussion works out, making process.args optional seems like a non-controversial first step.

crosbymichael · 2017-02-16T00:02:46Z

Overall, if we want to do this, I would just make process optional and be done with it. The init is always launched as root or whatever the userns mapping is and we don't change. It would be a fully privileged process in the container however and there is not a good way around that as we need to retain the caps for the finalizing step of the container.

Its not really a use case that we want to support but it can be done for advanced users that want to fire off many execs without fully creating a container and it up to the runtime and implementer to secure things, we cannot guarantee anything.

What do the other maintainers think?

crosbymichael · 2017-02-27T19:15:44Z

After a brief chat with maintainers in IRC, we are going to close this specific modification. Just having args as optional makes no sense at all. Either the entire process section should be optional for what you want to do or it should be left as is but this change is not ideal for any usecase.

Based on IRC discussion today (times in PST) [1]: 11:36 < crosbymichael> just take a step back and think about it. you have a process object in the spec. its a single object defining what to run. How do you run a process? you exec its args. From the spec pov its an atomic operation. in between create and start its not running the users code and is left up to the runtime. you either have a process defined by the spec and its created as an operation in the container on start or your dont. This means that the caller has no way to set the user/cwd/capabilities/… of the runtime's container process between 'create' and 'start'. You could avoid that limitation by requiring all process properties *except* process.args be applied at create-time, but my attempt to make process.args optional (which would have allowed that interpretation without burdening callers who never intended to call 'start') was rejected in favor of this all-or-nothing approach to 'process' handling [2]. [1]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2017-02-27.log.html#t2017-02-27T19:35:35 [2]: opencontainers#620 (comment) Signed-off-by: W. Trevor King <[email protected]>

Based on IRC discussion today (times in PST) [1]: 11:36 < crosbymichael> just take a step back and think about it. you have a process object in the spec. its a single object defining what to run. How do you run a process? you exec its args. From the spec pov its an atomic operation. in between create and start its not running the users code and is left up to the runtime. you either have a process defined by the spec and its created as an operation in the container on start or your dont. With the previous wording, it was unclear how large a hole we were poking with "the user-specified program MUST NOT be run at this time". This commit removes that ambiguous wording and replaces it with an explicit reference to 'process.args'. It makes it clear that everything outside of 'process' MUST happen at create-time. And it leaves all of 'process' except for 'process.args' up to the implementation. This means that the caller has no reliable way to set the user/cwd/capabilities/… of the runtime's container process between 'create' and 'start'. You could avoid that limitation by requiring all process properties *except* process.args be applied at create-time, but my attempt to make process.args optional (which would have allowed that interpretation without burdening callers who never intended to call 'start') was rejected in favor of this all-or-nothing approach to 'process' handling [2]. [1]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2017-02-27.log.html#t2017-02-27T19:35:35 [2]: opencontainers#620 (comment) Signed-off-by: W. Trevor King <[email protected]>

wking mentioned this pull request Nov 11, 2016

config: Make 'process.args' optional #489

Closed

wking mentioned this pull request Nov 16, 2016

Transition from Markdown to AsciiDoc #628

Closed

mikebrow reviewed Jan 10, 2017

View reviewed changes

wking force-pushed the optional-process.args-v2 branch from 35ee63b to 7071706 Compare January 11, 2017 18:24

mikebrow reviewed Jan 11, 2017

View reviewed changes

wking force-pushed the optional-process.args-v2 branch from 7071706 to e45ddba Compare January 11, 2017 18:52

dqminh reviewed Jan 11, 2017

View reviewed changes

RobDolinMS added this to the 1.0.0 milestone Jan 11, 2017

wking force-pushed the optional-process.args-v2 branch from e45ddba to bebdd29 Compare January 11, 2017 23:58

crosbymichael closed this Feb 27, 2017

This was referenced Feb 27, 2017

runtime: Explicitly make process.* timing implementation-defined #700

Merged

config: Make process optional #701

Merged

config: Make 'process.args' optional #620

config: Make 'process.args' optional #620

Uh oh!

Conversation

wking commented Nov 11, 2016

Uh oh!

WeiZhang555 commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking commented Nov 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow commented Jan 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobDolinMS commented Jan 11, 2017

Uh oh!

crosbymichael commented Feb 2, 2017

Uh oh!

wking commented Feb 2, 2017

Uh oh!

crosbymichael commented Feb 16, 2017

Uh oh!

crosbymichael commented Feb 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

WeiZhang555 commented Nov 14, 2016 •

edited

Loading

mikebrow Jan 10, 2017 •

edited

Loading

mikebrow Jan 11, 2017 •

edited

Loading

wking Jan 11, 2017 •

edited

Loading