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.

Sign up for GitHub

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

Jump to bottom

[1.1] libctr/cgroups: don't take init's cgroup into account #3811

Merged
merged 1 commit into from
Apr 7, 2023
Merged

[1.1] libctr/cgroups: don't take init's cgroup into account #3811

merged 1 commit into from
Apr 7, 2023

Conversation

haircommander
Copy link
Contributor

Sometimes, the init process is not in the root cgroup. This can be noted by GetInitPath, which already scrubs the path of init.scope.

This was encountered when trying to patch the Kubelet to handle systemd being in a separate cpuset from root (to allow load balance disabling for containers). At present, there's no way to have libcontainer or runc manage cgroups in a hierarchy outside of the one init is in (unless the path contains init.scope, which is limiting)

@haircommander
Copy link
Contributor Author

PTAL @kolyshkin @mrunalp

@kolyshkin
Copy link
Contributor

To reviewers: this is a backport of PR #3784 to release-1.1 branch. In comments to that PR, I tried to find out whether this change can break anything. TL;DR, my conclusion was (and still is) that this is not a breaking change. More to say, using PID 1's cgroup as a base is not correct when host pid ns is used, and thus it was removed a long time ago (but not for systemd cgroup driver).

Now, this PR is required for the case where systemd is placed into a non-root cgroup.

Such placement is needed in recent kernels, when we want some containers (cgroups) to have CPU load-balancing disabled (cpuset.sched_load_balance = 0). Unfortunately, this also requires all the cgroup parents (up to root cgroup) to also have LB disabled. So, in order to achieve this without disabling CPU load balancing systemd-wide, all other processes on the system (including systemd) are moved to a separate cgroup hierarchy.

kolyshkin
kolyshkin previously approved these changes Apr 4, 2023
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

mrunalp
mrunalp previously approved these changes Apr 5, 2023
View reviewed changes
@kolyshkin kolyshkin added this to the 1.1.6 milestone Apr 5, 2023
@AkihiroSuda
Copy link
Member

DCO check is failing

@haircommander haircommander dismissed stale reviews from kolyshkin and mrunalp via 6cc1a3d April 6, 2023 13:29
@haircommander
Copy link
Contributor Author

fixed!

@kolyshkin kolyshkin added area/systemd backport/1.1-pr A backport to 1.1.x release. labels Apr 6, 2023
kolyshkin
kolyshkin previously approved these changes Apr 6, 2023
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Still LGTM

mrunalp
mrunalp previously approved these changes Apr 6, 2023
View reviewed changes
@haircommander haircommander dismissed stale reviews from kolyshkin and mrunalp via 9b17bdc April 7, 2023 01:38
kolyshkin
kolyshkin approved these changes Apr 7, 2023
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

still LGTM

@kolyshkin kolyshkin mentioned this pull request Apr 7, 2023
Merged
AkihiroSuda
AkihiroSuda requested changes Apr 7, 2023
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Please use git cherry-pick with -x

@haircommander
libctr/cgroups: don't take init's cgroup into account
10cfd81 
Sometimes, the init process is not in the root cgroup.
This can be noted by GetInitPath, which already scrubs the path of `init.scope`.

This was encountered when trying to patch the Kubelet to handle systemd being in a separate cpuset
from root (to allow load balance disabling for containers). At present, there's no way to have libcontainer or runc
manage cgroups in a hierarchy outside of the one init is in (unless the path contains `init.scope`, which is limiting)

Signed-off-by: Peter Hunt <pehunt@redhat.com>
(cherry picked from commit 54e2021)
@haircommander
Copy link
Contributor Author

Please use git cherry-pick with -x

done!

AkihiroSuda
AkihiroSuda approved these changes Apr 7, 2023
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@mrunalp mrunalp merged commit 53333a5 into opencontainers:release-1.1 Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrunalp mrunalp mrunalp approved these changes

@kolyshkin kolyshkin kolyshkin approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees
No one assigned
Labels
area/systemd backport/1.1-pr A backport to 1.1.x release.
Projects
None yet
Milestone
1.1.6
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@haircommander @kolyshkin @AkihiroSuda @mrunalp