Add support for cgroup namespace #1916

crosbymichael · 2018-10-22T13:57:59Z

Carrying #781. Thanks for @cyphar 's work. 😄

Cgroup namespace can be configured in config.json as other
namespaces. Here is an example:

"namespaces": [
	{
		"type": "pid"
	},
	{
		"type": "network"
	},
	{
		"type": "ipc"
	},
	{
		"type": "uts"
	},
	{
		"type": "mount"
	},
	{
		"type": "cgroup"
	}
],

Note that if you want to run a container which has shared cgroup ns with
another container, then it's strongly recommended that you set
proper CgroupsPath of both containers(the second container's cgroup
path must be the subdirectory of the first one). Or there might be
some unexpected results.

Signed-off-by: Yuanhong Peng pengyuanhong@huawei.com

crosbymichael · 2018-10-22T13:59:14Z

For #1884, We worked on a repo and I have verified that this fixes the issue. Without the patch, you can repo in about 2-5min, with the patch, it runs for hours without error. I'm confident that this fixes the issue.

mikebrow

some nits...

mikebrow · 2018-10-22T18:03:40Z

libcontainer/nsenter/nsexec.c

+
+						/* Send the init_func pid back to our parent. */
+						/*
+						 *                       * Send the init_func pid and the pid of the first child back to our parent.


comment block mishap..

mikebrow · 2018-10-22T18:04:37Z

libcontainer/nsenter/nsexec.c

@@ -201,7 +207,8 @@ static void update_setgroups(int pid, enum policy_t setgroup)
 		 * open(2) or write(2) will return ENOENT. This is fine.
 		 */
 		if (errno != ENOENT)
-			bail("failed to write '%s' to /proc/%d/setgroups", policy, pid);
+			bail("failed to write '%s' to /proc/%d/setgroups",
+			     policy, pid);


harder to see what's new when the diff includes lots of odd extra spaces and new lines throughout

libcontainer/nsenter/nsexec.c

mikebrow · 2018-10-22T18:35:06Z

libcontainer/nsenter/nsexec.c

-			 * with clone() because there were some old kernel versions where
-			 * clone(CLONE_PARENT | CLONE_NEWPID) was broken, so we'll just do
-			 * it the long way.
+			   if (config.cloneflags & CLONE_NEWCGROUP)


comment mishap

This is a very simple implementation because it doesn't require any configuration unlike the other namespaces, and in its current state it only masks paths. This feature is available in Linux 4.6+ and is enabled by default for kernels compiled with CONFIG_CGROUP=y. Signed-off-by: Aleksa Sarai <asarai@suse.de> Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

cyphar

Also there's a lot of whitespace and wrapping changes -- were these added by accident?

libcontainer/nsenter/nsexec.c

libcontainer/process_linux.go

crosbymichael · 2018-10-25T14:07:27Z

So as far as the whitespace, I thought we all agreed on using indent -linux for our C files so that they stay consistent, same as gofmt.

crosbymichael · 2018-10-25T14:18:00Z

Resolved the comments, let me know if you want me to revert the formatting changes.

AkihiroSuda · 2018-10-25T14:32:05Z

Max line length seems changed in recent indent?

crosbymichael · 2018-10-25T16:53:07Z

@AkihiroSuda I think it has always been 80chars

crosbymichael · 2018-10-29T19:35:58Z

Whats the next steps here? I have verified the issues that this fixes and it would be nice to get this in. Let me know what changes you want or not

dqminh · 2018-10-29T22:48:55Z

libcontainer/process_linux.go

+			// TODO: should not be the responsibility to call here
+			p.manager.Destroy()
+		}
+	}()


this block is odd because we do most of the same thing earlier. Shouldnt we only need to apply the cgroup namespace and wait for child here ?

do you mean, just this defer?

No, basically most of the block except the new part about setting up cgroup namespace and wait for first child to exit. Here is the current block we have just a few lines above: https://github.com/opencontainers/runc/blob/master/libcontainer/process_linux.go#L276-L295

Now this is interesting. We certainly put the parent into cgroup before we spawning the child accoring to our current code, so i wonder where the leak can be. Also #1884 is about runc exec, shouldnt we apply the same thing in setnsProcess too ? I think that is the root cause of #1884, isnt it ?

I think that the old Apply code should be removed since there's not much point in applying it twice -- maybe everything including the intelRdt cleanup as well as the comments should be just wholesale moved here... Not sure.

We certainly put the parent into cgroup before we spawning the child accoring to our current code, so i wonder where the leak can be.

I'm not sure that's correct -- we have err := p.cmd.Start() before the cgroup application code. Though, I find it hard to believe that runc init can do all of its setup and reach the Go code before Go runs through 5 lines of code (especially since Linux's scheduler is biased to the currently-running process -- so theoretically the parent process should get more work done in the short race after fork+exec).

having the Apply after we have PIDs in the cgroup breaks memory.kmem.limit_in_bytes which can be set only when the cgroup is empty.

Is it safe to remove the second Apply?

I still don't understand why there are two identical defer blocks in the same start() method.

@kolyshkin I saw #2238 is suggesting to remove one

cyphar · 2018-10-31T03:01:09Z

It's always been 80chars, but I think when I ran the formatting it was with 120 characters (which is becoming a more common "sane" width). My only complaint with the formatting is that it makes stuff harder to read (function arguments are wrapped in odd ways and so on -- something that you don't see in actual Linux code). I would exclude them and we can re-establish a format in a separate PR.

crosbymichael · 2018-10-31T14:28:50Z

Sounds good to me, i'll make the changes.

Cgroup namespace can be configured in `config.json` as other namespaces. Here is an example: ``` "namespaces": [ { "type": "pid" }, { "type": "network" }, { "type": "ipc" }, { "type": "uts" }, { "type": "mount" }, { "type": "cgroup" } ], ``` Note that if you want to run a container which has shared cgroup ns with another container, then it's strongly recommended that you set proper `CgroupsPath` of both containers(the second container's cgroup path must be the subdirectory of the first one). Or there might be some unexpected results. Signed-off-by: Yuanhong Peng <pengyuanhong@huawei.com> Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

crosbymichael · 2018-10-31T14:52:19Z

Ok, pushed the formatting changes

cyphar · 2018-11-13T05:58:56Z

LGTM, and I hope we can get this in 1.0 but if we can't we can always do it in the next release. I've tested it out and it appears to work for most basic uses.

Last call for pre-1.0 merges!
/cc @opencontainers/runc-maintainers

crosbymichael · 2018-11-13T15:45:43Z

I think we should merge this for 1.0, its a new feature but the implementation also fixes a nasty bug under stress

cyphar · 2018-11-13T16:40:02Z

Yeah, I agree.

mrunalp · 2018-11-13T19:54:12Z

LGTM

mrunalp · 2018-11-13T19:56:10Z

will merge once pull approve returns.

giuseppe · 2019-01-04T16:45:21Z

It seems that this change introduced a problem with the systemd cgroupfs backend.

I've added this snippet to the default "runc spec" config file (under linux/resources):

      "memory": {
        "kernel": 41943040,
        "disableOOMKiller": false
      }

now when I try to run a container I get:

# runc --systemd-cgroup run foo
container_linux.go:344: starting container process caused "process_linux.go:311: applying cgroup configuration for process caused \"failed to set memory.kmem.limit_in_bytes, because either tasks have already joined this cgroup or it has children\""

Fix for opencontainers/runc#1884 is in opencontainers/runc#1916 - problem described in 1884 is independent of kubernetes version. - happens with kernel >= v4.8 - fixed in runc version v1.0.0-rc6 and above ( opencontainers/runc@9a3a8a5 ) - which got pulled into containerd v1.3.0-beta.0 and above ( containerd/containerd@97dd5df#diff-4061fcef378a6d912e14e2ce162a1995)

crosbymichael requested a review from cyphar October 22, 2018 17:01

mikebrow reviewed Oct 22, 2018

View reviewed changes

crosbymichael force-pushed the cgns branch 2 times, most recently from 80dc628 to 637df62 Compare October 23, 2018 18:27

crosbymichael force-pushed the cgns branch from 637df62 to b1debc3 Compare October 23, 2018 20:23

cyphar reviewed Oct 24, 2018

View reviewed changes

libcontainer/nsenter/nsexec.c Show resolved Hide resolved

libcontainer/nsenter/nsexec.c Outdated Show resolved Hide resolved

libcontainer/process_linux.go Outdated Show resolved Hide resolved

crosbymichael force-pushed the cgns branch from b1debc3 to e3afb2c Compare October 25, 2018 14:16

crosbymichael force-pushed the cgns branch from e3afb2c to c96490d Compare October 25, 2018 14:29

dqminh reviewed Oct 29, 2018

View reviewed changes

crosbymichael force-pushed the cgns branch from c96490d to c5b0ad5 Compare October 31, 2018 14:50

crosbymichael force-pushed the cgns branch from c5b0ad5 to df3fa11 Compare October 31, 2018 14:51

cyphar added this to the 1.0.0 milestone Nov 13, 2018

cyphar mentioned this pull request Nov 13, 2018

Tight container limits may cause "read init-p: connection reset by peer" #1914

Open

mrunalp merged commit 4769cdf into opencontainers:master Nov 13, 2018

rgulewich mentioned this pull request Dec 7, 2018

cgroup namespace support moby/moby#38332

Closed

rgulewich mentioned this pull request Dec 14, 2018

Start containers in their own cgroup namespaces moby/moby#38377

Merged

BooleanCat mentioned this pull request Dec 17, 2018

Resilience in adding of exec tasks to cgroups #1950

Merged

devimc mentioned this pull request Mar 12, 2019

exec: failed to write to cgroup.procs kata-containers/agent#476

Closed

dims mentioned this pull request Oct 14, 2019

Bump Containerd version to 1.3.0 kubernetes-sigs/image-builder#77

Merged

roman-mazur mentioned this pull request Oct 31, 2019

Puppeteer configuration can break a container balena-os/balena-engine#190

Open

kolyshkin mentioned this pull request Mar 4, 2020

Use named error return for initProcess#start #2238

Merged

kolyshkin mentioned this pull request Jun 1, 2020

(*initProcess).start: rm second Apply #2446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for cgroup namespace #1916

Add support for cgroup namespace #1916

crosbymichael commented Oct 22, 2018

crosbymichael commented Oct 22, 2018

mikebrow left a comment

mikebrow Oct 22, 2018

mikebrow Oct 22, 2018

mikebrow Oct 22, 2018

cyphar left a comment

crosbymichael commented Oct 25, 2018

crosbymichael commented Oct 25, 2018

AkihiroSuda commented Oct 25, 2018

crosbymichael commented Oct 25, 2018

crosbymichael commented Oct 29, 2018

dqminh Oct 29, 2018

crosbymichael Oct 30, 2018

dqminh Oct 30, 2018 •

edited

Loading

cyphar Oct 31, 2018 •

edited

Loading

giuseppe Jan 9, 2019

kolyshkin Mar 3, 2020

thaJeztah Mar 3, 2020

cyphar commented Oct 31, 2018

crosbymichael commented Oct 31, 2018

crosbymichael commented Oct 31, 2018

cyphar commented Nov 13, 2018 •

edited by caniszczyk

Loading

crosbymichael commented Nov 13, 2018

cyphar commented Nov 13, 2018

mrunalp commented Nov 13, 2018 •

edited by caniszczyk

Loading

mrunalp commented Nov 13, 2018

giuseppe commented Jan 4, 2019

Add support for cgroup namespace #1916

Add support for cgroup namespace #1916

Conversation

crosbymichael commented Oct 22, 2018

crosbymichael commented Oct 22, 2018

mikebrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar left a comment

Choose a reason for hiding this comment

crosbymichael commented Oct 25, 2018

crosbymichael commented Oct 25, 2018

AkihiroSuda commented Oct 25, 2018

crosbymichael commented Oct 25, 2018

crosbymichael commented Oct 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dqminh Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

cyphar Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar commented Oct 31, 2018

crosbymichael commented Oct 31, 2018

crosbymichael commented Oct 31, 2018

cyphar commented Nov 13, 2018 • edited by caniszczyk Loading

crosbymichael commented Nov 13, 2018

cyphar commented Nov 13, 2018

mrunalp commented Nov 13, 2018 • edited by caniszczyk Loading

mrunalp commented Nov 13, 2018

giuseppe commented Jan 4, 2019

dqminh Oct 30, 2018 •

edited

Loading

cyphar Oct 31, 2018 •

edited

Loading

cyphar commented Nov 13, 2018 •

edited by caniszczyk

Loading

mrunalp commented Nov 13, 2018 •

edited by caniszczyk

Loading