runtime: do not require pid while created

[darstahl]

It seems odd to me that the pid is REQUIRED when the status is created, as at that time, the container process is not running. How would one know the pid for a process that is not yet running (or created, as the lifecycle only requires that create creates the namespace, and prohibits it from creating the process)?

If this is not the case, then the language for container process should be clarified.

Signed-off-by: Darren Stahl darst@microsoft.com

[wking]

On Wed, Jul 12, 2017 at 08:26:03PM +0000, Darren Stahl wrote: It seems odd to me that the `pid` is REQUIRED when the `status` is `created`, as at that time, the container process is not running.

It *is* running. The container process MUST be running by the time ‘create’ finishes, for example, to hold open a new PID namespace. The difference between ‘created’ and ‘running’ is just whether that container process has executed the user-specified program or not [1]. So the clone(2) (or fork(2) or whatever) that creates the *process* happens in ‘create’. And the execvpe(3) (or whatever) that executes the user-specified program happens in ‘start’. Without a container process (running a runtime-supplied program) running between ‘create’ and ‘start’, doing things like custom cgroup management is impossible (no container PID to add to the cgroup your setting up). I think we can close this PR. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc6/runtime.md#L20

[wking]

I gave a Linux-centrix answer to a potentially Windows-motivated issue [1]. Getting a bit more generic, the reason we supply ‘pid’ in the state JSON is so runtime-callers can reach around the runtime to do whatever setup they need without having to go through a hook (lots more on this in #384). I'm not sure how that decomposes into Windows primitives, but without something PID-ish, I don't see how Windows users are supposed to manipulate their container when they need/want to reach around the runtime and touch things directly. Maybe that reaching around is not going to be supported on Windows? If so, we probably just want to make the ‘pid’ entry here POSIX-platform specific. Or maybe Linux specific, since the Solaris folks had previously said something about a start-time process as well [2]. [1]: #895 (comment) [2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-07-13-17.03.log.html#l-24

[darstahl]

Yes, this was a Windows motivated PR, but without the context of the separation of create and start, it seemed odd that a PID would exist before the process. There is no runtime supplied container process on Windows. The user-specified program in Windows containers (both Hyper-V and Windows Server) must be a new process created at the time of start as a Windows containers do not have a analogy to execvpe(3).

I can't comment on if going behind the back of the Windows runtime will be supported or not, but that would be done using the container id rather than pid.

[wking]

On Wed, Jul 12, 2017 at 08:55:13PM +0000, Darren Stahl wrote: I can't comment on if going behind the back of the Windows runtime will be supported or not, but that would be done using the container `id` rather than `pid`.

On Linux, the container ID is a userspace concept maintained by the runtime. On Windows and Solaris, container IDs seem to be kernel primitives. I think that's a reasonable argument for making ‘pid’ Linux-only (which would also obsolete #894). It would make some things (like prctl calls to configure Solaris container processes between ‘create’ and ‘start’ [1]) impossible, but we can always add it back in later on. On the other hand, if we keep ‘pid’ required for 1.0, then relaxing that requirement later is more likely to annoy state consumers (although the state JSON is not formally covered by runtime-spec's SemVer [2]). [1]: https://docs.oracle.com/cd/E23824_01/html/821-1460/rmctrls.task-33.html [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc6/config.md#L17

[vbatts]

LGTM

[vbatts]

Oh i forgot the update needed to be platform specifc.

[darstahl]

What would be the suggested way to have a field that is not required in the created state on Windows, but is still required in the running state?

Would it be better to just exclude this field on Windows instead as suggested above?

[crosbymichael]

Maybe:

(int, REQUIRED when statusiscreated under linux, OPTIONAL on other platforms) is the ID of the container process, as seen by the host.

[wking]

Maybe:

(int, REQUIRED when status is created under linux, OPTIONAL on other platforms) is the ID of the container process, as seen by the host.

Our current pattern is to use either “Linux” (e.g. here) or “linux” (e.g. here). I'd rather not use a lowercase “linux” without backticks. Otherwise that wording looks good to me.

[crosbymichael]

@wking fine with me. @darrenstahlmsft please make this change ASAP for 1.0

[wking]

Other changes we probably want to make (from here):

• add a pointer to the Go property.
• remove pid from the required section of the JSON Schema.

[darstahl]

Updated and added changes suggested by @wking

[darstahl]

@crosbymichael I'm fine if you want to take the carry over this one.

[wking]

An alternative would be to leave off the pointer and use omitempty, since zero isn't a valid PID. Which way we should go depends on the unresolved #772, but the maintainers may have a one-off opinion on this.

[wking]

An alternative would be to leave off the pointer and use omitempty…

#897 is currently going this way.

[darstahl]

I talked to @crosbymichael offline. He's going to carry this. I'll close this PR in favour of #897

[darstahl]

Closing in favour of #897