-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support running containers without CGroups #3581
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon 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 |
--cgroup-driver=none seems more consistent with Docker |
Flag change works for me; might be useful to add a cgroupv2 setting to it as well. |
LGTM |
Few things I want to do here in addition to existing changes:
|
That could have some positive impact on the code as well since we could introduce a |
☔ The latest upstream changes (presumably #3509) made this pull request unmergeable. Please resolve the merge conflicts. |
@mheon Needs a rebase. |
@mheon What is the scoop on this. Seems like something we would want? |
Blocked on I could get this landed, but restrict it to |
Is there a PR opened for runc? |
I got the |
With all of the work on getting podman to run well under a systemd unit file, would this PR help us allow better management of cgroups via systemd commands? |
Yes - though with limitations (that the Openstack folks found too limiting). It forces creation of a PID namespace (no |
Rebased and updated. This now only works with OCI runtimes that have been explicitly flagged as supporting it - in this case, Should be ready for review now. It's missing tests still, but I need to figure out a good way of doing those. |
@giuseppe @vrothberg @rhatdan Mind taking a look at the tests here to see if you can figure out what's going on? I've been poking for most of two days without success |
Seems to be tied to crun? |
contrib/cirrus/integration_test.sh
Outdated
@@ -45,6 +45,7 @@ case "$SPECIALMODE" in | |||
export OCI_RUNTIME=/usr/bin/crun | |||
make | |||
make install PREFIX=/usr ETCDIR=/etc | |||
make install.config PREFIX=/usr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is a bit off
contrib/cirrus/integration_test.sh
Outdated
@@ -57,6 +58,7 @@ case "$SPECIALMODE" in | |||
none) | |||
make | |||
make install PREFIX=/usr ETCDIR=/etc | |||
make install.config PREFIX=/usr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is a bit off
The failing kill tests look like a flake to me. I can't reproduce locally at least. They fail (error below) when trying to run the container (not trying to kill it).
A funny one is
... rerunning |
Restarted all of the three failed jobs |
Found the failure, squashed down, expecting this to go green now. |
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM but I'd like @vrothberg to take one last look before merging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for noticing it that late:
- We need changes to man pages that explain what it does along with supported values.
- I prefer changing the value from
default
toenabled
. I find it a bit more approachable thandefault
anddisabled
.
This is mostly used with Systemd, which really wants to manage CGroups itself when managing containers via unit file. Signed-off-by: Matthew Heon <matthew.heon@pm.me>
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/retest |
/hold |
/lgtm |
/hold cancel |
Presently requires
crun
to test, but I'm working up patches torunc
as well.Add support for a
--no-cgroups
flag, which disables container creation of CGroups. This is mainly intended for use with systemd unit files - you'd manage the container from a unit file (for example, one generated bygenerate systemd
), without CGroups set, so systemd has sole control of resource limits, shutdown ordering, etc.