libct/cgroups/utils: fix/separate cgroupv1 code #2411

kolyshkin · 2020-05-15T22:53:12Z

~~NOTE: this is based on and currently includes #2401. I'll rebase once that one is merged.~~ Rebased

This tries to untangle the mess of code in libcontainers/cgroups/utils.go. There are multiple functions that do same or similar things, with multiple names, and it's not clear which ones should be used for cgroup v1, cgroup v2, or both.

Please review commit by commit.

Besides moving the code around, this PR reverts some attempts to make the functions work equally well for v1 and v2. The truth is, v2 is a very different beast and most of the functions that were v1-specific should stand that way.

The only real fix here is commit 775a5cb:

libct/criuApplyCgroups: don't set cgroup paths for v2

There is no need to have cgroupv1-specific controller paths on restore
in case of cgroupv2.

I think the above was the last use of unholy multiple v1 paths in v2 code. 🎉

AkihiroSuda · 2020-05-17T03:48:02Z

libcontainer/cgroups/v1_utils.go

+	"strings"
+)
+
+// Functions in this source file are specific to cgroupv1,


Can we split pkg

I was able to do that. The problem, though, is all the existing users of runc need to adopt. I'm not sure how disruptive it is.

oops, forgot the tests :)

AkihiroSuda · 2020-05-19T00:18:29Z

LGTM

kolyshkin · 2020-05-19T20:38:58Z

So, I checked moby/moby and found these cgroupv1-only functions are used from the code:

daemon/daemon_unix.go:  mnt, root, err := cgroups.FindCgroupMountpointAndRoot("", "cpu")
daemon/oci_linux.go:                    initPath, err := cgroups.GetInitCgroup("cpu")
daemon/oci_linux.go:                    _, err = cgroups.GetOwnCgroup("cpu")
pkg/sysinfo/sysinfo_linux.go:   cgMounts, err := cgroups.GetCgroupMounts(false)
pkg/sysinfo/sysinfo_linux.go:   _, err := cgroups.FindCgroupMountpoint("", "pids")

so this PR will obviously break it.

The reason is, all this stuff is very cgroupv1 specific and it makes no sense to use it when cgroupv2 is used on the host. In case it is still used in such scenario it means the code is doing something wrong and is broken.

@thaJeztah please note the above (and I will be happy to help fix it in moby code).

kolyshkin · 2020-05-19T23:40:39Z

rebased to trigger CI

kolyshkin · 2020-05-20T00:11:05Z

@thaJeztah please note the above (and I will be happy to help fix it in moby code).

update: looks like the place which needs fixing is WithCgroups in daemon/oci_linux.go. Indeed, this code is probably not working right for cgroupv2. Everything else is mostly s|cgroups|cgroups/cgroupv1|g.

kolyshkin · 2020-05-20T15:21:09Z

hmm, a single failure from Fedora32 on Vagrant (logs here):

=== RUN   TestExecInTTY
    TestExecInTTY: execin_test.go:351: unexpected carriage-return in output
--- FAIL: TestExecInTTY (0.14s)

does not look like to be related. This comes from #1146. Hmm

Update: filed #2425

kolyshkin · 2020-05-20T15:57:52Z

OK I think this one is ready. @opencontainers/runc-maintainers PTAL

kolyshkin · 2020-05-20T20:42:01Z

rebased

AkihiroSuda · 2020-05-20T22:39:28Z

LGTM

kolyshkin · 2020-05-22T22:41:35Z

@thaJeztah PTAL. I looked through the moby/moby code and after moby/moby#41014 and moby/moby#41016 the changes to moby/moby should be trivial.

kolyshkin · 2020-05-23T02:42:46Z

updated the patchset to not move GetAllSubsystems out of cgroup1 package (i.e. it can be used for both v1 and v2 code). No other changes.

giuseppe · 2020-06-15T08:25:45Z

There is still a case where we would end up using v1 code in v2 in k8s upstream. @giuseppe I know you were looking at updating this into cadvisor. Do you have your latest tree?

get it to work into cAdvisor requires some substantial changes to how cAdvisor uses libcontainer, or recreate in cAdvisor the cgroupv1.Mount struct when running on cgroup v2. I am worried that there is not much time for introducing changes given that the Kubernetes code freeze for 1.19 is in 10 days.

kolyshkin · 2020-06-15T23:45:52Z

So, what we can do now is commit everything except the last patch (so the v1 code is moved to a separate file but not (yet) to a separate package, leaving this last step for later time.

mrunalp · 2020-06-15T23:54:06Z

@kolyshkin Doing it in steps sounds good 👍

kolyshkin · 2020-06-15T23:55:23Z

OK so what I did is removed two commits:

one that moved code to a separate cgroupv1 package
one that made type NotFoundError private

@giuseppe PTAL

kolyshkin · 2020-06-15T23:56:46Z

Phase 2 (move v1 to a separate package) can be done once we made sure all the users are (or can be) converted.

This function is not used and were never used in any cgroupv2 code. To have it stay that way, let it return error in case it's called for v2. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2020-06-16T03:25:25Z

rebased

giuseppe · 2020-06-16T14:46:36Z

libcontainer/cgroups/utils.go

-			Subsystems: availableControllers,
-		}
-		return []Mount{m}, nil
+		return nil, errUnified


also this change breaks the Kubelet and cAdvisor

This function should not really be used for cgroupv2 code. Currently it is used in kubernetes code, so we can't remove the v2 case yet. Add a TODO item to remove v2 code once kubernetes is converted to not use it, and separate out v1 code. Signed-off-by: Kir Kolyshkin <[email protected]>

It's bad and wrong to use these functions for any cgroupv2 code, and there are no existing users (in runc, at least). Make them return an error in such case. Also, remove the cgroupv2-specific handling from findCgroupMountpointAndRootFromReader(). Signed-off-by: Kir Kolyshkin <[email protected]>

Its value can be easily deduced from the request type. While at it, simplify the call logic. Signed-off-by: Kir Kolyshkin <[email protected]>

There is no need to have cgroupv1-specific controller paths on restore in case of cgroupv2. Signed-off-by: Kir Kolyshkin <[email protected]>

This function is only called from cgroupv1 code, so there is no need for it to implement cgroupv2 stuff. Make it v1-specific, and panic if it is called from v2 code (since this is an internal function, the panic would mean incorrect runc code). Signed-off-by: Kir Kolyshkin <[email protected]>

In particular, state that for cgroup v2 the result is very different. Signed-off-by: Kir Kolyshkin <[email protected]>

This function is cgroupv1-specific, is only used once, and its name is very close to the name of another function, FindCgroupMountpoint. Inline it into the (only) caller. Signed-off-by: Kir Kolyshkin <[email protected]>

In most project, "utils" is a big mess, and this is not an exception. Try to clean it up a bit by moving cgroup v1 specific code to a separate source file. There are no code changes in this commit, just moving it from one file to another. Signed-off-by: Kir Kolyshkin <[email protected]>

When testing GetCgroupMounts, the map data is supposed to be obtained from /proc/self/cgroup, but since we're mocking things, we provide our own map. Unfortunately, not all controllers existing in mountinfos were listed. Also, "name=systemd" needs special handling, so add it. The controllers added were: * for fedoraMountinfo case: name=systemd * for systemdMountinfo case: name=systemd, net_prio * for bedrockMountinfo case: name=systemd, net_prio, pids Signed-off-by: Kir Kolyshkin <[email protected]>

It is easy to just use TrimPrefix which does nothing in case the prefix does not exist. Signed-off-by: Kir Kolyshkin <[email protected]>

no changes, just a few git renames Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin added area/cgroupv2 area/checkpoint-restore kind/refactor refactoring labels May 15, 2020

AkihiroSuda reviewed May 17, 2020

View reviewed changes

kolyshkin force-pushed the v1-specific branch 2 times, most recently from 1259ffa to bc51452 Compare May 18, 2020 18:46

kolyshkin force-pushed the v1-specific branch 2 times, most recently from b88e242 to 5fa4cf9 Compare May 19, 2020 22:11

This comment has been minimized.

Sign in to view

kolyshkin force-pushed the v1-specific branch from 5fa4cf9 to c5ff6ba Compare May 19, 2020 23:39

kolyshkin force-pushed the v1-specific branch from c5ff6ba to b55fb4f Compare May 20, 2020 01:27

kolyshkin mentioned this pull request May 20, 2020

cgroupv2 support meta issue #2315

Closed

kolyshkin force-pushed the v1-specific branch from b55fb4f to b0adb9f Compare May 20, 2020 01:34

kolyshkin mentioned this pull request May 20, 2020

TestExecInTTY: execin_test.go:351: unexpected carriage-return in output #2425

Open

kolyshkin force-pushed the v1-specific branch from b0adb9f to 36d6ff2 Compare May 20, 2020 20:39

kolyshkin mentioned this pull request May 22, 2020

refactor some CPU RT and CFS code moby/moby#41016

Merged

kolyshkin force-pushed the v1-specific branch from 36d6ff2 to 1761d42 Compare May 23, 2020 02:41

This was referenced May 23, 2020

cgroup features detection speedup and simplification cri-o/cri-o#3799

Closed

fixes for pkg/sysinfo/checkCgroupPids containers/common#162

Merged

kolyshkin dismissed AkihiroSuda’s stale review via 6b4ddd0 June 15, 2020 23:53

kolyshkin force-pushed the v1-specific branch from 430759c to 6b4ddd0 Compare June 15, 2020 23:53

kolyshkin mentioned this pull request Jun 16, 2020

Move cgroupv1 specific code out of libcontainers/cgroups pkg #2471

Open

libct/cgroups/utils: getControllerPath return err for v2

3834222

This function is not used and were never used in any cgroupv2 code. To have it stay that way, let it return error in case it's called for v2. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the v1-specific branch from 6b4ddd0 to 837de0a Compare June 16, 2020 03:25

giuseppe reviewed Jun 16, 2020

View reviewed changes

kolyshkin added 11 commits June 16, 2020 12:39

libc/criuSwrk: remove applyCgroups param

52b56bc

Its value can be easily deduced from the request type. While at it, simplify the call logic. Signed-off-by: Kir Kolyshkin <[email protected]>

libct/criuApplyCgroups: don't set cgroup paths for v2

d5c57dc

There is no need to have cgroupv1-specific controller paths on restore in case of cgroupv2. Signed-off-by: Kir Kolyshkin <[email protected]>

libct/cgroups: improve ParseCgroupFile docs

d244b40

In particular, state that for cgroup v2 the result is very different. Signed-off-by: Kir Kolyshkin <[email protected]>

libcontainer/cgroups: rm FindCgroupMountpointDir

7db2d3e

This function is cgroupv1-specific, is only used once, and its name is very close to the name of another function, FindCgroupMountpoint. Inline it into the (only) caller. Signed-off-by: Kir Kolyshkin <[email protected]>

libct/cgroupv1/getCgroupMountsHelper: minor nit

cec5ae7

It is easy to just use TrimPrefix which does nothing in case the prefix does not exist. Signed-off-by: Kir Kolyshkin <[email protected]>

libct/cgroups/fs: rename some files

8c5a19f

no changes, just a few git renames Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the v1-specific branch from 837de0a to 8c5a19f Compare June 16, 2020 19:46

kolyshkin mentioned this pull request Jun 16, 2020

criu int test failures #2475

Closed

AkihiroSuda approved these changes Jun 17, 2020

View reviewed changes

giuseppe approved these changes Jun 17, 2020

View reviewed changes

mrunalp approved these changes Jun 17, 2020

View reviewed changes

mrunalp merged commit 12a7c8f into opencontainers:master Jun 17, 2020

kolyshkin mentioned this pull request Jun 8, 2021

parseCgroupFromReader(): Ignore unified paths with no subsystem #3001

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libct/cgroups/utils: fix/separate cgroupv1 code #2411

libct/cgroups/utils: fix/separate cgroupv1 code #2411

kolyshkin commented May 15, 2020 •

edited

Loading

AkihiroSuda May 17, 2020

kolyshkin May 18, 2020

kolyshkin May 18, 2020

kolyshkin May 18, 2020

AkihiroSuda commented May 19, 2020 •

edited

Loading

kolyshkin commented May 19, 2020

This comment has been minimized.

This comment has been minimized.

kolyshkin commented May 19, 2020

kolyshkin commented May 20, 2020

kolyshkin commented May 20, 2020 •

edited

Loading

kolyshkin commented May 20, 2020

kolyshkin commented May 20, 2020

AkihiroSuda commented May 20, 2020 •

edited

Loading

kolyshkin commented May 22, 2020

kolyshkin commented May 23, 2020 •

edited

Loading

giuseppe commented Jun 15, 2020

kolyshkin commented Jun 15, 2020

mrunalp commented Jun 15, 2020

kolyshkin commented Jun 15, 2020 •

edited

Loading

kolyshkin commented Jun 15, 2020

kolyshkin commented Jun 16, 2020

giuseppe Jun 16, 2020

kolyshkin Jun 16, 2020

libct/cgroups/utils: fix/separate cgroupv1 code #2411

libct/cgroups/utils: fix/separate cgroupv1 code #2411

Conversation

kolyshkin commented May 15, 2020 • edited Loading

AkihiroSuda May 17, 2020

Choose a reason for hiding this comment

kolyshkin May 18, 2020

Choose a reason for hiding this comment

kolyshkin May 18, 2020

Choose a reason for hiding this comment

kolyshkin May 18, 2020

Choose a reason for hiding this comment

AkihiroSuda commented May 19, 2020 • edited Loading

kolyshkin commented May 19, 2020

This comment has been minimized.

This comment has been minimized.

kolyshkin commented May 19, 2020

kolyshkin commented May 20, 2020

kolyshkin commented May 20, 2020 • edited Loading

kolyshkin commented May 20, 2020

kolyshkin commented May 20, 2020

AkihiroSuda commented May 20, 2020 • edited Loading

kolyshkin commented May 22, 2020

kolyshkin commented May 23, 2020 • edited Loading

giuseppe commented Jun 15, 2020

kolyshkin commented Jun 15, 2020

mrunalp commented Jun 15, 2020

kolyshkin commented Jun 15, 2020 • edited Loading

kolyshkin commented Jun 15, 2020

kolyshkin commented Jun 16, 2020

giuseppe Jun 16, 2020

Choose a reason for hiding this comment

kolyshkin Jun 16, 2020

Choose a reason for hiding this comment

kolyshkin commented May 15, 2020 •

edited

Loading

AkihiroSuda commented May 19, 2020 •

edited

Loading

kolyshkin commented May 20, 2020 •

edited

Loading

AkihiroSuda commented May 20, 2020 •

edited

Loading

kolyshkin commented May 23, 2020 •

edited

Loading

kolyshkin commented Jun 15, 2020 •

edited

Loading