podman: add new cgroup mode split by giuseppe · Pull Request #6666 · containers/podman

giuseppe · 2020-06-18T12:01:01Z

add a new cgroup mode split.

When running under systemd there is no need to create yet another cgroup for the container.

With conmon-delegated the current cgroup will be split in two sub cgroups:

- supervisor
- container

The supervisor cgroup will hold conmon and the podman process, while the container cgroup is used by the OCI runtime (using the cgroupfs backend).

Closes: #6400

Signed-off-by: Giuseppe Scrivano [email protected]

giuseppe · 2020-06-18T12:01:28Z

@vrothberg PTAL

vrothberg

The commit messages states conmon-delegated but the code adds a no-conmon-delegated. I'd guess we should update the docs as well.

Do we need to update the systemd unit files in pkg/system/generate?

vrothberg · 2020-06-18T12:07:16Z

libpod/container_internal_linux.go

Can we make "no-conmon-delegated" a constant somewhere?

The commit messages states conmon-delegated but the code adds a no-conmon-delegated. I'd guess we should update the docs as well.

Thanks, I've renamed it to conmon-delegated and added a constant

Do we need to update the systemd unit files in pkg/system/generate?

I don't think we can do that yet. crun needs containers/crun#409

mheon · 2020-06-18T13:30:38Z

docs/source/markdown/podman-run.1.md

If it's also specific to systemd cgroups (looks like it is) we should also mention that

not sure it is so specific to systemd. It can be used also without systemd, even if it is less useful

this will also break remote. We probably need a check to not permit the option being used as remote as it changes the cgroup of the Podman process itself

mheon · 2020-06-18T13:32:19Z

Overall, I like this. I think it does break podman stats/podman top though, and it probably doesn't need to - we just need to look in the current cgroup instead

goochjj · 2020-06-18T15:32:00Z

Is there a reason why you did cgroups 2 only? This approach works for cgroups 1:
goochjj@3d6ec20

I'd happily make this coexist if need be.

goochjj · 2020-06-18T15:35:40Z

For that matter, why is cgroup-parent so draconian? It only allows items to end in .slice.

And yet this works:
"cgroupsPath": "/machine.slice/rwhod.service/container",

in config.json

Container cgroups could be implemented with --cgroup-parent /machine.slice/%n/container, if the cgroup-parent check weren't rejecting it. This commit proves it works.

Would a better implementation be implementing --conmon-cgroup-parent /machine.slice/%n/supervisor?

giuseppe · 2020-06-18T15:44:48Z

Container cgroups could be implemented with --cgroup-parent /machine.slice/%n/container, if the cgroup-parent check weren't rejecting it. This commit proves it works.

we could relax them.

Would a better implementation be implementing --conmon-cgroup-parent /machine.slice/%n/supervisor?

The current implementation works well also without systemd, i.e. you have a cgroup you own and you want to create both conmon+container cgroup in it.

giuseppe · 2020-06-18T15:46:21Z

Is there a reason why you did cgroups 2 only? This approach works for cgroups 1:
goochjj@3d6ec20

I'd happily make this coexist if need be.

just simplicity. We can surely add cgroup v1 support. It must be done for each controller separately, it is not enough to get the cgroup from the pid controller

goochjj · 2020-06-18T16:15:33Z

Is there a reason why you did cgroups 2 only? This approach works for cgroups 1:
goochjj@3d6ec20
I'd happily make this coexist if need be.

just simplicity. We can surely add cgroup v1 support. It must be done for each controller separately, it is not enough to get the cgroup from the pid controller

I read the cgroup from the pid controller, but in the Move call, I move all of them... that should do it, right?

I don't know if pids will always be 3 - I can fix the find if need be.

giuseppe · 2020-06-18T16:17:02Z

I read the cgroup from the pid controller, but in the Move call, I move all of them... that should do it, right?

I don't know if pids will always be 3 - I can fix the find if need be.

there could be different cgroups for each controller, the safest is to read each controller and then do the move as part of the same loop for that controller

goochjj · 2020-06-18T16:21:16Z

I read the cgroup from the pid controller, but in the Move call, I move all of them... that should do it, right?
I don't know if pids will always be 3 - I can fix the find if need be.

there could be different cgroups for each controller, the safest is to read each controller and then do the move as part of the same loop for that controller

... except you were using the same call to determine the name of the supervisor cgroup - so I could only return 1

So should the supervisor cgroup name be based on the pids namespace, and the move do something else? i.e. just tack "container" onto the end? tack "container" onto the end if it's not /?

goochjj · 2020-06-18T17:56:24Z

@giuseppe
giuseppe#2

A couple thoughts here:

The cgroup of the container is handled by the OCI runtime, as passed into the bundle. All conmon-delegated does is ensure that cgroup passed in the spec is "/container", which needs an additional function (GetPidCgroup()) to determine what cgroups we were passed.
The cgroup of the conmon process is already handled in oci_conmon_linux... So the new function to move to a subtree shouldn't be necessary... The strategy also uses a "cgroupfs" method for moving the Pids, not a "lets create a systemd transition unit to do it" method. That code already exists in oci_conmon_linux, it just needs the same as above - use of GetPidCgroup to construct the "/supervisor" cgroup path. And the cgroups package already deals with "do all the controllers" logic.

goochjj · 2020-06-18T18:02:01Z

Overall, I like this. I think it does break podman stats/podman top though, and it probably doesn't need to - we just need to look in the current cgroup instead

FYI top is working for me - it properly shows the /slice/unit/container cgroup

Stats is not.

giuseppe · 2020-06-18T18:34:35Z

1. The cgroup of the container is handled by the OCI runtime, as passed into the bundle.  All conmon-delegated does is ensure that cgroup passed in the spec is "/container", which needs an additional function (GetPidCgroup()) to determine what cgroups we were passed.

2. The cgroup of the conmon process is already handled in oci_conmon_linux... So the new function to move to a subtree shouldn't be necessary... The strategy also uses a "cgroupfs" method for moving the Pids, not a "lets create a systemd transition unit to do it" method.  That code already exists in oci_conmon_linux, it just needs the same as above - use of GetPidCgroup to construct the "/supervisor" cgroup path.  And the cgroups package already deals with "do all the controllers" logic.

for cgroup v2 there cannot be processes in the inner groups. We need to make sure the conmon process is already in the right sub cgroup before it starts (or have a way to communicate to conmon to move itself). We need to do such steps before setting up the container, the previous code path cannot be used.

... except you were using the same call to determine the name of the supervisor cgroup - so I could only return 1

there is only one hierarchy in cgroup v2, so that is enough.

In any case, I think I can just make my PR more generic to handle cgroup v1 too, even if it is not necessary to move the conmon process in a sub cgroup

goochjj · 2020-06-18T18:47:23Z

pr updated w/ podman stats working

I'll be the first to admit, I don't fully understand all the implications, and I don't have a cgv2 machine to test on. I do know this all works on cgv1, and it looks simpler and reuses code paths that were already present and seem to make sense.

But I don't see how podman stats is going to work if you never pass the cgroup parent back through the container object...

Thanks!

goochjj · 2020-06-18T19:08:17Z

FCOS appears to be running cg1/2 hybrid, and pkg/cgroups doesn't adjust /sys/fs/cgroup/unified.

Is the long term plan to fix pkg/cgroups or deprecate it?

goochjj · 2020-06-19T14:54:33Z

LGTM

goochjj · 2020-06-19T14:55:24Z

@mheon @vrothberg PTAL

mheon · 2020-06-19T15:11:47Z

We want to hold this until Monday - we're in a semi-feature-freeze state for v2.0.0 release, which should happen in a few hours. Will do a full review once that is cut.

rhatdan · 2020-06-22T18:33:37Z

docs/source/markdown/podman-create.1.md

Can we just call this current? I don't see where split- adds any additional information to the user.

we may need current in future to really mean current and use the cgroup where Podman is running instead of creating two sub-cgroups

reuse-current or perhaps just reuse then?

docs/source/markdown/podman-run.1.md

libpod/oci_conmon.go

openshift-ci-robot · 2020-06-25T08:02:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

docs/source/markdown/podman-run.1.md

mheon · 2020-06-25T13:13:56Z

Can you add the cgroup-parent validation to validate() in libpod/container_validate.go?

When running under systemd there is no need to create yet another cgroup for the container. With conmon-delegated the current cgroup will be split in two sub cgroups: - supervisor - container The supervisor cgroup will hold conmon and the podman process, while the container cgroup is used by the OCI runtime (using the cgroupfs backend). Closes: containers#6400 Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2020-06-25T15:18:40Z

Can you add the cgroup-parent validation to validate() in libpod/container_validate.go?

added and pushed a new version

giuseppe · 2020-06-29T11:02:48Z

ready for review

mheon · 2020-06-29T13:55:37Z

LGTM

goochjj · 2020-06-29T14:07:32Z

LGTM

TomSweeneyRedHat · 2020-06-29T14:43:11Z

docs/source/markdown/podman-run.1.md

+The **enabled** option will create a new cgroup under the cgroup-parent.
+The **disabled** option will force the container to not create CGroups, and thus conflicts with CGroup options (**--cgroupns** and **--cgroup-parent**).
 The **no-conmon** option disables a new CGroup only for the **conmon** process.
+The **split** option splits the current cgroup in two sub-cgroups: one for conmon and one for the container payload.  It is not possible to set **--cgroup-parent** with **split**.


We really, really, really need to create a conmon man page that we can add a link to for pages like this. @haircommander any chance of making this happen, even if the page is terribly short and sweet?

This is not a holdback for this review at this time.

opened containers/conmon#184

TomSweeneyRedHat

LGTM

mheon · 2020-06-29T14:49:38Z

/lgtm

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 Jun 18, 2020

openshift-ci-robot requested review from baude and jwhonce June 18, 2020 12:01

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2020

giuseppe mentioned this pull request Jun 18, 2020

pursuing conventional systemd+podman interaction #6400

Closed

vrothberg reviewed Jun 18, 2020

View reviewed changes

giuseppe force-pushed the conmon-delegate branch 3 times, most recently from 0148073 to 8832f34 Compare June 18, 2020 12:35

mheon reviewed Jun 18, 2020

View reviewed changes

giuseppe force-pushed the conmon-delegate branch 3 times, most recently from e168a7c to 173e414 Compare June 18, 2020 14:44

goochjj mentioned this pull request Jun 18, 2020

[WIP] podman: add new cgroup mode conmon-delegated giuseppe/libpod#2

Open

goochjj mentioned this pull request Jun 18, 2020

conmon syslog proxy containers/conmon#176

Closed

giuseppe force-pushed the conmon-delegate branch from 25e14a5 to 66c9e08 Compare June 19, 2020 14:53

rhatdan reviewed Jun 22, 2020

View reviewed changes

giuseppe force-pushed the conmon-delegate branch from 66c9e08 to c555172 Compare June 25, 2020 08:02

rhatdan reviewed Jun 25, 2020

View reviewed changes

docs/source/markdown/podman-run.1.md Outdated Show resolved Hide resolved

giuseppe force-pushed the conmon-delegate branch from c555172 to ac92674 Compare June 25, 2020 12:42

giuseppe changed the title ~~podman: add new cgroup mode split-current~~ podman: add new cgroup mode split Jun 25, 2020

mheon reviewed Jun 25, 2020

View reviewed changes

docs/source/markdown/podman-run.1.md Outdated Show resolved Hide resolved

mheon reviewed Jun 25, 2020

View reviewed changes

docs/source/markdown/podman-run.1.md Outdated Show resolved Hide resolved

giuseppe force-pushed the conmon-delegate branch from ac92674 to 6ee5f74 Compare June 25, 2020 15:16

TomSweeneyRedHat reviewed Jun 29, 2020

View reviewed changes

openshift-ci-robot assigned mheon Jun 29, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2020

openshift-merge-robot merged commit 6ac009d into containers:master Jun 29, 2020

haircommander mentioned this pull request Jun 29, 2020

add man page containers/conmon#184

Closed

huww98 mentioned this pull request Mar 11, 2021

cephadm: use split cgroup strategy for podman ceph/ceph#40025

Merged

3 tasks

runiq mentioned this pull request May 4, 2022

Running rootless Podman in Systemd unit with PrivateTmp=true fails #14106

Closed

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 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Conversation

giuseppe commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giuseppe commented Jun 18, 2020

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

vrothberg Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheon commented Jun 18, 2020

Uh oh!

goochjj commented Jun 18, 2020

Uh oh!

goochjj commented Jun 18, 2020

Uh oh!

giuseppe commented Jun 18, 2020

Uh oh!

giuseppe commented Jun 18, 2020

Uh oh!

goochjj commented Jun 18, 2020

Uh oh!

giuseppe commented Jun 18, 2020

Uh oh!

goochjj commented Jun 18, 2020

Uh oh!

goochjj commented Jun 18, 2020

Uh oh!

goochjj commented Jun 18, 2020

Uh oh!

giuseppe commented Jun 18, 2020

Uh oh!

goochjj commented Jun 18, 2020

Uh oh!

goochjj commented Jun 18, 2020

Uh oh!

goochjj commented Jun 19, 2020

Uh oh!

goochjj commented Jun 19, 2020

Uh oh!

mheon commented Jun 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Jun 25, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mheon commented Jun 25, 2020

Uh oh!

giuseppe commented Jun 25, 2020

Uh oh!

giuseppe commented Jun 29, 2020

Uh oh!

mheon commented Jun 29, 2020

Uh oh!

goochjj commented Jun 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe commented Jun 18, 2020 •

edited

Loading

vrothberg Jun 18, 2020 •

edited

Loading

giuseppe Jun 18, 2020 •

edited

Loading