Skip to content
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

Merged
merged 1 commit into from
Sep 10, 2019

Conversation

mheon
Copy link
Member

@mheon mheon commented Jul 16, 2019

Presently requires crun to test, but I'm working up patches to runc 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 by generate systemd), without CGroups set, so systemd has sole control of resource limits, shutdown ordering, etc.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 16, 2019
@openshift-ci-robot
Copy link
Collaborator

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L labels Jul 16, 2019
@AkihiroSuda
Copy link
Collaborator

--cgroup-driver=none seems more consistent with Docker

@mheon
Copy link
Member Author

mheon commented Jul 16, 2019

Flag change works for me; might be useful to add a cgroupv2 setting to it as well.

@giuseppe
Copy link
Member

LGTM

@mheon
Copy link
Member Author

mheon commented Jul 16, 2019

Few things I want to do here in addition to existing changes:

  • Remove uses of kill --all in the OCI runtime where we can. That definitely needs CGroups to be working.
  • Add something to inspect indicating that there are no CGroups for the container

@vrothberg
Copy link
Member

Flag change works for me; might be useful to add a cgroupv2 setting to it as well.

That could have some positive impact on the code as well since we could introduce a CgroupsManagerNone and use it in the switches.

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #3509) made this pull request unmergeable. Please resolve the merge conflicts.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2019
@rhatdan
Copy link
Member

rhatdan commented Aug 19, 2019

@mheon Needs a rebase.

@rhatdan
Copy link
Member

rhatdan commented Aug 22, 2019

@mheon What is the scoop on this. Seems like something we would want?

@mheon
Copy link
Member Author

mheon commented Aug 22, 2019

Blocked on runc support right now.

I could get this landed, but restrict it to crun for now (similar to how we do --json for logging for the OCI runtime?)

@rhatdan
Copy link
Member

rhatdan commented Aug 22, 2019

Is there a PR opened for runc?
I am fine with crun as an alternative.

@mheon
Copy link
Member Author

mheon commented Aug 22, 2019

I got the runc code mostly working, but when Openstack noted that this wouldn't solve their problems, I abandoned pushing it the final bit of the way. Could be done, but would probably be a day or two to polish it up for PR.

@rhatdan
Copy link
Member

rhatdan commented Aug 22, 2019

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?

@mheon
Copy link
Member Author

mheon commented Aug 22, 2019

Yes - though with limitations (that the Openstack folks found too limiting).

It forces creation of a PID namespace (no --pid=host or --pid=container), completely disables resource limits (systemd solves this by applying its own, so not a really big deal), breaks a lot of commands (top, stats, a few more I'm forgetting).

@mheon mheon changed the title [WIP] Support running containers without CGroups Support running containers without CGroups Aug 26, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 26, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2019
@mheon
Copy link
Member Author

mheon commented Aug 26, 2019

Rebased and updated.

This now only works with OCI runtimes that have been explicitly flagged as supporting it - in this case, crun (runc support is sitting on my hard drive at 80-ish percent complete, waiting for me to find time to finish it).

Should be ready for review now. It's missing tests still, but I need to figure out a good way of doing those.

@mheon
Copy link
Member Author

mheon commented Sep 5, 2019

@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

@rhatdan
Copy link
Member

rhatdan commented Sep 5, 2019

Seems to be tied to crun?

@@ -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
Copy link
Member

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

@@ -57,6 +58,7 @@ case "$SPECIALMODE" in
none)
make
make install PREFIX=/usr ETCDIR=/etc
make install.config PREFIX=/usr
Copy link
Member

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

@vrothberg
Copy link
Member

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).

"Error while adding pod to CNI network \"podman\": running [/usr/sbin/iptables -t filter -N CNI-FORWARD --wait]: exit status 1: iptables: Chain already exists.\n"

A funny one is /var/tmp/go/src/github.com/containers/libpod/test/e2e/run_test.go:958:

[+0650s]   Expected
[+0650s]       <string>: 
[+0650s]   not to equal
[+0650s]       <string>: 

... rerunning special_testing_cgroupv2 TEST_REMOTE_CLIENT:true

@vrothberg
Copy link
Member

Restarted all of the three failed jobs

@mheon
Copy link
Member Author

mheon commented Sep 9, 2019

Found the failure, squashed down, expecting this to go green now.

@mheon
Copy link
Member Author

mheon commented Sep 9, 2019

The crun tests look green, so this is good to go once testing finishes.
@baude @rhatdan @vrothberg @giuseppe @TomSweeneyRedHat PTAL

Copy link
Member

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rhatdan
Copy link
Member

rhatdan commented Sep 9, 2019

@TomSweeneyRedHat
Copy link
Member

LGTM but I'd like @vrothberg to take one last look before merging

Copy link
Member

@vrothberg vrothberg left a 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 to enabled. I find it a bit more approachable than default and disabled.

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>
@mheon
Copy link
Member Author

mheon commented Sep 10, 2019

Done

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mheon
Copy link
Member Author

mheon commented Sep 10, 2019

/retest

@baude
Copy link
Member

baude commented Sep 10, 2019

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2019
@baude
Copy link
Member

baude commented Sep 10, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2019
@mheon
Copy link
Member Author

mheon commented Sep 10, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2019
@openshift-merge-robot openshift-merge-robot merged commit 7ac6ed3 into containers:master Sep 10, 2019
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants