*: Avoid creation panics when 'process' and 'linux' are unset #1726
Conversation
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 <wking@tremily.us>
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 <wking@tremily.us>
As it can be since at least opencontainers/runtime-spec@b373a155 (config: Split platform-specific configuration into its own section, 2016-05-02, opencontainers/runtime-spec#414). Signed-off-by: W. Trevor King <wking@tremily.us>
This avoids a panic for containers that do not set Process. And even if Process was set, there is no reason to require the executable to be available *at create time* [1]. Subsequent activity could be scheduled to get a binary in place at the configured location before 'start' is called. [1]: opencontainers#827 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
|
LGTM |
| @@ -186,6 +181,13 @@ func (l *linuxStandardInit) Init() error { | |||
| return newSystemErrorWithCause(err, "init seccomp") | |||
| } | |||
| } | |||
| if len(l.config.Args) == 0 { | |||
There was a problem hiding this comment.
We want to validate this on creation not after we have done all this work. Change the above code to if process == nil error out
There was a problem hiding this comment.
In other words, there is no reason to do 90% of container creation if we get to the end and we cannot run the container because process == nil
There was a problem hiding this comment.
In other words, there is no reason to do 90% of container creation if we get to the end and we cannot run the container because process == nil
The point of this PR is allowing create for containers with no process (catching up with opencontainers/runtime-spec#701). You can't start those containers, which is why we have this check here, but we certainly don't want to error those out during create.
| return fmt.Errorf("cannot use console socket if runc will not detach or allocate tty") | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func validateProcessSpec(spec *specs.Process) error { | ||
| if spec == nil { |
There was a problem hiding this comment.
this should be an error case
|
@wking Regarding the |
|
needs rebase |
|
The second and the third commits are obsoleted by
The first one and the last one doesn't make much sense to me. Yet, if needed, please open a new PR. |
processhas been optional since opencontainers/runtime-spec#701, andlinuxhas been optional since at least opencontainers/runtime-spec#414. This pull-request makes a few pivots so runc can handle something like the spec's minimal config. I actually tested with:because the runtime crashes without access to a container-side
/proc(I have syndtr/gocapability#14 open to address part of this, but other parts would take more work). With the changes in this PR,runc create testsucceeded with that configuration. It also mucked up my local terminal creation (although I rebooted without digging into exactly why), so take care when testing this yourself.There's some previous discussion on moving the
LookPathcall starting here.