Improvements to how runc exec is handled
#4592
Conversation
| state, err := container.State() | ||
| if err != nil { | ||
| return -1, err | ||
| } | ||
| bundle, ok := utils.SearchLabels(state.Config.Labels, "bundle") | ||
| if !ok { | ||
| return -1, errors.New("bundle not found in labels") | ||
| } |
There was a problem hiding this comment.
this is replaced with
bundle, ok := utils.SearchLabels(c.Config().Labels, "bundle")
if !ok {
return nil, errors.New("bundle not found in labels")
}(see below)
| // spec and stdio from the current process. | ||
| func newProcess(p specs.Process) (*libcontainer.Process, error) { | ||
| // newProcess converts [specs.Process] to [libcontainer.Process]. | ||
| func newProcess(p *specs.Process) (*libcontainer.Process, error) { |
There was a problem hiding this comment.
Maybe it also makes sense to make this a public method (and move to libcontainer).
Something like libcontainer.ProcessFromSpec maybe
There was a problem hiding this comment.
LGTM, thanks @kolyshkin!
Just curious, using the Config() method that retuned a new struct, was actually causing allocations or is go smart enough to optimize it and only return by copy the UID/GID? I mean, the struct didn't change, so it can at least realize it doesn't need to allocate it all the times.
Although I agree it is simpler this way, I'm curious if you did take a look at that. Don't take a look for my comment only :)
Let's move some code from execProcess to newProcess, fixing the
following few issues:
1. container.State (which does quite a lot) is not needed --
we only need container.Config here.
2. utils.SearchLabels is not needed when "runc exec --process" is used.
3. Context.String("process") is called twice.
4. It is not very clear from the code why checking for
len(context.Args()) is performed. Move the check to just before
Args is used, to make it clear why.
Signed-off-by: Kir Kolyshkin <[email protected]>
Every time we call container.Config(), a new copy of struct Config is created and returned, and we do it twice here. Accessing container.config directly fixes this. Fixes: 805b8c7 ("Do not create exec fifo in factory.Create") Signed-off-by: Kir Kolyshkin <[email protected]>
The rootuid and rootgid are only needed when detach and createTTY are both false. We also call c.Config() twice, every time creating a copy of struct Config. Solve both issues by passing container pointer to setupIO, and get rootuid/rootgid only when we need those. Signed-off-by: Kir Kolyshkin <[email protected]>
1. Pass an argument as a pointer rather than copying the whole structure. It was a pointer initially, but this has changed in commit b2d9d99 without giving a reason why. 2. The newProcess description was added by commit 9fac183 (yes, the very first one) and hasn't changed since. As of commit 29b139f, the part of it which says "and stdio from the current process" is no longer valid. Remove it, and while at it, rewrite the description entirely. Signed-off-by: Kir Kolyshkin <[email protected]>
Well, I spent a few minutes trying to reproduce this and you are right, Go seems to optimize this out (or my benchmark is gravely wrong). It's late here so I'll take a fresh look tomorrow. |
|
OK, I wrote a separate benchmark to demonstrate the improvement, you can find it here: https://gist.github.com/kolyshkin/98ff9ff8de39e3e548bc84808e3d10b8 Here are the results on my machine: (the "WithLocal" case is an optimization to not call |
A few small and specific patches around how
runc execis being set up.No changes to functionality, relatively easy to review.
Please see individual commit descriptions for details.