Add support for CgroupsPath #397

hqhq · 2015-11-12T02:50:14Z

It's defined in specs, add the support to runC.

Signed-off-by: Qiang Huang [email protected]

hqhq · 2015-11-12T03:06:34Z

We might have to discuss how CgroupsPath should get alone with Parent. They are doing similar thing, can they combine or we should just remove parent and use CgroupsPath because it's in specs?

LK4D4 · 2015-11-12T22:52:36Z

@hqhq Looks like we should remove Parent if it's not in spec.

hqhq · 2015-11-13T02:32:56Z

@LK4D4 Yes that what I had in mind, but should we do it in this PR? Since Docker is using Parent, I'm thinking about we have them both for now, after merging this to Docker, we change usage on Docker side to CgroupsPath, then remove Parent smoothly. WDYT?

ashahab-altiscale · 2015-11-13T06:31:19Z

Thanks for the PR. @hqhq I had a different idea for the cgroupsPath support: #399
Sorry I did not notice your PR before(mine can be combined with yours). However, the important thing is, if cgroups is passed, and it exists, runC must not modify it(other than joining the cgroups). This will allow me to remove cgroups related functionality from #357
Thanks

mrunalp · 2015-11-13T19:06:36Z

The intention behind cgroupsPath in the spec is that the container joins the path rather than creating a sub cgroup under it.

hqhq · 2015-11-16T11:45:47Z

However, the important thing is, if cgroups is passed, and it exists, runC must not modify it(other than joining the cgroups).

@ashahab-altiscale I think you understand the intention of cgroupsPath wrong, my understanding is the same as @mrunalp . Do you have some background that you need some features work like that? Maybe we can discuss it in specs.

ashahab-altiscale · 2015-11-16T13:26:08Z

@hqhq So you're saying that even when the operator passes the cgroup using cgroupsPath, it will be modified(in terms of memory, devices, etc) by runC?
If that is the intent of the spec cgroupsPath, then that would not serve the usecase I was thinking of.
My usecase is this: #350. I need to be able to launch containers(with runC) from inside userns containers. I have elaborated on the issues with this in #357. When I launch runC inside a userns container, runC tries to change devices.deny and devices.allow files, which only a host root can do. So there should be a nice way for the operator to pass in the cgroup and telling runC "I know what I'm doing, don't touch this cgroup, just join it". In my previous discussion with @mrunalp, he seemed to indicate this is what "cgroupsPath" does. Are you saying this would require an additional flag in the spec? There can be other cases where the operator wants to opt-out of runC's modifications of cgroup. What if there is an external cgroupsManager that the operator uses? (I think @wking mentioned that usecase).

LK4D4 · 2015-11-16T16:27:00Z

@mrunalp @hqhq @ashahab-altiscale I think cgoupPath won't be modified?

ashahab-altiscale · 2015-11-16T16:30:45Z

That's what I thought also, Alex.
On Nov 16, 2015 8:27 AM, "Alexander Morozov" [email protected]
wrote:

@mrunalp https://github.com/mrunalp @hqhq https://github.com/hqhq
@ashahab-altiscale https://github.com/ashahab-altiscale I think
cgoupPath won't be modified?

—
Reply to this email directly or view it on GitHub
#397 (comment).

wking · 2015-11-16T17:37:53Z

On Mon, Nov 16, 2015 at 05:26:10AM -0800, Abin Shahab wrote:

@hqhq So you're saying that even when the operator passes the
cgroup, it will be modified(in terms of memory, devices, etc) by
runC? If that is the intent of the spec cgroupsPath, then that
would not serve the usecase I was thinking of.

Re-reading the spec 1, whether cgroupsPath can be modified or just
joined seems to be open to interpretation. I suggest bumping this
over to the spec to get that sorted out before continuing with the
implementation here.

mrunalp · 2015-11-16T19:39:08Z

I see 4 cases; 3 of which were discussed in the PR that merged cgroupPath into the spec -

Specify resources: runtimes creates a path and updates values per resources.
Specify cgroupsPath: runtime joins the cgroupsPath.
Specify both: runtime joins the cgroupsPath and updates resources under that path (opencontainers/runtime-spec@429f936#diff-34c30be66233f08b447fb608ea0e66bbR30).
Specify none: Don't do anything with cgroups i.e. let the kernel give me something I run under automatically. This is useful for usernamespaced desktop applications such as gimp under xdg-app.

ashahab-altiscale · 2015-11-16T19:47:21Z

@mrunalp As discussed, case (2) will work for me if cgroupsPath + no resrouce specification also prevents runC from setting devices.allow and devices.deny. That's what fails when the host launching runC is itself a usernamespaced container( see #350 ). Either I can update my PR or @hqhq can update this. My userns PR(#357) is blocked on this.

wking · 2015-11-16T20:13:44Z

On Mon, Nov 16, 2015 at 11:39:11AM -0800, Mrunal Patel wrote:

I see 4 cases; 3 of which were discussed in the PR that merged
cgroupPath into the spec -

Ah, good (I hadn't gone back to re-read opencontainers/runtime-spec#137.

Specify both: runtime joins the cgroupsPath and updates resources
under that path
(opencontainers/runtime-spec@429f936#diff-34c30be66233f08b447fb608ea0e66bbR30).

Should I file a spec PR to echo this (informative?) Go comment in the
(normative?) Markdown description? Or are the Go comments also
normative? I feel like the Markdown was supposed to be stand-alone,
but I haven't looked up a reference to quote for that.

ashahab-altiscale · 2015-11-16T20:35:13Z

@wking That would be good. Lets not leave it to interpretation.

mrunalp · 2015-11-16T20:37:28Z

@wking Yeah, go for it.

This should help clarify the cgroupsPath setting added in opencontainers#137, which was the subject of some confusion in opencontainers/runc#397. Issues I'm trying to clarify here: * If you specify a cgroupsPath, is the container added to that path or a sub-cgroup underneath it [1]? (This commit rules in favor of "added to that path") * If you specify a cgroupsPath, can the runtime modify that cgroup [2]? (This commit rules "yes, if 'resources' is specified", following [3] and the Go comment from opencontainers#137 [4]). To help make the distinctions clearer, I've added a facet list to help folks think about the difference between cgroup creation, process assignment, and resource configuration. cgroupsPath is just about cgroup creation and process assignment. 'resources' is just about resource configuration. I've listed out Mrunal's first three cases [3] to be even clearer. I stayed away from the "neither are set" case, since I covered that fairly directly in opencontainers#237, which that was punted back to the list [5] and has seen no further interest. So I'm not clear on what the intended semantics are there, although Mrunal's wording in [4] seems to agree with the proposal in opencontainers#237. [1]: opencontainers/runc#397 (comment) [2]: opencontainers/runc#397 (comment) [3]: opencontainers/runc#397 (comment) [4]: opencontainers@429f936#diff-34c30be66233f08b447fb608ea0e66bbR30 [5]: https://groups.google.com/a/opencontainers.org/d/msg/dev/qWHoKs8Fsrk/c9mv6qXtDAAJ Message-ID: <[email protected]> Signed-off-by: W. Trevor King <[email protected]>

It's defined in specs, add the support to runC. Signed-off-by: Qiang Huang <[email protected]>

hqhq · 2015-11-17T12:45:32Z

I understood the intention of cgroupsPath wrong, if it's specified, it should replace the cgroup path rule in runc (combined by parent and name), now updated and this should work as expected in specs.

@ashahab-altiscale I didn't handle devices cgroup in this PR since I think that's a separate issue.

mrunalp · 2015-12-14T22:46:42Z

libcontainer/configs/cgroup_unix.go

If we want to avoid adding an additional field, we could overload Name and treat it as cgroupsPath if it is absolute. That way we don't need to have Parent, Name as well as CgroupsPath. WDYT?

Yes, that's what I planed, when we merge this, we can start removing Parent and Name.

hmm i think we can just start removing them here ? I think it's also clearer when we integrate this with docker too since there will be complication errors :p

This should help clarify the cgroupsPath setting added in opencontainers#137, which was the subject of some confusion in opencontainers/runc#397. Issues I'm trying to clarify here: * If you specify a cgroupsPath, is the container added to that path or a sub-cgroup underneath it [1]? (This commit rules in favor of "added to that path") * If you specify a cgroupsPath, can the runtime modify that cgroup [2]? (This commit rules "yes, if 'resources' is specified", following [3] and the Go comment from opencontainers#137 [4]). To help make the distinctions clearer, I've added a facet list to help folks think about the difference between cgroup creation, process assignment, and resource configuration. cgroupsPath is just about cgroup creation and process assignment. 'resources' is just about resource configuration. I've listed out Mrunal's first three cases [3] to be even clearer. I stayed away from the "neither are set" case, since I covered that fairly directly in opencontainers#237, which that was punted back to the list [5] and has seen no further interest. So I'm not clear on what the intended semantics are there, although Mrunal's wording in [4] seems to agree with the proposal in opencontainers#237. [1]: opencontainers/runc#397 (comment) [2]: opencontainers/runc#397 (comment) [3]: opencontainers/runc#397 (comment) [4]: opencontainers@429f936#diff-34c30be66233f08b447fb608ea0e66bbR30 [5]: https://groups.google.com/a/opencontainers.org/d/msg/dev/qWHoKs8Fsrk/c9mv6qXtDAAJ Message-ID: <[email protected]> Signed-off-by: W. Trevor King <[email protected]>

cyphar · 2016-01-26T21:38:49Z

IMHO, we should error out if the systemd manager has CgroupPath set (because systemd doesn't allow you to set arbitrary "slice" paths).

hqhq · 2016-01-27T03:54:09Z

Yeah I start thinking maybe we still need to separate --cgroup-parent and --slice usage.

*: add support for cgroup namespace

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

hqhq force-pushed the hq_add_cgrouppaht branch from b1e24bf to e50c039 Compare November 12, 2015 03:03

wking mentioned this pull request Nov 16, 2015

runtime-config-linux: Clarify resources vs. cgroupsPath opencontainers/runtime-spec#247

Closed

ashahab-altiscale mentioned this pull request Nov 17, 2015

CgroupsPath in runC #399

Closed

Add support for CgroupsPath

70611b6

It's defined in specs, add the support to runC. Signed-off-by: Qiang Huang <[email protected]>

hqhq force-pushed the hq_add_cgrouppaht branch from e50c039 to 70611b6 Compare November 17, 2015 12:37

mrunalp reviewed Dec 14, 2015
View reviewed changes

hqhq mentioned this pull request Dec 22, 2015

Update cgroupsPath to cgroupName and clarify the semantics around that. opencontainers/runtime-spec#270

Closed

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

hqhq deleted the hq_add_cgrouppaht branch February 14, 2016 01:34

stefanberger pushed a commit to stefanberger/runc that referenced this pull request Sep 8, 2017

Merge pull request opencontainers#397 from cyphar/add-cgroup-namespace

831d961

*: add support for cgroup namespace

Add support for CgroupsPath #397

Add support for CgroupsPath #397

Uh oh!

Conversation

hqhq commented Nov 12, 2015

Uh oh!

hqhq commented Nov 12, 2015

Uh oh!

LK4D4 commented Nov 12, 2015

Uh oh!

hqhq commented Nov 13, 2015

Uh oh!

ashahab-altiscale commented Nov 13, 2015

Uh oh!

mrunalp commented Nov 13, 2015

Uh oh!

hqhq commented Nov 16, 2015

Uh oh!

ashahab-altiscale commented Nov 16, 2015

Uh oh!

LK4D4 commented Nov 16, 2015

Uh oh!

ashahab-altiscale commented Nov 16, 2015

Uh oh!

wking commented Nov 16, 2015

Uh oh!

mrunalp commented Nov 16, 2015

Uh oh!

ashahab-altiscale commented Nov 16, 2015

Uh oh!

wking commented Nov 16, 2015

Uh oh!

ashahab-altiscale commented Nov 16, 2015

Uh oh!

mrunalp commented Nov 16, 2015

Uh oh!

hqhq commented Nov 17, 2015

Uh oh!

mrunalp Dec 14, 2015

Choose a reason for hiding this comment

Uh oh!

hqhq Dec 15, 2015

Choose a reason for hiding this comment

Uh oh!

dqminh Dec 15, 2015

Choose a reason for hiding this comment

Uh oh!

cyphar commented Jan 26, 2016

Uh oh!

hqhq commented Jan 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants