cgroupv2: don't enable threaded mode by default by lifubang · Pull Request #2390 · opencontainers/runc

lifubang · 2020-05-08T15:47:53Z

After this commit: 60c647e
Runc enable threaded mode in cgroup v2 by default.
If the cgroupPath is set to a absolute path like /docker/********, the memory subsystem can't be used by this mode.
So, I think we should use domain mode by default.

Signed-off-by: lifubang lifubang@acmcoder.com

lifubang · 2020-05-08T15:49:35Z

@AkihiroSuda PTAL

cyphar · 2020-05-08T17:38:13Z

I'm not sure I understand why we're putting cgroupv2 into threaded mode -- @AkihiroSuda is this to work around some odd permission issue (and why does it fix an EPERM)? The description in 60c647e and #2340 is pretty sparse.

AkihiroSuda · 2020-05-08T23:30:18Z

-			// Otherwise ENOTSUP may happen.
-			cgType := filepath.Join(current, "cgroup.type")
-			_ = ioutil.WriteFile(cgType, []byte("threaded"), 0644)
+			if rootless {


cgType := filepath.Join(current, "cgroup.type") cgTypeB, _ := ioutil.ReadFile(cgType) if strings.TrimSpace(string(cgTypeB)) == "domain invalid" { _ = ioutil.WriteFile(cgType, []byte("threaded"), 0644) }

AkihiroSuda · 2020-05-08T23:42:55Z

@cyphar
Writing threaded to cgroup.type is required when cgroup.type becomes domain invalid, mostly in nested environment

$ sudo podman run -it --rm --privileged --runtime=crun alpine
/ # cd /sys/fs/cgroup/
/sys/fs/cgroup # cat cgroup.controllers 
cpuset cpu io memory pids
/sys/fs/cgroup # cat cgroup.subtree_control 
/sys/fs/cgroup # echo +cpu > cgroup.subtree_control
/sys/fs/cgroup # mkdir foo
/sys/fs/cgroup # cat foo/cgroup.type
domain invalid

cyphar · 2020-05-09T06:44:17Z

Okay, but "domain invalid" means that the cgroup is in an invalid state (meaning that one of the cgroup rules has been violated -- most likely the no-internal-processes rule). Putting a cgroup into threaded mode doesn't fix that -- it switches it into an alternative mode which only allows controllers which are thread-aware to be enabled (such as cpu). This is why we can't enable the memory controller -- it isn't thread-aware.

IMHO, a more complete solution would be to figure out how to deal with the parent cgroup having child processes (which is a bit dodgy if we're going to move other programs on the system between cgroups) or to simply give an error if we hit domain invalid and explain what the issue is.

lifubang · 2020-05-09T22:36:51Z

or to simply give an error if we hit domain invalid and explain what the issue is.

Personally, I prefer this one. @AkihiroSuda WDYT? Because in man7 cgroups:

2. "Internal" processes are not permitted.  With the exception of the
          root cgroup, processes may reside only in leaf nodes (cgroups that
          do not themselves contain child cgroups).  The details are
          somewhat more subtle than this, and are described below.

Cgroups v2 "no internal processes" rule
       Cgroups v2 enforces a so-called "no internal processes" rule.
       Roughly speaking, this rule means that, with the exception of the
       root cgroup, processes may reside only in leaf nodes (cgroups that do
       not themselves contain child cgroups).  This avoids the need to
       decide how to partition resources between processes which are members
       of cgroup A and processes in child cgroups of A.

       For instance, if cgroup /cg1/cg2 exists, then a process may reside in
       /cg1/cg2, but not in /cg1.  This is to avoid an ambiguity in cgroups
       v1 with respect to the delegation of resources between processes in
       /cg1 and its child cgroups.  The recommended approach in cgroups v2
       is to create a subdirectory called leaf for any nonleaf cgroup which
       should contain processes, but no child cgroups.  Thus, processes
       which previously would have gone into /cg1 would now go into
       /cg1/leaf.  This has the advantage of making explicit the relation‐
       ship between processes in /cg1/leaf and /cg1's other children.

       The "no internal processes" rule is in fact more subtle than stated
       above.  More precisely, the rule is that a (nonroot) cgroup can't
       both (1) have member processes, and (2) distribute resources into
       child cgroups—that is, have a nonempty cgroup.subtree_control file.
       Thus, it is possible for a cgroup to have both member processes and
       child cgroups, but before controllers can be enabled for that cgroup,
       the member processes must be moved out of the cgroup (e.g., perhaps
       into the child cgroups).

       With the Linux 4.14 addition of "thread mode" (described below), the
       "no internal processes" rule has been relaxed in some cases.

AkihiroSuda · 2020-05-10T00:31:00Z

SGTM, but when no domain controller is enabled, we can write "threaded" without retuning error

AkihiroSuda · 2020-05-10T05:51:01Z

+			if strings.TrimSpace(string(cgType)) == "domain invalid" {
+				cgTypeParentFile := filepath.Join(current, "../cgroup.type")
+				cgTypeParent, _ := ioutil.ReadFile(cgTypeParentFile)
+				if bytes.HasPrefix(cgTypeParent, []byte("domain")) {


IIUC we don't need to check parent, we just need to check whether the current config contains domain controller

lifubang · 2020-05-10T16:19:33Z

runc/tests/rootless.sh

Line 103 in 1d14356

echo threaded > "$CGROUP_MOUNT/$CGROUP_PATH/cgroup.type"

I think rootless doesn't need threaded mode in default?
I don't know whether my opinion is right or not. @AkihiroSuda

AkihiroSuda · 2020-05-10T18:20:29Z

Let's check invalid cgroup.type and set threaded conditionally

AkihiroSuda · 2020-05-10T23:23:57Z

+		if grep -qw invalid "$CGROUP_MOUNT/$CGROUP_PATH/cgroup.type"; then
+			echo threaded > "$CGROUP_MOUNT/$CGROUP_PATH/cgroup.type"
+		fi
 		# Make sure cgroup.type doesn't contain "invalid". Otherwise write ops will fail with ENOTSUP.


You can drop L106-L110 now

@lifubang ^^^

This has been removed before LGTM.
I think we should keep these comments to let other people know why we need to write threaded to cgroups.type.

Is the echo threaded > still needed? Seems a bit odd to run the entire test suite under threaded.

AkihiroSuda · 2020-05-10T23:37:08Z

LGTM

kolyshkin

a few minor fixes, otherwise good

AkihiroSuda · 2020-05-11T04:48:01Z

LGTM

AkihiroSuda · 2020-05-12T02:03:15Z

@kolyshkin LGTY?

Because in threaded mode, we can't enable the memory controller -- it isn't thread-aware. Signed-off-by: lifubang <lifubang@acmcoder.com>

cyphar · 2020-05-13T09:08:50Z

LGTM.

issues fixed

giuseppe · 2020-05-13T10:23:42Z

@AkihiroSuda @kolyshkin PTAL

kolyshkin · 2020-05-13T21:29:28Z

LGTM

lifubang changed the title ~~cgroupv2: only enable threaded mode when rootless~~ cgroupv2: enable threaded mode only in rootless May 8, 2020

AkihiroSuda reviewed May 8, 2020

View reviewed changes

lifubang force-pushed the threadedordomain branch 2 times, most recently from fd7d8a4 to eea6058 Compare May 10, 2020 03:29

lifubang changed the title ~~cgroupv2: enable threaded mode only in rootless~~ cgroupv2: don't enable threaded mode by default May 10, 2020

lifubang mentioned this pull request May 10, 2020

Partially revert "CreateCgroupPath: only enable needed controllers" #2395

Merged

lifubang force-pushed the threadedordomain branch 2 times, most recently from 106fdd8 to 7ca4d5a Compare May 10, 2020 05:32

AkihiroSuda reviewed May 10, 2020

View reviewed changes

lifubang force-pushed the threadedordomain branch 2 times, most recently from 1e97975 to 757bb65 Compare May 10, 2020 15:52

lifubang force-pushed the threadedordomain branch from 757bb65 to e0c9737 Compare May 10, 2020 23:14

AkihiroSuda reviewed May 10, 2020

View reviewed changes

lifubang force-pushed the threadedordomain branch from e0c9737 to 2aefa92 Compare May 10, 2020 23:31

kolyshkin reviewed May 11, 2020

View reviewed changes

Comment thread libcontainer/cgroups/fs2/create.go Outdated

kolyshkin reviewed May 11, 2020

View reviewed changes

Comment thread libcontainer/cgroups/fs2/create.go Outdated

kolyshkin previously requested changes May 11, 2020

View reviewed changes

lifubang force-pushed the threadedordomain branch from 2aefa92 to d6a76d9 Compare May 11, 2020 04:30

cyphar reviewed May 12, 2020

View reviewed changes

Comment thread libcontainer/cgroups/fs2/create.go Outdated

kolyshkin reviewed May 12, 2020

View reviewed changes

Comment thread libcontainer/cgroups/fs2/create.go Outdated

kolyshkin reviewed May 12, 2020

View reviewed changes

Comment thread tests/rootless.sh Outdated

don't enable threaded mode by default

fe0669b

Because in threaded mode, we can't enable the memory controller -- it isn't thread-aware. Signed-off-by: lifubang <lifubang@acmcoder.com>

lifubang force-pushed the threadedordomain branch from d6a76d9 to fe0669b Compare May 13, 2020 08:27

kolyshkin merged commit 85d4264 into opencontainers:master May 13, 2020

lifubang deleted the threadedordomain branch May 28, 2020 01:31

thaJeztah mentioned this pull request Jul 8, 2022

Specifying cgroup limits on a child container fails with cgroups v2 docker/for-mac#6288

Open

3 tasks

Conversation

lifubang commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lifubang commented May 8, 2020

Uh oh!

cyphar commented May 8, 2020

Uh oh!

AkihiroSuda May 8, 2020

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented May 8, 2020

Uh oh!

cyphar commented May 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lifubang commented May 9, 2020

Uh oh!

AkihiroSuda commented May 10, 2020

Uh oh!

AkihiroSuda May 10, 2020

Choose a reason for hiding this comment

Uh oh!

lifubang commented May 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented May 10, 2020

Uh oh!

AkihiroSuda May 10, 2020

Choose a reason for hiding this comment

Uh oh!

kolyshkin May 11, 2020

Choose a reason for hiding this comment

Uh oh!

lifubang May 11, 2020

Choose a reason for hiding this comment

Uh oh!

cyphar May 12, 2020

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented May 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented May 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cyphar commented May 13, 2020 • edited by AkihiroSuda Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giuseppe commented May 13, 2020

Uh oh!

kolyshkin commented May 13, 2020 • edited by AkihiroSuda Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lifubang commented May 8, 2020 •

edited

Loading

cyphar commented May 9, 2020 •

edited

Loading

lifubang commented May 10, 2020 •

edited

Loading

AkihiroSuda commented May 10, 2020 •

edited

Loading

AkihiroSuda commented May 11, 2020 •

edited

Loading

cyphar commented May 13, 2020 •

edited by AkihiroSuda

Loading

kolyshkin commented May 13, 2020 •

edited by AkihiroSuda

Loading