runtime: do not require pid while created #895
Conversation
|
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
|
|
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
|
|
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 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 |
|
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
|
|
LGTM |
|
Oh i forgot the update needed to be platform specifc. |
|
What would be the suggested way to have a field that is not required in the Would it be better to just exclude this field on Windows instead as suggested above? |
|
Maybe:
|
Our current pattern is to use either “Linux” (e.g. here) or “ |
|
@wking fine with me. @darrenstahlmsft please make this change ASAP for 1.0 |
Through FIXME (whatever commit merges both opencontainers#895 and opencontainers#896). Signed-off-by: W. Trevor King <[email protected]>
Through FIXME (whatever commit merges both opencontainers#895 and opencontainers#896). Signed-off-by: W. Trevor King <[email protected]>
|
Other changes we probably want to make (from here):
|
Signed-off-by: Darren Stahl <[email protected]>
|
Updated and added changes suggested by @wking |
|
@crosbymichael I'm fine if you want to take the carry over this one. |
| Status string `json:"status"` | ||
| // Pid is the process ID for the container process. | ||
| Pid int `json:"pid"` | ||
| Pid *int `json:"pid"` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
An alternative would be to leave off the pointer and use
omitempty…
#897 is currently going this way.
There was a problem hiding this comment.
I talked to @crosbymichael offline. He's going to carry this. I'll close this PR in favour of #897
|
Closing in favour of #897 |
4 participants
It seems odd to me that the
pidis REQUIRED when thestatusiscreated, as at that time, the container process is not running. How would one know thepidfor a process that is not yet running (or created, as the lifecycle only requires thatcreatecreates 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 [email protected]