Fix setup cgroup before prestart hook #1239
Conversation
|
I'm not convinced this is correct. The reason why cgroups were written to be applied that late is so that hooks and other |
|
@moypray Can't you setup the cgroups in your config.json? |
|
@cyphar , I think the hook is not under control of container cgroup. because hook is not a child process of 'runc init' but parent runC. I think if the "runc init" meet the limit of cgroup should be ok. Is that right? |
|
@mrunalp actually, if I use the config of container, will bring lots of troubles: such as config should be persistent and uniformity. (docker daemon will keep a cache in memory, I need to modify lots of codes to fix this, and would break the origin docker interface, It is terrible). But thanks all the same. |
|
@cyphar hook processes are invoked by runc, they are not constrained by container resources. Or are you concerning that the manipulation of cgroup resources by hooks could be malicious? I think it's reasonable that we allow prestart hooks to customize cgroup resources. |
|
My concern is that a hook could do something like
In which case this patch should drop |
If hook wants to run processes in container, why should they not be constrained by cgroup resources? What if that a long-running forked process which can live with container processes?
I think the point is that hook authors should be responsible for the containers, most time they should be the same authors, like we permitted hooks to mess with container mounts.
This PR very likely needs to be amended, I haven't look it through, just talking about design first :) |
Fair enough. I'm just worried about people deciding to use hooks as a way to avoid touching the OCI specification (like the PR author is trying to do). But ultimately some usecases are more complicated than what can be described in |
|
@cyphar , sorry for misunderstanding, I think the hook is running on host, the same with docker, runc. and customized by service provider themselves. We should trust the hook. |
|
ping @opencontainers/runc-maintainers , How do you feel it, just talking about the design, not this PR implement, thx~ |
|
@moypray Most maintainers are on holidays now (including myself). I'd wait for a while before pinging people again, just because everyone is AFK at the moment. 😉 |
|
@cyphar , OK, thanks for your replay. |
|
@opencontainers/runc-maintainers , what do you think of this proposal?
@hqhq I have done some testing based on my fix. And I am sure that there will not be more go routines or threads created between the two positions of 'Set cgroup' operation. So it will not affect the pid cgroup. |
|
@opencontainers/runc-maintainers PTAL, design and code both look good to me. |
|
See also the original issue: #1044 |
|
@mrunalp This is what I was referring to for the GPU support in Kubernetes |
libcontainer/process_linux.go
Outdated
| // Setup cgroup before prestart hook, so that the prestart hook could apply cgroup permissions. | ||
| if err := p.manager.Set(p.config.Config); err != nil { | ||
| return newSystemErrorWithCause(err, "setting cgroup config for ready process") | ||
| } |
There was a problem hiding this comment.
NIT. Can we have slightly different error message text so it can be more easily determined where the error came from? Perhaps appending " in procReady" to the first message at line 339 and " in procHooks" here in the second?
|
@3XX0 Would you be willing to carry this over to a new rebased PR? |
|
@moypray Please rebase, thanks. |
|
LGTM |
* User Case: User could use prestart hook to add block devices to container. so the hook should have a way to set the permissions of the devices. Just move cgroup config operation before prestart hook will work. Signed-off-by: Wentao Zhang <[email protected]>
|
@mrunalp Sorry, saw it a little late ;) |
|
Ping @mrunalp |
|
LGTM |
Upstream reference: opencontainers/runc#1239 Fixes: stefwalter/oci-kvm-hook#3 * User Case: User could use prestart hook to add block devices to container. so the hook should have a way to set the permissions of the devices. Just move cgroup config operation before prestart hook will work. Signed-off-by: Antonio Murdaca <[email protected]>
Upstream reference: opencontainers/runc#1239 Fixes: stefwalter/oci-kvm-hook#3 * User Case: User could use prestart hook to add block devices to container. so the hook should have a way to set the permissions of the devices. Just move cgroup config operation before prestart hook will work. Signed-off-by: Antonio Murdaca <[email protected]>
Upstream reference: opencontainers/runc#1239 Fixes: stefwalter/oci-kvm-hook#3 * User Case: User could use prestart hook to add block devices to container. so the hook should have a way to set the permissions of the devices. Just move cgroup config operation before prestart hook will work. Signed-off-by: Antonio Murdaca <[email protected]>
User could use prestart hook to add block devices to container. so the
hook should have a way to set the permissions of the devices.
Just move cgroup config operation before prestart hook will work.
Signed-off-by: Wentao Zhang [email protected]