cgroup: always honor --cgroup-parent #10177
Conversation
openshift-ci-robot
added
the
approved
giuseppe
force-pushed
the
always-honor-cgroup-parent
branch
from
66653cd
to
81fddb8
Compare
libpod/container_internal_linux.go
Outdated
| @@ -2224,12 +2224,11 @@ func (c *Container) getOCICgroupPath() (string, error) { | |||
| } | |||
| cgroupManager := c.CgroupManager() | |||
| switch { | |||
| case c.config.CgroupParent != "": | |||
There was a problem hiding this comment.
I think we should avoid this for the systemd cgroup manager
giuseppe
force-pushed
the
always-honor-cgroup-parent
branch
8 times, most recently
from
1db5d2c
to
a69caee
Compare
|
green tests |
| @@ -2225,7 +2225,7 @@ func (c *Container) getOCICgroupPath() (string, error) { | |||
| cgroupManager := c.CgroupManager() | |||
| switch { | |||
| case (rootless.IsRootless() && (cgroupManager == config.CgroupfsCgroupsManager || !unified)) || c.config.NoCgroups: | |||
| return "", nil | |||
| return c.config.CgroupParent, nil | |||
There was a problem hiding this comment.
Do we risk breaking existing rootless containers that have CgroupParent set to the default?
There was a problem hiding this comment.
good point.
Not sure what is the best way to deal with it. Should we stop setting it if it is the default value (perhaps until podman 4.x)?
There was a problem hiding this comment.
Is there any way we can do an access check on the cgroup, see if we have the ability to use it without error? If not, then your suggestion is definitely the best way forward.
There was a problem hiding this comment.
I think that will be error prone and difficult to get right. We won't only have to check if the current user has permissions to the cgroup, but that any ID mapped in the user namespace has it. And that will be even more complicated with cgroup v1.
Looking more into it, I don't think it will be a problem if we just check for c.config.CgroupParent == CgroupfsDefaultCgroupParent as that cgroup is usually owned by root.
What do you think about the new version?
EDIT: we don't really need to check for IDs mapped in the user namespace, since the cgroup is not configured from the container user namespace (if it has any)
giuseppe
force-pushed
the
always-honor-cgroup-parent
branch
4 times, most recently
from
bb109c3
to
3fcdb90
Compare
if --cgroup-parent is specified, always honor it without doing any detection whether cgroups are supported or not. Closes: containers#10173 Signed-off-by: Giuseppe Scrivano <[email protected]>
giuseppe
force-pushed
the
always-honor-cgroup-parent
branch
from
3fcdb90
to
17ce567
Compare
|
LGTM |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
github-actions
bot
added
the
locked - please file new issue/PR
if --cgroup-parent is specified, always honor it without doing any
detection whether cgroups are supported or not.
Closes: #10173
Signed-off-by: Giuseppe Scrivano [email protected]