Simpler v2 cgroup interface by crosbymichael · Pull Request #109 · containerd/cgroups

crosbymichael · 2019-11-05T18:38:48Z

The resource type is changed in this but I would rather have a v2 native
Resource type and do a mapping from an OCI spec's resource to the v2.Resource
type.

ref #104

Signed-off-by: Michael Crosby crosbymichael@gmail.com

crosbymichael · 2019-11-05T18:40:48Z

@AkihiroSuda and @Zyqsempai please take a look at this new approach. it should make things much easier to manage and extend.

codecov-io · 2019-11-05T18:46:46Z

Codecov Report

Merging #109 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #109   +/-   ##
=======================================
  Coverage   45.42%   45.42%           
=======================================
  Files          23       23           
  Lines        1638     1638           
=======================================
  Hits          744      744           
  Misses        768      768           
  Partials      126      126

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0a1dbc...54a40c2. Read the comment docs.

v2/controller.go

v2/utils.go

v2/controller.go

v2/paths.go

v2/cpu.go

AkihiroSuda · 2019-11-06T03:19:59Z

v2/controller.go

+}
+
+func (c *Controller) Delete() error {
+	return remove(c.path)


can be just os.Remove?

I think we still need the remove all logic but i'm not sure if this is more robust in v2

AkihiroSuda · 2019-11-06T03:22:19Z

cmd/cgroups-playground needs to be updated

The resource type is changed in this but I would rather have a v2 native Resource type and do a mapping from an OCI spec's resource to the v2.Resource type. Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

v2/manager.go

v2/paths.go

Zyqsempai

You removed everything from playground, do we have any chance to test your solution?

v2/memory.go

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

crosbymichael · 2019-11-06T17:02:00Z

Ok, pushed a big update. This has stats implemented along with renaming the playground tool to cgctl with a couple methods to play with this.

NAME:
   cgctl - cgroup v2 management tool

USAGE:
   cgctl [global options] command [command options] [arguments...]

VERSION:
   1

COMMANDS:
   new      create a new cgroup
   del      delete a cgroup
   list     list processes in a cgroup
   stat     stat a cgroup
   help, h  Shows a list of commands or help for one command

GLOBAL OPTIONS:
   --debug             enable debug output in the logs
   --mountpoint value  cgroup mountpoint (default: "/sys/fs/cgroup")
   --help, -h          show help
   --version, -v       print the version

> sudo ./cgctl new --enable test
> sudo ./cgctl list test
> sudo ./cgctl stat test
> sudo ./cgctl del test

AkihiroSuda · 2019-11-06T17:06:32Z

cmd/cgctl/main.go

+			if err != nil {
+				return err
+			}
+			if err := c.ToggleControllers(controllers, v2.Enable); err != nil {


This needs to be done for the parent group?

you don't need to enable this at all if the system is setup to enable them

When you have cpu in the top group /sys/fs/cgroup, cpu is enabled for /sys/fs/cgroup/foo, but not enabled for /sys/fs/cgroup/foo/bar.

root@suda-ws01:/sys/fs/cgroup# cat cgroup.controllers cpuset cpu io memory pids rdma root@suda-ws01:/sys/fs/cgroup# mkdir foo root@suda-ws01:/sys/fs/cgroup# cat foo/cgroup.controllers cpu io memory pids root@suda-ws01:/sys/fs/cgroup# mkdir foo/bar root@suda-ws01:/sys/fs/cgroup# cat foo/bar/cgroup.controllers root@suda-ws01:/sys/fs/cgroup#

yes, its up to the creator of the cgroup to enable these. I don't think the sub cgroup should modify the parent settings

AkihiroSuda · 2019-11-06T17:07:32Z

v2/paths.go

-
-// VerifyGroupPath verifies the format of g.
-// VerifyGroupPath doesn't verify whether g actually exists on the system.
-func VerifyGroupPath(g GroupPath) error {


this function can be kept

AkihiroSuda · 2019-11-06T17:52:47Z

v2/manager.go

+	}
+	out := make(map[string]uint64)
+	for _, controller := range controllers {
+		filename := fmt.Sprintf("%s.stat", controller)


how will it work for other files such as pids.current?

Probably we should either add file name to key or return map[string]map[string]interface{}

i'ts still a uint

crosbymichael · 2019-11-06T18:07:19Z

We don't have to fix everything in this PR, it's meant to show that we can simplify things for v2 and to get this in so we can carry on the work

Zyqsempai

I agree with @crosbymichael we shouldn't waste our time on perfecting this particular Pr, anyway, we have a lot of work to do so far. So I think we can merge it and polish it little by little in further PR's.

estesp

LGTM

containerd#109 (comment) Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda · 2019-11-07T04:43:44Z

follow-up: #110

split fs2 package from fs, as mixing up fs and fs2 is very likely to result in unmaintainable code. Inspired by containerd/cgroups#109 Fix opencontainers#2157 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

split fs2 package from fs, as mixing up fs and fs2 is very likely to result in unmaintainable code. Inspired by containerd/cgroups#109 Fix #2157 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

split fs2 package from fs, as mixing up fs and fs2 is very likely to result in unmaintainable code. Inspired by containerd#109 Fix #2157 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

crosbymichael force-pushed the v2-controller branch from 95a78dc to df36c14 Compare November 5, 2019 18:48

Zyqsempai reviewed Nov 5, 2019

View reviewed changes

v2/controller.go Outdated Show resolved Hide resolved

v2/controller.go Outdated Show resolved Hide resolved

v2/controller.go Outdated Show resolved Hide resolved

v2/controller.go Outdated Show resolved Hide resolved

v2/controller.go Outdated Show resolved Hide resolved

fuweid reviewed Nov 6, 2019

View reviewed changes

v2/controller.go Outdated Show resolved Hide resolved

fuweid reviewed Nov 6, 2019

View reviewed changes

v2/controller.go Outdated Show resolved Hide resolved

fuweid reviewed Nov 6, 2019

View reviewed changes

v2/utils.go Show resolved Hide resolved

AkihiroSuda reviewed Nov 6, 2019

View reviewed changes

v2/controller.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Nov 6, 2019

View reviewed changes

v2/controller.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Nov 6, 2019

View reviewed changes

v2/paths.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Nov 6, 2019

View reviewed changes

v2/cpu.go Show resolved Hide resolved

AkihiroSuda reviewed Nov 6, 2019

View reviewed changes

crosbymichael force-pushed the v2-controller branch 2 times, most recently from 3537de5 to 677330f Compare November 6, 2019 15:36

Simpler v2 cgroup interface

3239d7b

The resource type is changed in this but I would rather have a v2 native Resource type and do a mapping from an OCI spec's resource to the v2.Resource type. Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

crosbymichael force-pushed the v2-controller branch from 677330f to 3239d7b Compare November 6, 2019 15:40

AkihiroSuda reviewed Nov 6, 2019

View reviewed changes

v2/manager.go Show resolved Hide resolved

v2/paths.go Outdated Show resolved Hide resolved

v2/paths.go Outdated Show resolved Hide resolved

Zyqsempai reviewed Nov 6, 2019

View reviewed changes

v2/memory.go Outdated Show resolved Hide resolved

crosbymichael added 2 commits November 6, 2019 11:39

Rename playground to cgctl

7bf30d7

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

Add basic stats implementation

54a40c2

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

AkihiroSuda reviewed Nov 6, 2019

View reviewed changes

Zyqsempai reviewed Nov 6, 2019

View reviewed changes

AkihiroSuda approved these changes Nov 6, 2019

View reviewed changes

estesp approved these changes Nov 6, 2019

View reviewed changes

estesp merged commit 82928e8 into containerd:master Nov 6, 2019

crosbymichael deleted the v2-controller branch November 6, 2019 20:04

AkihiroSuda added a commit to AkihiroSuda/containerd-cgroups that referenced this pull request Nov 7, 2019

v2: restore VerifyGroupPath

788d082

containerd#109 (comment) Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda mentioned this pull request Nov 7, 2019

v2: restore VerifyGroupPath; v2: fix TestParseCgroupFromReader #110

Merged

AkihiroSuda mentioned this pull request Nov 7, 2019

cgroup2: split fs2 from fs opencontainers/runc#2169

Merged

Conversation

crosbymichael commented Nov 5, 2019

Uh oh!

crosbymichael commented Nov 5, 2019

Uh oh!

codecov-io commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Nov 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zyqsempai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crosbymichael commented Nov 6, 2019

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!

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented Nov 6, 2019

Uh oh!

Zyqsempai left a comment

Choose a reason for hiding this comment

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Nov 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Nov 5, 2019 •

edited

Loading