cgroup2: unshare cgroupns by default regardless to API version by AkihiroSuda · Pull Request #41072 · moby/moby

AkihiroSuda · 2020-06-05T09:05:32Z

- What I did
Fix #41071

cgroup2 mode sets cgroupns=private by default , but the default was overridden to host when API < 1.41.

- How to verify it

$ sudo ls -l /proc/1/ns/cgroup
lrwxrwxrwx 1 root root 0 Jun  5 18:04 /proc/1/ns/cgroup -> 'cgroup:[4026531835]'
$ DOCKER_API_VERSION=1.40 docker run --rm alpine ls -l /proc/1/ns/cgroup
lrwxrwxrwx    1 root     root             0 Jun  5 09:04 /proc/1/ns/cgroup -> cgroup:[4026532639]

- A picture of a cute animal (not mandatory but encouraged)
🐧

api/server/router/container/container_routes.go

cmd/dockerd/daemon.go

thaJeztah · 2020-06-11T19:21:57Z

ping @kolyshkin PTAL

api/server/router/container/container_routes.go

kolyshkin

Can simplify it a lot by using cgroups.IsCgroup2UnifiedMode() in place

Fix moby#41071 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda · 2020-06-15T07:16:49Z

@kolyshkin That package only compiles on Linux and cannot be imported in cmd/dockerd/daemon.go.

AkihiroSuda · 2020-06-18T07:19:07Z

@kolyshkin @thaJeztah @cpuguy83 PTAL

cpuguy83 · 2020-06-18T18:40:33Z

Should this be in the router? If we are doing this regardless of API version, seems like the router doesn't need to know about v2.

thaJeztah · 2020-06-18T18:42:15Z

^ good point

kolyshkin · 2020-06-18T19:25:17Z

Should this be in the router? If we are doing this regardless of API version, seems like the router doesn't need to know about v2.

It's the router who resets the cgroupns=private (default for cgroupv2) setting for older clients. Chances are, older clients don't know/care about cgroupv2, so the reset should only be done for cgroupv1, thus the need for router to know whether its v1 or v2.

kolyshkin

LGTM

cpuguy83 · 2020-06-18T20:18:02Z

I don't think we should contort the router to have system level information because we can't tell if a value passed from the router is a default value or not.

kolyshkin · 2020-06-18T21:53:21Z

I don't think we should contort the router to have system level information

So, you are suggesting we should perform the change somewhere else. Fine, but...

we can't tell if a value passed from the router is a default value or not

...this is exactly what the problem is. We do not know if a cgroupns value that comes from the router is default or not, so we don't know if we need to change it or not :-\

IOW, if you don't like the way how it's currently done, can you please propose an alternative way to do the same thing?

cpuguy83 · 2020-06-19T19:00:27Z

Maybe not perfect for future (albeit highly unlikely to have changes around this case), but telling the router what the default should be seems cleaner.

I also opened #41129 to discuss how to handle this across the board because it is problematic in other places as well.

AkihiroSuda · 2020-06-22T05:20:38Z

Can we merge this PR and discuss the router design in #41129 ?

cpuguy83 · 2020-06-22T16:30:32Z

I would rather not add extra compexity like this just to get the change in.
I think it's also a bit of a stretch to make this change to older API versions.

justincormack · 2020-09-25T13:43:10Z

Its not safe to run cgroupsv2 unnamespaced it appears, so I think we have to make this change for previous API versions.

cpuguy83 · 2020-09-25T19:10:29Z

@justincormack I was curious about that. Good catch.

So we can implement this totally in the daemon and not touch the API side.

AkihiroSuda · 2020-09-29T05:36:00Z

not touch the API side.

API server has if hostConfig.CgroupnsMode.IsEmpty() { hostConfig.CgroupnsMode = container.CgroupnsMode("host") } , so this PR modifies API server to disable this block for cgroup v2

kolyshkin

LGTM

thaJeztah

LGTM

cpuguy83

LGTM

It'd be nice to rework this a bit, but we need this in.

thaJeztah reviewed Jun 5, 2020

View reviewed changes

api/server/router/container/container_routes.go Outdated Show resolved Hide resolved

AkihiroSuda force-pushed the fix-41071 branch from 4756515 to 938a00f Compare June 5, 2020 10:07

thaJeztah reviewed Jun 5, 2020

View reviewed changes

cmd/dockerd/daemon.go Outdated Show resolved Hide resolved

thaJeztah added this to the 20.03.0 milestone Jun 11, 2020

thaJeztah added impact/changelog status/2-code-review labels Jun 11, 2020

kolyshkin reviewed Jun 12, 2020

View reviewed changes

api/server/router/container/container_routes.go Outdated Show resolved Hide resolved

kolyshkin requested changes Jun 12, 2020

View reviewed changes

AkihiroSuda force-pushed the fix-41071 branch from 938a00f to e7bd39e Compare June 12, 2020 08:51

cgroup2: unshare cgroupns by default regardless to API version

79cfcba

Fix moby#41071 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda force-pushed the fix-41071 branch from e7bd39e to 79cfcba Compare June 15, 2020 07:15

kolyshkin approved these changes Jun 18, 2020

View reviewed changes

AkihiroSuda mentioned this pull request Jun 30, 2020

v20.10 docs for cgroup v2 and rootless docker/docs#11042

Merged

kolyshkin approved these changes Oct 1, 2020

View reviewed changes

thaJeztah approved these changes Oct 1, 2020

View reviewed changes

cpuguy83 approved these changes Oct 1, 2020

View reviewed changes

cpuguy83 merged commit 816fbcd into moby:master Oct 1, 2020

AkihiroSuda mentioned this pull request Jan 22, 2021

TestCgroupNamespacesRunOlderClient: support cgroup v2 #41917

Merged

AkihiroSuda added the area/cgroup2 cgroup v2 label Jan 23, 2021

fredericdalleau mentioned this pull request May 18, 2022

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

Open

3 tasks

Conversation

AkihiroSuda commented Jun 5, 2020

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Jun 11, 2020

Uh oh!

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Jun 15, 2020

Uh oh!

AkihiroSuda commented Jun 18, 2020

Uh oh!

cpuguy83 commented Jun 18, 2020

Uh oh!

thaJeztah commented Jun 18, 2020

Uh oh!

kolyshkin commented Jun 18, 2020

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Jun 18, 2020

Uh oh!

kolyshkin commented Jun 18, 2020

Uh oh!

cpuguy83 commented Jun 19, 2020

Uh oh!

AkihiroSuda commented Jun 22, 2020

Uh oh!

cpuguy83 commented Jun 22, 2020

Uh oh!

justincormack commented Sep 25, 2020

Uh oh!

cpuguy83 commented Sep 25, 2020

Uh oh!

AkihiroSuda commented Sep 29, 2020

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants