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

core: don't use the unified hierarchy for the systemd cgroup yet #4628

Merged
merged 1 commit into from Nov 10, 2016

Conversation

martinpitt
Copy link
Contributor

Too many things don't get along with the unified hierarchy yet:

So revert the default to the legacy hierarchy for now. Developers of the above
software can opt into the unified hierarchy with
"systemd.legacy_systemd_cgroup_controller=0".

Too many things don't get along with the unified hierarchy yet:

 * opencontainers/runc#1175
 * moby/moby#28109
 * lxc/lxc#1280

So revert the default to the legacy hierarchy for now. Developers of the above
software can opt into the unified hierarchy with
"systemd.legacy_systemd_cgroup_controller=0".
@martinpitt martinpitt added regression ⚠️ A bug in something that used to work correctly and broke through some recent commit activate pid1 cgroups and removed activate labels Nov 9, 2016
@martinpitt
Copy link
Contributor Author

@htejun, WDYT?

@keszybz
Copy link
Member

keszybz commented Nov 9, 2016

Implementation-wise this looks correct.

@htejun
Copy link
Contributor

htejun commented Nov 9, 2016

Looks good to me. Thanks!

@keszybz
Copy link
Member

keszybz commented Nov 10, 2016

Like I said on semaphore, I think it'd be nice to provide a ./configure switch for this, similar to the one for the KillUserProcesses default. But anyway, we can merge this, and add the switch later.

@poettering
Copy link
Member

Can we please not play games of toggling this all the time. Please fix this properly, add the configure switch, and leave this enabled by default.

I am pretty sure this should stay on by default, and it should be on distros to turn it off, but we should make it easy for them by providing said configure switch plus an explanation in NEWS. But playing ping-pong and turning it off and on every second version is just wrong...

@martinpitt any chance you can prep a proper fix for this that adds such a switch and turns it on again? thanks!

@poettering
Copy link
Member

i created a blocker issue in #4669 now, so that this is fixed properly before the 233 release

@htejun
Copy link
Contributor

htejun commented Nov 14, 2016

Just posted #4670 which puts cgroup v2 systemd hierarchy on /sys/fs/cgroup/systemd-cgroup2 (any idea for a better name?) while maintaining the "name=systemd" hierarchy on /sys/fs/cgroup/systemd in parallel. This should avoid issues with most tools. For the ones which fail to parse if there's an entry for the v2 hierarchy in /proc/$PID/cgroup, I have no idea yet.

@martinpitt
Copy link
Contributor Author

Please fix this properly, add the configure switch, and leave this enabled by default.

I didn't because IMHO a configure switch is the worst way; a runtime switch is a much lower barrier for both affected users for which it breaks, and developers of lxc, docker, and friends to try out the unified hiearchy without rebuilding. Also, it's another doubling of combinatorial explosion of which we don't test the other half.

I'd actually prefer reverting the revert and let unified cgroups be always on in upstream releases, and only revert it downstream. But if you still want that even after #4670, I'll add a configure option.

@poettering
Copy link
Member

I didn't because IMHO a configure switch is the worst way; a runtime switch is a much lower barrier for both affected users for which it breaks, and developers of lxc, docker, and friends to try out the unified hiearchy without rebuilding.

A compile time switch doesn't prohibt a kernel cmdline switch. This was discussed before, there should be a compile-time default selectable via a configure switch, and then there should be a kernel cmdline switch that overrides that if set.

manover pushed a commit to manover/systemd that referenced this pull request Jan 21, 2017
Too many things don't get along with it yet, like docker, LXC, or runc.

Forwarded to systemd/systemd#4628.

Closes: #843509
Werkov pushed a commit to Werkov/systemd that referenced this pull request Jan 26, 2017
…temd#4628)

Too many things don't get along with the unified hierarchy yet:

 * opencontainers/runc#1175
 * moby/moby#28109
 * lxc/lxc#1280

So revert the default to the legacy hierarchy for now. Developers of the above
software can opt into the unified hierarchy with
"systemd.legacy_systemd_cgroup_controller=0".
(cherry picked from commit 843d5ba)
xaiki pushed a commit to endlessm/systemd that referenced this pull request Feb 1, 2017
Too many things don't get along with it yet, like docker, LXC, or runc.

Forwarded to systemd/systemd#4628.

Closes: #843509
globin pushed a commit to NixOS/systemd that referenced this pull request Feb 16, 2017
…temd#4628)

Too many things don't get along with the unified hierarchy yet:

 * opencontainers/runc#1175
 * moby/moby#28109
 * lxc/lxc#1280

So revert the default to the legacy hierarchy for now. Developers of the above
software can opt into the unified hierarchy with
"systemd.legacy_systemd_cgroup_controller=0".
(cherry picked from commit 843d5ba)
evverx referenced this pull request in htejun/systemd Feb 19, 2017
It is expected that general-purpose distributions might want to override this.
This commit is made separate from grandparent to make it easy to revert if
needed.

Fixes systemd#4669.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cgroups pid1 regression ⚠️ A bug in something that used to work correctly and broke through some recent commit
Development

Successfully merging this pull request may close these issues.

None yet

4 participants