don't fail when subsystem not mounted #476

hqhq · 2015-03-24T08:10:16Z

We do this aim two goals:

don't fail when some subsystems are not mounted.
fail hard instead of ignoring the error when a user specifies
an option and we are unable to fulfill the request.

Signed-off-by: Qiang Huang h.huangqiang@huawei.com

crosbymichael · 2015-03-24T17:29:25Z

cgroups/fs/devices.go

@@ -10,7 +10,7 @@ type DevicesGroup struct {

 func (s *DevicesGroup) Apply(d *data) error {
 	dir, err := d.join("devices")
-	if err != nil {
+	if err != nil && !cgroups.IsNotFound(err) {


We really cannot do this because devices is a hard requirement. Without the devices cgroup it is a security issue because a container will have unrestricted access to device nodes.

crosbymichael · 2015-03-24T18:33:26Z

@hqhq what cgroups does your systems not have mounted?

hqhq · 2015-03-25T13:11:23Z

@crosbymichael some of our clients don't mount all cgroup subsystems, maybe just mount cpuset and memory cgroup. It's OK if Docker take device cgroup as hard requirement, but I think that can be done on Docker side, because that's what Docker needed, and we can show specific message, libcontainer should be more common, what do you think?

crosbymichael · 2015-03-25T17:10:18Z

@hqhq I think cgroups are a fundamental aspect for creating containers and support should be required. There is little overhead for mounting these and joining them as monitoring via cgroups is very important to understand the performance of your containerized apps. I feel like containers are the sum of namespaces, cgroups, caps, and rootfs jails (chroot). Each are equally important and should be required.

@vmarmol how do you feel about making these mounts required or not?

vmarmol · 2015-03-25T21:00:27Z

I see @hqhq's point about libcontainer maybe not being the place where we enforce this, but I do see the value in having a setup we know and test frequently. We already assume cgroups are mounted in various places and it'll be brittle for those that don't. I'm conflicted but I side with @crosbymichael about having a good experience for the 99% usecases.

hqhq · 2015-03-28T09:43:41Z

@crosbymichael @vmarmol I don't think we should take all cgroup subsystems as hard requirement, devices cgroup has a good reason but not all of others. I added some check on Docker side moby/moby#11882, and we can take devices cgroup as hard requirement in libcontaier.

hqhq · 2015-03-28T10:05:02Z

I updated devices part, WDYT?

crosbymichael · 2015-03-30T19:02:25Z

@hqhq ok, i think that maybe a good approach to take.

vmarmol · 2015-03-30T23:20:08Z

cgroups/systemd/apply_systemd.go

+	// Normally dir should not be empty, one case is that cgroup subsystem
+	// is not mounted, we will get empty dir, and we want it fail here.
+	if dir == "" {
+		return fmt.Errorf("No such directory for %s.", file)


nit: lowercase "No" for Go errors to start with lowercase.

vmarmol · 2015-03-30T23:20:35Z

Sounds reasonable to me.

hqhq · 2015-03-31T02:54:14Z

Updated.

mrunalp · 2015-03-31T22:51:21Z

@hqhq Do we need any changes on the systemd side?

hqhq · 2015-04-01T06:32:30Z

@mrunalp Yes we need indeed, updated, thanks.

hqhq · 2015-04-15T07:54:14Z

Rebased, ping.

hqhq · 2015-04-22T02:02:27Z

ping.

dqminh · 2015-04-22T02:13:38Z

cgroups/systemd/apply_systemd.go

+	if path == "" {
+		return nil
+	}
+


should this check be moved into cpusetGroup.ApplyDir ?

Yes, thanks, updated.

vishh · 2015-04-22T02:14:25Z

This still doesn't satisfy the use case where no cgroups are mounted right?

We do this aim two goals: - don't fail when some subsystems are not mounted (devices cgroup is an exception because it will cause secirity issues). - fail hard instead of ignoring the error when a user specifies an option and we are unable to fulfill the request. Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>

hqhq · 2015-04-22T03:00:03Z

@vishh No, for now devices cgroup is hard requirement for security.

dqminh · 2015-04-22T03:33:34Z

LGTM

vmarmol · 2015-04-22T16:42:17Z

cgroups/fs/cpuset.go

@@ -48,6 +48,11 @@ func (s *CpusetGroup) GetStats(path string, stats *cgroups.Stats) error {
 }

 func (s *CpusetGroup) ApplyDir(dir string, cgroup *configs.Cgroup, pid int) error {


Shouldn't we not even make the call to apply for this subsystem if it doesn't exist?

I was outside, just moved in as @dqminh suggested.
If outside, we should do that on both fs and systemd side, but I don't think that's a big deal. Just want make sure which way you prefer.

Yeah, checking this outside would duplicate this logic, I like it here better too 👍

calavera · 2015-04-27T22:05:12Z

code LGTM.

hqhq · 2015-05-11T08:15:59Z

ping.

dqminh · 2015-05-11T10:39:43Z

LGTM

vmarmol · 2015-05-11T16:29:47Z

LGTM

don't fail when subsystem not mounted

hqhq mentioned this pull request Mar 24, 2015

Revert "cgroups: only return path when subsystem really mounted #474

Merged

crosbymichael reviewed Mar 24, 2015
View reviewed changes

hqhq force-pushed the hq_dont_fail_subsystem branch from 3e65a95 to 2d6ee23 Compare March 28, 2015 10:02

vmarmol reviewed Mar 30, 2015
View reviewed changes

hqhq force-pushed the hq_dont_fail_subsystem branch from 2d6ee23 to 1bcb6f3 Compare March 31, 2015 02:53

hqhq force-pushed the hq_dont_fail_subsystem branch from 1bcb6f3 to e10bc4b Compare April 1, 2015 06:31

hqhq force-pushed the hq_dont_fail_subsystem branch from e10bc4b to a824f09 Compare April 15, 2015 07:53

GordonTheTurtle added dco/yes and removed dco/no labels Apr 15, 2015

dqminh reviewed Apr 22, 2015
View reviewed changes

hqhq force-pushed the hq_dont_fail_subsystem branch from a824f09 to 27d3dd3 Compare April 22, 2015 03:10

vmarmol reviewed Apr 22, 2015
View reviewed changes

calavera mentioned this pull request Apr 27, 2015

run fails with open /cpuset/docker/[newcontiainerid]/cpuset.cpus: no such file or directory moby/moby#9654

Closed

vmarmol added a commit that referenced this pull request May 11, 2015

Merge pull request #476 from hqhq/hq_dont_fail_subsystem

a37b2a4

don't fail when subsystem not mounted

vmarmol merged commit a37b2a4 into docker-archive:master May 11, 2015

hqhq deleted the hq_dont_fail_subsystem branch May 12, 2015 01:06

hqhq mentioned this pull request Jun 29, 2015

cgroups: fs: fix issues with .Apply() #652

Closed

hqhq mentioned this pull request Oct 20, 2015

Fixing container start when cgroup subsystem not found opencontainers/runc#341

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't fail when subsystem not mounted #476

don't fail when subsystem not mounted #476

hqhq commented Mar 24, 2015

crosbymichael Mar 24, 2015

crosbymichael commented Mar 24, 2015

hqhq commented Mar 25, 2015

crosbymichael commented Mar 25, 2015

vmarmol commented Mar 25, 2015

hqhq commented Mar 28, 2015

hqhq commented Mar 28, 2015

crosbymichael commented Mar 30, 2015

vmarmol Mar 30, 2015

vmarmol commented Mar 30, 2015

hqhq commented Mar 31, 2015

mrunalp commented Mar 31, 2015

hqhq commented Apr 1, 2015

hqhq commented Apr 15, 2015

hqhq commented Apr 22, 2015

dqminh Apr 22, 2015

hqhq Apr 22, 2015

vishh commented Apr 22, 2015

hqhq commented Apr 22, 2015

dqminh commented Apr 22, 2015

vmarmol Apr 22, 2015

hqhq Apr 23, 2015

calavera Apr 27, 2015

calavera commented Apr 27, 2015

hqhq commented May 11, 2015

dqminh commented May 11, 2015

vmarmol commented May 11, 2015

		@@ -48,6 +48,11 @@ func (s CpusetGroup) GetStats(path string, stats cgroups.Stats) error {
		}

		func (s CpusetGroup) ApplyDir(dir string, cgroup configs.Cgroup, pid int) error {

don't fail when subsystem not mounted #476

don't fail when subsystem not mounted #476

Conversation

hqhq commented Mar 24, 2015

crosbymichael Mar 24, 2015

Choose a reason for hiding this comment

crosbymichael commented Mar 24, 2015

hqhq commented Mar 25, 2015

crosbymichael commented Mar 25, 2015

vmarmol commented Mar 25, 2015

hqhq commented Mar 28, 2015

hqhq commented Mar 28, 2015

crosbymichael commented Mar 30, 2015

vmarmol Mar 30, 2015

Choose a reason for hiding this comment

vmarmol commented Mar 30, 2015

hqhq commented Mar 31, 2015

mrunalp commented Mar 31, 2015

hqhq commented Apr 1, 2015

hqhq commented Apr 15, 2015

hqhq commented Apr 22, 2015

dqminh Apr 22, 2015

Choose a reason for hiding this comment

hqhq Apr 22, 2015

Choose a reason for hiding this comment

vishh commented Apr 22, 2015

hqhq commented Apr 22, 2015

dqminh commented Apr 22, 2015

vmarmol Apr 22, 2015

Choose a reason for hiding this comment

hqhq Apr 23, 2015

Choose a reason for hiding this comment

calavera Apr 27, 2015

Choose a reason for hiding this comment

calavera commented Apr 27, 2015

hqhq commented May 11, 2015

dqminh commented May 11, 2015

vmarmol commented May 11, 2015