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

[WIP] cgroupv2: fix systemd driver not putting pid into cgroup #2311

Closed
wants to merge 1 commit into from
Closed

[WIP] cgroupv2: fix systemd driver not putting pid into cgroup #2311

wants to merge 1 commit into from

Conversation

kolyshkin
Copy link
Contributor

Apparently, the driver never cared to add pid to the cgroup.

Fixes: #2310

@kolyshkin kolyshkin mentioned this pull request Apr 13, 2020
Closed
@kolyshkin kolyshkin changed the title cgroupv2: fix systemd driver not putting pid into cgroup [WIP] cgroupv2: fix systemd driver not putting pid into cgroup Apr 13, 2020
@kolyshkin
Copy link
Contributor Author

Let me test this first...

@kolyshkin kolyshkin force-pushed the cgroupv2-fix-apply branch from 8bb3bc3 to 5965d16 Compare April 13, 2020 01:33
@kolyshkin
Copy link
Contributor Author

Temporary

@kolyshkin
Copy link
Contributor Author

Tested locally, I confirm it fixes the issue.

@kolyshkin
Copy link
Contributor Author

Hmm, I don't see Travis CI in the list of checks here :-\

@kolyshkin kolyshkin force-pushed the cgroupv2-fix-apply branch from 5965d16 to afb60fa Compare April 13, 2020 02:47
@kolyshkin
Copy link
Contributor Author

Travis succeeds (see results here: https://travis-ci.org/github/opencontainers/runc/jobs/674240854)

ok 47 ps
ok 48 ps -f json
ok 49 ps -e -x

Removing temp patches.

@kolyshkin kolyshkin force-pushed the cgroupv2-fix-apply branch from afb60fa to 8b1c80c Compare April 13, 2020 05:21
@kolyshkin
Copy link
Contributor Author

This one is ready to be merged

@mrunalp @AkihiroSuda @crosbymichael PTAL

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Apr 13, 2020 •
edited
Loading

LGTM

Approved with PullApprove

@kolyshkin kolyshkin changed the title [WIP] cgroupv2: fix systemd driver not putting pid into cgroup cgroupv2: fix systemd driver not putting pid into cgroup Apr 13, 2020
giuseppe
giuseppe reviewed Apr 13, 2020
View reviewed changes
libcontainer/cgroups/systemd/unified_hierarchy.go
@@ -151,6 +151,9 @@ func (m *UnifiedManager) Apply(pid int) error {
if err := createCgroupsv2Path(path); err != nil {
return err
}
if err := cgroups.WriteCgroupProc(path, pid); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

isn't it done automatically by systemd?

I see we are specifying the PID here: https://github.com/opencontainers/runc/blob/master/libcontainer/cgroups/systemd/unified_hierarchy.go#L61-L63

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that systemd v1 driver (aka LegacyManager) is doing it explicitly:

  • Apply -> joinCgroups -> join for all controllers except cpuset
  • ... -> cpuset.ApplyDir for cpuset

runc/libcontainer/cgroups/systemd/apply_systemd.go

Line 312 in ccbb336

if err := cgroups.WriteCgroupProc(path, pid); err != nil {

Copy link
Member

Choose a reason for hiding this comment

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

I guess it is done because some controllers are not supported by systemd (like cpuset). On cgroup v2 it is enough to join the cgroup once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@giuseppe you're right, adding PIDs property should be enough, and yet it is not working. Investigating.

Copy link
Member

Choose a reason for hiding this comment

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

I'd investigate if dbusConnection.StartTransientUnit waits correctly that the systemd unit is created. Does it eventually join the correct cgroup if you add a sleep here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #2310 (comment)

@kolyshkin kolyshkin mentioned this pull request Apr 13, 2020
Merged
@kolyshkin kolyshkin changed the title cgroupv2: fix systemd driver not putting pid into cgroup [WIP] cgroupv2: fix systemd driver not putting pid into cgroup Apr 13, 2020
@kolyshkin
Copy link
Contributor Author

See here:
#2310 (comment) (TL;DR: it was selinux).

I guess we can close this one, but we still need to figure out what to do about getting the error back from systemd, if it fails.

@kolyshkin kolyshkin closed this Apr 14, 2020
@kolyshkin kolyshkin deleted the cgroupv2-fix-apply branch May 20, 2020 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giuseppe giuseppe giuseppe left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SELinux] cgroupv2: runc run --systemd-cgroup do not put container in proper cgroup
3 participants
@kolyshkin @AkihiroSuda @giuseppe