Start containers in their own cgroup namespaces by rgulewich · Pull Request #38377 · moby/moby

rgulewich · 2018-12-14T23:15:38Z

- What I did

This adds cgroup namespace support to docker. It's enabled for all containers that are not run with --privileged, if the kernel supports it.

Fixes #38332.

- How I did it

Now that runc supports cgroup namespaces, just add a namespace of type "cgroup" to the list.

At daemon startup, this looks for /proc/self/ns/cgroup to be present to determine whether or not to enable it.

- How to verify it

Run a new container and verify that its pid 1 is running in its own cgroup namespace.

- Description for the changelog

Add cgroup namespace support for Linux containers

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

GordonTheTurtle · 2018-12-14T23:15:40Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "38332-cgroup-ns" [email protected]:rgulewich/moby.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

thaJeztah

Thanks! Only gave this a cursory glance, but left some comments.

Looks like this information will be returned by the /info endpoint, so we'll need changes to the swagger file https://github.com/moby/moby/blob/master/api/swagger.yaml, and the API changelog https://github.com/moby/moby/blob/master/docs/api/version-history.md as well

thaJeztah · 2018-12-15T00:54:43Z

integration/container/run_linux_test.go

Perhaps we should have a testEnv.SupportsCgroupNS() utility for this 🤔

thaJeztah · 2018-12-15T02:14:06Z

pkg/sysinfo/sysinfo_linux.go

We may want to extract it to a function, e.g.

Suggested change

sysInfo.CgroupNamespaces = true

sysInfo.CgroupNamespaces = cgroupNamespacesSupported()

Looking at that, perhaps this should be a utility in libcontainer; somewhere around here; https://github.com/opencontainers/runc/blob/f5b99917df9fbe1d9a4114966fb088dd6860e85a/libcontainer/cgroups/utils.go#L30-L45

and/or in the containerd/cgroups package https://github.com/containerd/cgroups/blob/master/utils.go

Good idea - it makes sense to me for this to live in libcontainer. What do you think about adding it in its own sub-package of libcontainer? Everything in the libcontainer/cgroups package deals with cgroups themselves, whereas this is actually a namespace - I worry that putting them together in the cgroups package might make this more confusing.

So maybe something like libcontainer/namespace/cgroups.go?

pkg/sysinfo/sysinfo_linux.go

thaJeztah · 2018-12-15T02:20:35Z

daemon/oci_linux.go

Wondering if this should be enabled by default if the host support it, or if it should be an opt-in/opt-out

I'm not too familiar with the feature itself, but my train of thought is; is this causing a change in behavior? Would there be situations where namespaced cgroups are not desirable?

@AkihiroSuda @crosbymichael ?

I was on the fence about this myself - I listed a couple other options in the GH issue. @cpuguy83, any thoughts?

The main change in behaviour is that /proc/self/cgroup will now hide the Docker-related stuff. Personally I think it should be enabled by default and shouldn't be daemon-wide at all -- it should be treated like all other namespaces. My only worry is that sharing a cgroup with --cgroup-parent will now become more complicated because if you have two containers in the same cgroup but in different cgroup namespaces then things start to get a bit weird (especially with nsdelegate thrown in).

Sorry, I missed this notification.

Agree we should enable by default, but make sure not to break users who expect things like cgroup-parent and privileged to work a certain way.

The current behaviour is to enable it by default (just like any other namespace), except if the container is privileged.

The main side effect (which shows up in the integration tests), is that the cgroup hierarchy is no longer visible from inside the container. I don't know if this qualifies as a breaking change or not.

@cyphar / @cpuguy83, what are your concerns about running with --cgroup-parent? I can add some integration tests to confirm the behaviour.

The main side effect (which shows up in the integration tests), is that the cgroup hierarchy is no longer visible from inside the container. I don't know if this qualifies as a breaking change or not.

This is the main purpose of cgroup namespaces, so it's good that it works. 😉 I don't think it'll count as a breaking change unless some folks have really tightly integrated into what /proc/self/cgroups contains which really isn't a good idea anyway.

what are your concerns about running with --cgroup-parent?

Since writing the above comment I've remembered that cgroup sharing doesn't exist in Docker (getting two containers to share a cgroup) and so --cgroup-parent shouldn't be a problem after all. The concern would be that if you have two containers sharing a cgroup you would want them to share cgroup namespaces.

We do support sharing of most namespaces, so we could add support for cgroup namespace sharing.

pkg/sysinfo/sysinfo.go

thaJeztah · 2018-12-15T02:36:14Z

CI failures look related

23:54:57 FAIL: docker_cli_build_test.go:3973: DockerSuite.TestBuildContainerWithCgroupParent
23:54:57 
23:54:57 docker_cli_build_test.go:3996:
23:54:57     c.Fatalf("There is no expected memory cgroup with parent /%s/: %s", cgroupParent, result.Combined())
23:54:57 ... Error: There is no expected memory cgroup with parent /test/: Sending build context to Docker daemon  2.048kB


23:54:57 Step 1/2 : FROM busybox
23:54:57  ---> 6ad733544a63
23:54:57 Step 2/2 : RUN cat /proc/self/cgroup
23:54:57  ---> Running in a799443f87c8
23:54:57 11:devices:/
23:54:57 10:pids:/
23:54:57 9:cpuset:/
23:54:57 8:cpu,cpuacct:/
23:54:57 7:freezer:/
23:54:57 6:net_cls,net_prio:/
23:54:57 5:hugetlb:/
23:54:57 4:blkio:/
23:54:57 3:perf_event:/
23:54:57 2:memory:/
23:54:57 1:name=systemd:/
23:54:57 Removing intermediate container a799443f87c8
23:54:57  ---> 48b98162374e
23:54:57 Successfully built 48b98162374e
23:54:57 Successfully tagged buildcgroupparent:latest
23:54:57 

00:15:14 FAIL: docker_cli_run_test.go:3237: DockerSuite.TestRunContainerWithCgroupParent
00:15:14 
00:15:14 docker_cli_run_test.go:3242:
00:15:14     // cgroup-parent relative path
00:15:14     testRunContainerWithCgroupParent(c, "test", "cgroup-test")
00:15:14 docker_cli_run_test.go:3267:
00:15:14     c.Fatalf("unexpected cgroup paths. Expected at least one cgroup path to have suffix %q. Cgroup Paths: %v", expectedCgroup, cgroupPaths)
00:15:14 ... Error: unexpected cgroup paths. Expected at least one cgroup path to have suffix "test/ee671df4af17e4f7594f6199410af9e09025b4cefb4f3fdef3391f35eaf976d5". Cgroup Paths: map[cpu,cpuacct:/ hugetlb:/ blkio:/ memory:/ name=systemd:/ perf_event:/ devices:/ pids:/ cpuset:/ freezer:/ net_cls,net_prio:/]
00:15:14 


00:16:09 FAIL: docker_cli_run_test.go:3272: DockerSuite.TestRunInvalidCgroupParent
00:16:09 
00:16:09 docker_cli_run_test.go:3276:
00:16:09     testRunInvalidCgroupParent(c, "../../../../../../../../SHOULD_NOT_EXIST", "SHOULD_NOT_EXIST", "cgroup-invalid-test")
00:16:09 docker_cli_run_test.go:3307:
00:16:09     c.Fatalf("unexpected cgroup paths. Expected at least one cgroup path to have suffix %q. Cgroup Paths: %v", expectedCgroup, cgroupPaths)
00:16:09 ... Error: unexpected cgroup paths. Expected at least one cgroup path to have suffix "SHOULD_NOT_EXIST/42c0cf9e9b4892b42a23b57f943d417a52a8373608c7d69b32e56bc528bc0761". Cgroup Paths: map[pids:/ net_cls,net_prio:/ blkio:/ perf_event:/ name=systemd:/ devices:/ cpuset:/ cpu,cpuacct:/ freezer:/ hugetlb:/ memory:/]
00:16:09 


00:24:18 FAIL: docker_cli_daemon_test.go:1857: DockerDaemonSuite.TestDaemonCgroupParent
00:24:18 
00:24:18 [d2f8a22850347] waiting for daemon to start
00:24:18 [d2f8a22850347] daemon started
00:24:18 
00:24:18 docker_cli_daemon_test.go:1881:
00:24:18     c.Assert(found, checker.True, check.Commentf("Cgroup path for container (%s) doesn't found in cgroups file: %s", expectedCgroup, cgroupPaths))
00:24:18 ... obtained bool = false
00:24:18 ... Cgroup path for container (test/0c465dd7a395dc6ce8c0d6a292233f3580b0192f20db92712fb892077c8c8268) doesn't found in cgroups file: map[devices:/ pids:/ cpuset:/ freezer:/ perf_event:/ memory:/ name=systemd:/ cpu,cpuacct:/ net_cls,net_prio:/ hugetlb:/ blkio:/]
00:24:18 
00:24:18 [d2f8a22850347] exiting daemon
00:24:18 [d2f8a22850347] waiting for daemon to start
00:24:18 [d2f8a22850347] daemon started
00:24:18 
00:24:18 [d2f8a22850347] exiting daemon
00:24:18

codecov · 2018-12-16T00:05:27Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b20a14b). Click here to learn what that means.
The diff coverage is 36.58%.

@@            Coverage Diff            @@
##             master   #38377   +/-   ##
=========================================
  Coverage          ?   36.99%           
=========================================
  Files             ?      612           
  Lines             ?    45468           
  Branches          ?        0           
=========================================
  Hits              ?    16823           
  Misses            ?    26352           
  Partials          ?     2293

rgulewich · 2018-12-16T00:40:37Z

@thaJeztah This change doesn't change the /info endpoint on my machine:

$ curl -sS http://localhost:4243/v1.26/info | jq . | grep -i cgroup
  "CgroupDriver": "cgroupfs",

Should it be added to that endpoint?

olljanat · 2018-12-21T17:02:17Z

@rgulewich plz check failing tests

olljanat · 2019-01-11T06:57:20Z

(reserved for my derek commands)

olljanat · 2019-01-12T10:36:39Z

@rgulewich #38426 removed SameHostDaemon check so CI fails now to:

07:13:07 # github.com/docker/docker/integration-cli [github.com/docker/docker/integration-cli.test]
07:13:07 integration-cli/docker_cli_daemon_test.go:1858:18: undefined: SameHostDaemon
07:13:07 integration-cli/docker_cli_run_test.go:3239:18: undefined: SameHostDaemon

so can your rebase and update tests?

rgulewich · 2019-01-14T23:17:22Z

@olljanat Pushed changes. The experimental failures don't look to be related, unless I'm mistaken.

rgulewich · 2019-02-14T18:47:22Z

Would anyone be able to give a more thorough review of this? I agree with @cyphar's comment in opencontainers/runc#1976 that moving the detection code into libcontainer isn't really necessary (though that PR is there if that's preferable). @cpuguy83, would you mind checking that any concerns you have are covered?

@AkihiroSuda / @yongtang / @olljanat / @kolyshkin / @justincormack - you all look to have committed code in this area somewhat recently. Would any of you mind reviewing?

Thanks!

cpuguy83 · 2019-02-19T22:34:17Z

I'm a bit worried that we are having to change tests and as such making a breaking change... but maybe the tests are just not good tests.
I feel like the cgroup API is pretty well defined here and depending on a particular heirarchy for a cgroup seems wrong.... but still worried that people could be doing this, particularly because there's no way to turn it off.

rgulewich · 2019-02-20T04:37:47Z

@cpuguy83 - Fair enough. I wouldn't want to break someone who was depending on this behaviour. Would a run-time flag to enable / disable this for the entire daemon be sufficient, do you think?

yongtang · 2019-05-12T03:18:25Z

All tests passed, merging.

AkihiroSuda · 2019-07-09T13:16:50Z

Is there docker CLI pr for overriding default-cgroupns-mode ?

thaJeztah · 2019-07-09T13:56:15Z

@AkihiroSuda I don't think there is one yet (at least; I don't see one linked)

tonistiigi · 2019-07-18T16:40:18Z

@rgulewich TestCgroupNamespacesRunPrivileged added in this test seems to be failing consistently with

assertion failed: error is not nil: Error response from daemon: privileged mode is incompatible with private cgroup namespaces. You must run the container in the host cgroup namespace when running privileged mode

eg. https://jenkins.dockerproject.org/job/Docker%20Master/label=centos-7-noselinux-devicemapper/10631/consoleFull

It doesn't fail in PRs and in master just because it is skipped because of the system condition checks. Can you follow-up please?

rgulewich · 2019-07-19T23:09:55Z

@rgulewich TestCgroupNamespacesRunPrivileged added in this test seems to be failing consistently with

assertion failed: error is not nil: Error response from daemon: privileged mode is incompatible with private cgroup namespaces. You must run the container in the host cgroup namespace when running privileged mode

eg. https://jenkins.dockerproject.org/job/Docker%20Master/label=centos-7-noselinux-devicemapper/10631/consoleFull

It doesn't fail in PRs and in master just because it is skipped because of the system condition checks. Can you follow-up please?

Good catch, @tonistiigi - please see my PR here: #39578

rgulewich requested a review from vdemeester as a code owner December 14, 2018 23:15

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature status/0-triage and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Dec 14, 2018

thaJeztah requested changes Dec 15, 2018

View reviewed changes

thaJeztah added status/1-design-review status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/0-triage labels Dec 15, 2018

GordonTheTurtle assigned aaronlehmann Dec 29, 2018

derek bot added rebuild/z and removed rebuild/z status/failing-ci Indicates that the PR in its current state fails the test suite labels Jan 11, 2019

GordonTheTurtle removed the rebuild/z label Jan 11, 2019

derek bot added the rebuild/experimental label Jan 15, 2019

GordonTheTurtle removed the rebuild/experimental label Jan 15, 2019

derek bot added the rebuild/windowsRS1 label Jan 15, 2019

GordonTheTurtle removed the rebuild/windowsRS1 label Jan 15, 2019

rgulewich mentioned this pull request Feb 8, 2019

Add utility function for checking if cgroup namespaces are enabled opencontainers/runc#1975

Closed

yongtang added the rebuild/windowsRS1 label May 9, 2019

GordonTheTurtle removed the rebuild/windowsRS1 label May 9, 2019

yongtang added rebuild/experimental labels May 9, 2019

GordonTheTurtle removed rebuild/experimental labels May 9, 2019

yongtang merged commit 3042254 into moby:master May 12, 2019

thaJeztah added impact/changelog docs/revisit impact/api labels May 13, 2019

rgulewich mentioned this pull request May 13, 2019

libcontainer: cgroup namespaces enabled utility function opencontainers/runc#1976

Closed

thaJeztah mentioned this pull request May 13, 2019

Bump API version to v1.41 #39208

Merged

This was referenced Jun 7, 2019

[19.03 backport] Added garbage collector for image layers docker-archive/engine#268

Closed

[19.03 backport] Integration: change signatures to fix golint warnings docker-archive/engine#281

Merged

AkihiroSuda mentioned this pull request Jul 9, 2019

libpod: support for cgroup namespace containers/podman#3509

Merged

AkihiroSuda mentioned this pull request Jul 9, 2019

docker run flag for overriding daemon default-cgroupns-mode docker/cli#1988

Closed

rgulewich mentioned this pull request Jul 19, 2019

Fix running in privileged mode against a daemon with --default-cgroupns-mode=host #39578

Merged

thaJeztah mentioned this pull request Jan 25, 2020

[19.03 backport] Added garbage collector for image layers #40412

Closed

4 tasks

thaJeztah added this to the 20.03.0 milestone Apr 2, 2020

AkihiroSuda mentioned this pull request Apr 7, 2020

support --privileged --cgroupns=private on cgroup v1 #40788

Closed

This was referenced Jan 26, 2021

Figure out if we should use read-write mounts in non-privileged containerd containers concourse/concourse#6471

Closed

Determine if containers should be started in their own cgroup namespaces concourse/concourse#6474

Closed

taylorsilva mentioned this pull request Feb 8, 2021

Start non-privileged containers in their own cgroup namespace concourse/concourse#6492

Merged

11 tasks

MarkKoz mentioned this pull request Dec 19, 2021

Cgroupv2: root cgroup.subtree_control is empty and cannot be written to when a private cgroup namespace is used #43093

Closed

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

	sysInfo.CgroupNamespaces = true
	sysInfo.CgroupNamespaces = cgroupNamespacesSupported()

Conversation

rgulewich commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GordonTheTurtle commented Dec 14, 2018

Uh oh!

thaJeztah left a comment

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!

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!

Uh oh!

thaJeztah commented Dec 15, 2018

Uh oh!

codecov bot commented Dec 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rgulewich commented Dec 16, 2018

Uh oh!

olljanat commented Dec 21, 2018

Uh oh!

olljanat commented Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olljanat commented Jan 12, 2019

Uh oh!

rgulewich commented Jan 14, 2019

Uh oh!

rgulewich commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Feb 19, 2019

Uh oh!

rgulewich commented Feb 20, 2019

Uh oh!

yongtang commented May 12, 2019

Uh oh!

AkihiroSuda commented Jul 9, 2019

Uh oh!

thaJeztah commented Jul 9, 2019

Uh oh!

tonistiigi commented Jul 18, 2019

Uh oh!

rgulewich commented Jul 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

rgulewich commented Dec 14, 2018 •

edited

Loading

codecov bot commented Dec 16, 2018 •

edited

Loading

olljanat commented Jan 11, 2019 •

edited

Loading

rgulewich commented Feb 14, 2019 •

edited

Loading