CgroupsPath in runC #399

ashahab-altiscale · 2015-11-13T06:29:50Z

This sets up the cgroupsPath. If cgroupsPath is passed in, runC will
check if the cgroup exists. If it exists, runC will just join the
cgroup, and will not make any modifications to the cgroups unless explicitly asked for.
If it does not exist, runC will create the cgroup and join it.
Signed-off-by: Abin Shahab [email protected]

hqhq · 2015-11-16T11:37:44Z

spec.go

Sorry I don't understand your PR, why set cgroupsPath to name? cgroupsPath should be a relative or absolute path, not just a cgroup name.

@hqhq Cgroups path is expected to be a path relative to the cgroups mount: https://github.com/opencontainers/specs/blob/master/runtime_config_linux.go#L29

@ashahab-altiscale I think the problem here is name should not identify a cgroup, because it should just be the name, not the full path.
In the code, cgroup path can be affected by parent besides name, though it won't happen in real life for now because we only support parent for Docker and Docker don't use cgroupsPath. But that'll be tricky if we want to expand these features in the future.

We should remove parent as you and @LK4D4 discussed. CgroupsPath must not be a full path, but relative to cgroup mount on host. That's what I interpret from spec.

Also, since cgroupsPath does not support split hierarchy, would parent be ever different than what this line retrieves: https://github.com/opencontainers/runc/blob/master/spec.go#L436

Sorry for the confusing, here all cgroup path I mentioned are relative to cgroup mount point, and full path I meat absolute path including cgroup name, like /web/apache/apache1, here I think what name should be is apache1, not the entire /web/apache/apache1.

I understand now, thanks for explanation. However, this name can also be a
path, since it gets appended to the cgroup mount.
On Nov 18, 2015 12:16 AM, "Qiang Huang" [email protected] wrote:

In spec.go
#399 (comment):

@@ -438,6 +438,9 @@ func createCgroupConfig(name string, spec _specs.LinuxRuntimeSpec, devices []_co
if err != nil {
return nil, err
}

if spec.Linux.CgroupsPath != "" {

name = spec.Linux.CgroupsPath

}

Sorry for the confusing, here all cgroup path I mentioned are relative to
cgroup mount point, and full path I meat absolute path including cgroup
name, like /web/apache/apache1, here I think what name should be is
apache1, not the entire /web/apache/apache1.

—
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/runc/pull/399/files#r45170849.

@hqhq I just tested this with cgroupsPath with test1/test2/test3 as a cgroupsPath, and the behavior is as expected. I have test1/test2/test3 cgroup created before starting runc.
When I do runc start it appropriately joins and modifies test3 cgroup.
Do you have other comments on my approach?

@mrunalp @crosbymichael Any feedback on this? This will allow me to launch containers inside userns containers.

This sets up the cgroupsPath. If cgroupsPath is passed in, runC will check if the cgroup exists. If it exists, runC will just join the cgroup, and will not make any modifications to the cgroups. If it does not exist, runC will create the cgroup and join it. Signed-off-by: Abin Shahab <[email protected]>

ashahab-altiscale · 2015-11-17T05:48:41Z

@hqhq @mrunalp @wking I have changed the PR to comply with the case (2) discussed here: #397

LK4D4 · 2015-11-17T18:36:13Z

Looks like everything about cgroups should be relative.

mrunalp · 2015-11-17T18:45:36Z

Yep, relative to the cgroup mount point.

ashahab-altiscale · 2015-11-18T05:30:21Z

Thanks. @wking Can this be farther clarified in your doc PR(can't find the link to it)

wking · 2015-11-18T06:03:10Z

On Tue, Nov 17, 2015 at 09:30:23PM -0800, Abin Shahab wrote:

@wking Can this be farther clarified in your doc PR(can't find the
link to it)

The PR I spun off from #397 is opencontainers/runtime-spec#247, but 1
already has:

cgroupsPath is expected to be relative to the cgroups mount
point. If not specified, cgroups will be created under '/'.

which I think covers both cases pretty clearly (as far as relative
paths are concerned). What more would you like?

ashahab-altiscale · 2015-11-18T08:01:03Z

@wking , you are right, this is clear that cgroupsPath must be relative. Therefore there is no point in it being a full path.

mrunalp · 2015-12-08T20:54:21Z

libcontainer/configs/cgroup_unix.go

We shouldn't have a bool for this. If there is a path passed, then we use it. Resource may or may not be empty.

@mrunalp I removed it.

mrunalp · 2015-12-10T18:26:43Z

libcontainer/cgroups/fs/apply_raw.go

We shouldn't require this right? If there is a path passed in then we use that path.

Signed-off-by: Abin Shahab <[email protected]>

ashahab-altiscale · 2015-12-10T23:21:44Z

@mrunalp Made the changes you requested. Devices cgroup uses os.stat to check existence of cgroupsPath, and decides accordingly.

mrunalp · 2015-12-14T22:24:32Z

spec.go

We use a combination of parent and the name to derive the path. So, you need to handle the parent as well and make sure it is correct.

I think it was like this before. We used path as Name in docker for cgroup-parent.

Yup, @LK4D4 @mrunalp and I have discussed this, and the current behavior in runc(if operator provides parent, it will append parent to Name) should be sufficient.

LK4D4 · 2015-12-15T17:48:36Z

Yes, Name field is sorta confusing. But it was like this even before.

GordonTheTurtle added the status/0-triage label Nov 13, 2015

ashahab-altiscale mentioned this pull request Nov 13, 2015

Add support for CgroupsPath #397

Closed

ashahab-altiscale force-pushed the 396-cgroupsPath branch 4 times, most recently from 26e5f06 to 33344e9 Compare November 15, 2015 22:41

hqhq reviewed Nov 16, 2015
View reviewed changes

ashahab-altiscale force-pushed the 396-cgroupsPath branch from 33344e9 to 2363d45 Compare November 17, 2015 05:34

mrunalp reviewed Dec 8, 2015
View reviewed changes

GordonTheTurtle added the dco/no label Dec 10, 2015

ashahab-altiscale force-pushed the 396-cgroupsPath branch from e2cf52e to b70705c Compare December 10, 2015 01:22

GordonTheTurtle removed the dco/no label Dec 10, 2015

mrunalp reviewed Dec 10, 2015
View reviewed changes

ashahab-altiscale force-pushed the 396-cgroupsPath branch 4 times, most recently from bf059ec to 1b6360e Compare December 10, 2015 23:17

Removed ExternalCgroups from cgroups struct

f115082

Signed-off-by: Abin Shahab <[email protected]>

ashahab-altiscale force-pushed the 396-cgroupsPath branch from 1b6360e to f115082 Compare December 10, 2015 23:20

mrunalp reviewed Dec 14, 2015
View reviewed changes

mlaventure mentioned this pull request Jan 21, 2016

Replace Cgroup Parent and Name fields by CgroupsPath #497

Merged

crosbymichael modified the milestones: 0.0.8, 0.0.9 Feb 4, 2016

LK4D4 closed this in #497 Feb 10, 2016

CgroupsPath in runC #399

CgroupsPath in runC #399

Uh oh!

Conversation

ashahab-altiscale commented Nov 13, 2015

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!

Choose a reason for hiding this comment

Uh oh!

ashahab-altiscale commented Nov 17, 2015

Uh oh!

LK4D4 commented Nov 17, 2015

Uh oh!

mrunalp commented Nov 17, 2015

Uh oh!

ashahab-altiscale commented Nov 18, 2015

Uh oh!

wking commented Nov 18, 2015

Uh oh!

ashahab-altiscale commented Nov 18, 2015

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!

ashahab-altiscale commented Dec 10, 2015

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!

LK4D4 commented Dec 15, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants