remove cgroup path recursively in cgroup v2 by lifubang · Pull Request #2412 · opencontainers/runc

lifubang · 2020-05-15T23:39:02Z

fix #2403

If we have subcgroups in a cgroup path, we should remove it recursively.

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

kolyshkin · 2020-05-16T01:54:46Z

We need a test case -- create sub-cgroups, check they are actually removed.

lifubang · 2020-05-17T11:03:33Z

We need a test case -- create sub-cgroups, check they are actually removed.

I add a test case in delete.bats, it test success in my computer(Fedora 31).

root@acm:/opt/mygo/src/github.com/opencontainers/runc# make localintegration TESTPATH=/delete.bats
go build "-mod=vendor" -buildmode=pie  -tags "seccomp selinux apparmor" -ldflags "-X main.gitCommit="15ec27f390f377780e1c332d24588e7e2ba7d3d3-dirty" -X main.version=1.0.0-rc10+dev " -o runc .
go build "-mod=vendor" -buildmode=pie  -tags "seccomp selinux apparmor" -ldflags "-X main.gitCommit="15ec27f390f377780e1c332d24588e7e2ba7d3d3-dirty" -X main.version=1.0.0-rc10+dev " -o contrib/cmd/recvtty/recvtty ./contrib/cmd/recvtty
bats -t tests/integration/delete.bats
1..4
ok 1 runc delete
ok 2 runc delete --force
ok 3 runc delete --force with subcgroups in v2
ok 4 runc delete --force ignore not exist
root@acm:/opt/mygo/src/github.com/opencontainers/runc#

But failed in CI:

not ok 24 runc delete --force with subcgroups in v2
# (in test file tests/integration/delete.bats, line 74)
#   `[ "$status" -eq 0 ]' failed

the line 72-74 is:

# run busybox detached
  runc run -d --console-socket $CONSOLE_SOCKET test_busybox
  [ "$status" -eq 0 ]

So, we can't create rw cgroup mount in CI?
@kolyshkin PTAL, thanks.

AkihiroSuda · 2020-05-20T03:31:04Z

LGTM, though I expect the function name to be more descriptive

AkihiroSuda · 2020-05-20T03:54:59Z

LGTM

AkihiroSuda · 2020-05-20T17:10:05Z

LGTM

kolyshkin · 2020-05-21T00:01:36Z

+// Because there may be subcgroups in it.
+func RemovePathUnified(path string) error {
+	infos, err := ioutil.ReadDir(path)
+	if err == nil {


1 nit:

if err != nil { if os.IsNotExist(err) { err = nil } return err }

so

the rest of function will be less indented

we won't return ENOENT.

kolyshkin · 2020-05-21T00:02:02Z

+		for _, info := range infos {
+			if info.IsDir() {
+				// We should remove subcgroups dir first
+				if err = RemovePathUnified(filepath.Join(path, info.Name())); err != nil && !os.IsNotExist(err) {


with the above suggestion, we won't have to check for IsNotExist here.

kolyshkin · 2020-05-21T00:02:18Z

+				}
+			}
+		}
+		if err == nil || os.IsNotExist(err) {


...and here

kolyshkin · 2020-05-21T00:08:52Z

+			}
+		}
+		if err == nil || os.IsNotExist(err) {
+			return os.Remove(path)


I think we should avoid returning ENOENT from here as well (similar to what os.RemoveAll() does.

kolyshkin · 2020-05-21T00:11:37Z

+// RemovePathUnified aims to remove cgroup path recursively
+// Because there may be subcgroups in it.
+func RemovePathUnified(path string) error {
+	infos, err := ioutil.ReadDir(path)


OK, I think we can optimize things for the common case here:

err := os.Remove(path) if err == nil || os.IsNotExist(err) { return nil } // remove subdirectories first infos, err := ioutil.ReadDir(path) ....

I still think it's a pretty good optimization. Will follow up.

kolyshkin · 2020-05-21T00:16:17Z


+// RemovePathUnified aims to remove cgroup path recursively
+// Because there may be subcgroups in it.
+func RemovePathUnified(path string) error {


Since we're only using this function from fs2, why don't we move it to fs2 and make it private.

Maybe rename to removeCgroupPath or so.

kolyshkin · 2020-05-21T00:21:45Z

left an optimization suggestion for RemovePathUnified, and a few nits.

Found another problem as well -- the test case added works even without the changes. I think we need to make sure the test case fails, then make it work, otherwise it's not clear what are we fixing here. Maybe the sub-cgroup need to have some limits set? IDK.

$ git fetch lifubang 
...
$ git checkout removecgpath
Branch 'removecgpath' set up to track remote branch 'removecgpath' from 'lifubang'.
Switched to a new branch 'removecgpath'

$ git show libcontainer/ | patch -p1 -R # revert the "fixing" part
patching file libcontainer/cgroups/fs2/fs2.go
patching file libcontainer/cgroups/utils.go

$ sudo -s
# RUNC_USE_SYSTEMD=yes make localintegration TESTPATH=/delete.bats
go build "-mod=vendor" -buildmode=pie  -tags "seccomp selinux apparmor" -ldflags "-X main.gitCommit="430a1e88c359974ba441cf75b619626cece7c261-dirty" -X main.version=1.0.0-rc10+dev " -o runc .
go build "-mod=vendor" -buildmode=pie  -tags "seccomp selinux apparmor" -ldflags "-X main.gitCommit="430a1e88c359974ba441cf75b619626cece7c261-dirty" -X main.version=1.0.0-rc10+dev " -o contrib/cmd/recvtty/recvtty ./contrib/cmd/recvtty
bats -t tests/integration/delete.bats
1..4
ok 1 runc delete
ok 2 runc delete --force
ok 3 runc delete --force ignore not exist
ok 4 runc delete --force in cgroupv2 with subcgroups

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

AkihiroSuda · 2020-05-26T22:10:43Z

LGTM

mrunalp · 2020-05-27T22:50:10Z

LGTM

kolyshkin · 2020-05-28T18:25:36Z

+  # create subcgroups
+  cat <<EOF > nest.sh
+  cd /sys/fs/cgroup
+  for f in \$(cat cgroup.controllers); do echo +\$f > cgroup.subtree_control; done


This gives an error on my system when trying to enable io controller.

Most probably just +pids is enough (and it won't error).

kolyshkin · 2020-05-28T18:26:11Z

+
+  # create a sub process
+  __runc exec -d test_busybox sleep 1d
+  [ "$status" -eq 0 ]


__runc does not set $status, so this check is checking and older status.

kolyshkin · 2020-05-28T18:26:47Z

+  [ "$status" -eq 0 ]
+
+  # find the pid of sleep
+  pid=$(__runc exec test_busybox ps -a | grep 1d | awk '{print $1}')


busybox ps does not have -a, but fortunately it is ignored.

kolyshkin · 2020-05-28T18:27:16Z

+  cat cgroup.threads
+EOF
+  cat nest.sh | runc exec test_busybox sh
+  [[ ${output} =~ [0-9]+ ]]


you don't check the exit status

kolyshkin · 2020-05-28T18:28:07Z

+
+  # create subcgroups
+  cat <<EOF > nest.sh
+  cd /sys/fs/cgroup


makes sense to add set -x -u -e

kolyshkin · 2020-05-28T18:31:26Z

+
+  # check cgroup.threads' value
+  runc exec test_busybox cat /sys/fs/cgroup/foo/cgroup.threads
+  [[ ${output} =~ [0-9]+ ]]


We have already checked it above (the output of nest.sh script), looks like this check is redundant.

kolyshkin reviewed May 16, 2020

View reviewed changes

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

kolyshkin added area/cgroupv2 kind/bug labels May 16, 2020

lifubang force-pushed the removecgpath branch from 61b6f52 to 15ec27f Compare May 17, 2020 10:07

lifubang marked this pull request as draft May 17, 2020 10:46

kolyshkin mentioned this pull request May 18, 2020

cgroupv2 support meta issue #2315

Closed

lifubang commented May 18, 2020

View reviewed changes

Comment thread tests/integration/delete.bats Outdated

lifubang force-pushed the removecgpath branch from f5d6b94 to 65dd995 Compare May 19, 2020 09:56

kolyshkin reviewed May 19, 2020

View reviewed changes

Comment thread tests/integration/delete.bats Outdated

kolyshkin reviewed May 19, 2020

View reviewed changes

Comment thread tests/integration/delete.bats Outdated

lifubang force-pushed the removecgpath branch from 65dd995 to 98a5239 Compare May 19, 2020 23:22

lifubang commented May 20, 2020

View reviewed changes

Comment thread tests/integration/delete.bats Outdated

lifubang force-pushed the removecgpath branch 2 times, most recently from 9f06248 to 2b1da44 Compare May 20, 2020 02:49

lifubang marked this pull request as ready for review May 20, 2020 03:23

AkihiroSuda reviewed May 20, 2020

View reviewed changes

Comment thread libcontainer/cgroups/utils.go Outdated

lifubang force-pushed the removecgpath branch from 2b1da44 to be92ba8 Compare May 20, 2020 03:46

lifubang commented May 20, 2020

View reviewed changes

Comment thread libcontainer/cgroups/utils.go Outdated

lifubang force-pushed the removecgpath branch from be92ba8 to 430a1e8 Compare May 20, 2020 15:43

kolyshkin reviewed May 21, 2020

View reviewed changes

Comment thread libcontainer/cgroups/utils.go Outdated

}

}

}

if err == nil || os.IsNotExist(err) {

Copy link
Copy Markdown

Contributor

kolyshkin May 21, 2020

Choose a reason for hiding this comment

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

...and here

kolyshkin reviewed May 21, 2020

View reviewed changes

remove cgroup path recursively in cgroup v2

82fa194

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

lifubang force-pushed the removecgpath branch from 430a1e8 to 82fa194 Compare May 26, 2020 15:35

mrunalp merged commit 4f0bdaf into opencontainers:master May 27, 2020

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

kolyshkin reviewed May 28, 2020

View reviewed changes

kolyshkin mentioned this pull request May 28, 2020

cgroups/fs2: make removeCgroupPath faster #2437

Merged

Conversation

lifubang commented May 15, 2020

Uh oh!

Uh oh!

kolyshkin commented May 16, 2020

Uh oh!

lifubang commented May 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented May 21, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AkihiroSuda commented May 20, 2020 •

edited

Loading

AkihiroSuda commented May 20, 2020 •

edited

Loading

AkihiroSuda commented May 20, 2020 •

edited

Loading

AkihiroSuda commented May 26, 2020 •

edited

Loading

mrunalp commented May 27, 2020 •

edited by AkihiroSuda

Loading