config-linux: clarify cgroup requirements #493

cyphar · 2016-06-08T13:03:17Z

This helps clear up some of the vagueness around cgroupsPath in the spec. It's
loosely modeled on what runC does (as well as allowing for custom cgroup
managers in runtimes). I also updated the cgroup section to make it clear that
lazy cgroup setup (where you only attach to the bare minimum of requested
cgroups) is allowed under the spec. This all originated from an IRC discussion
on #opencontainers.

This possibly obsoletes #270 (although that has some renaming going on).

Signed-off-by: Aleksa Sarai [email protected]

/cc @opencontainers/runtime-spec-maintainers @wking

vishh · 2016-06-08T17:43:12Z

config-linux.md

-If `cgroupsPath` is not specified, implementations can define the default cgroup path.
+If `cgroupsPath` is
+* ... an absolute path (starting with `/`), the implementation MUST take the path to be relative to the cgroup mount point.
+* ... a relative path (not starting with `/`), implementations SHOULD take the value to be the suffix of the resulting cgroup path.


nit: What does resulting cgroup path mean in this context? Did you mean relative to the cgroups of the runtime process?

I'm trying to make it not-too restrictive. There's no reason that a runtime has to make it relative to the cgroups of the runtime process. "Should be the suffix" means that it should be the last component in the path (but component is the wrong word since it can container /).

On Wed, Jun 08, 2016 at 10:07:28PM -0700, Aleksa Sarai wrote:

+* ... a relative path (not starting with /), implementations SHOULD take the value to be the suffix of the resulting cgroup path.

I'm trying to make it not-too restrictive. There's no reason that a
runtime has to make it relative to the cgroups of the runtime
process. "Should be the suffix" means that it should be the last
component in the path (but component is the wrong word since it can
container /).

How about:

… MAY interpret the path relative to a runtime-determined location
in the cgroup hierarchy.

I prefer SHOULD, since I believe a runtime should have a good reason for it to not do that way. But yes, I like that wording better.

On Thu, Jun 09, 2016 at 05:42:57PM -0700, Aleksa Sarai wrote:

+* ... a relative path (not starting with /), implementations SHOULD take the value to be the suffix of the resulting cgroup path.

I prefer SHOULD, since I believe a runtime should have a good reason
for it to not do that way…

Is which leaf of a random runtime-generated directory you're at all
that important to you? I don't see a lot of space between MAY and
SHOULD here.

Another thing worth clarifying about these relative paths is whether they always create a new leaf in the hierarchy. Given the uncertainty of the MAY/SHOULD wording for interpreting relative paths, I'd say that they MUST create a new leaf in the hierarchy (so folks who want to join an existing node MUST use an absolute cgroupsPath).

But then you have the systemd cgroup manager, where you specify a slice (which may not be a new leaf). While this might all change in cgroupv2, currently I'm trying to allow for custom cgroup managers to be compliant as long as they allow "absolute paths mode".

On Thu, Jun 09, 2016 at 10:23:11PM -0700, Aleksa Sarai wrote:

+* ... a relative path (not starting with /), implementations SHOULD take the value to be the suffix of the resulting cgroup path.

But then you have the systemd cgroup manager, where you specify a
slice (which may not be a new leaf).

Then maybe require a runtime servicing multiple ‘create’ calls with
identical cgroupsPaths to always join the same cgroups (regardless of
the absolute/relative/unset-ness)? That way folks using relative
paths for systemd (or whatever) have some idea of whether they'll be
joining an existing node or creating a new leaf in the hierarchy.

I've tried to resolve this in the latest version (saying that it must be consistent if a value of cgroupsPath is specified). I don't like the wording though, maybe you could help me reword it?

wking · 2016-06-10T18:18:10Z

config-linux.md

 Implementations of the Spec can choose to name cgroups in any manner.
 The Spec does not include naming schema for cgroups.
-The Spec does not support [split hierarchy][cgroup-v2].
+The Spec does not support the [unified or "default" hierarchy][cgroup-v2].


The old wording is “we don't support per-controller paths” for the reasons discussed in cgroup-v2.txt. It's not about limiting the cgroup version. Although you're now the second person I've heard interpret the old wording that way, so we probably want to adjust the wording. I think we do want to support v2 cgroups though, and it may be more clear than I thought if the runtime joins both v1 and v2 cgroups as needed to collect the controllers it wants to join (because a given controller can only be exposed in the v1 or v2 hierarchy, and not in both at once). So if you want to join the devices controller and that controller's in the v2 hierarchy, you join the appropriate v2 cgroup.

is it important to have this? Even if it was supported there would be no changes to the spec and this is more of a runtime concern right?

On Fri, Jun 10, 2016 at 11:45:55AM -0700, Michael Crosby wrote:

-The Spec does not support [split hierarchy][cgroup-v2].
+The Spec does not support the [unified or "default" hierarchy][cgroup-v2].

is it important to have this? Even if it was supported there would
be no changes to the spec and this is more of a runtime concern
right?

It was added to motivate a single cgroup-path property
vs. per-controller paths 1. I think it's useful to keep the
(informative) motivation documented here (although I think we can
clarify the wording).

But the thing is that we do support split hierarchies (that's the default on any modern GNU/Linux system). The reason I made this change was because the link text was wrong (cgroupv2 is the exact opposite of a split hierarchy).

I understand the point (cgroupsPath is a single value), but there's something wrong about this sentence when I read it.

On Fri, Jun 10, 2016 at 09:06:48PM -0700, Aleksa Sarai wrote:

I understand the point (cgroupsPath is a single value), but there's
something wrong about this sentence when I read it.

The wording could be improved. My suggestion is 1:

This specification does not support per-controller paths for the
reasons discussed in the [v2 cgroup docs][cgroup-v2].

Mashimiao · 2016-07-07T06:40:52Z

@cyphar any updates?

cyphar · 2016-07-07T07:43:56Z

@Mashimiao Thanks for pinging me, I completely forgot about this PR. 😉

@wking There, I've fixed up all of the things you mentioned. PTAL.

wking · 2016-07-07T14:00:12Z

config-linux.md

+For example, to run a new process in an existing container without updating limits, `resources` need not be specified.
+
+A runtime MUST at least use the minimum set of cgroup controllers required to fulfil the `resources` settings.
+However, a runtime CAN decide to not attach to any additional cgroup controllers supported by the system.


I still think we want “fulfil” → “fulfill” (although it looks like that's UK vs. US spelling, so not a big deal) and “supported by the system“ → “”.

ahem UK spelling is clearly superior. 😉

cyphar · 2016-07-21T15:27:29Z

@wking There, I've fixed up all of your concerns I think.

Some of the wording was a bit clumsy (and incorrect, by conflating different concepts in control groups as "cgroups"). Signed-off-by: Aleksa Sarai <[email protected]>

wking · 2016-07-21T16:39:13Z

config-linux.md

+`cgroupsPath` can be used to either control the cgroup hierarchy for containers or to run a new process in an existing container.
+If `cgroupsPath` is:
+* ... an absolute path (starting with `/`), the runtime MUST take the path to be relative to the cgroup mount point.
+* ... a relative path (not starting with `/`), the runtime MUST create a new leaf in the cgroup hierarchy and MAY interpret the path relative to a runtime-determined location in the cgroup hierarchy.


“MUST create a new leaf” conflicts with the later “…MUST consistently attach to the same place …” (see also here). Now that we have the consistently-attach wording, I'm ok with weaker wording like this, and don't really care about MAY vs. SHOULD.

What about something like:

* ... a relative path (not starting with `/`), the runtime MUST create a new leaf in the cgroup unless the same lexical path is used by another existing container [and MAY interpret the path relative to a runtime-determined location in the cgroup hierarchy].

Where "existing" means that it has been at least created. I'm not sure what wording to use there. I can drop the "must create a new leaf" -- but you're the one who suggested it and I do agree with the sentiment somewhat. But maybe we should drop it -- I think consistency is far more important than whether it's a new leaf.

On Fri, Jul 22, 2016 at 02:15:25AM -0700, Aleksa Sarai wrote:

I can drop the "must create a new leaf" -- but you're the one who
suggested it and I do agree with the sentiment somewhat. But maybe
we should drop it -- I think consistency is far more important than
whether it's a new leaf.

I think we should drop the new-leaf wording. I'd suggested it before
we had the consistency wording.

Sure. Fixed.

wking · 2016-07-22T16:14:19Z

config-linux.md

+* ... an absolute path (starting with `/`), the runtime MUST take the path to be relative to the cgroup mount point.
+* ... a relative path (not starting with `/`), the runtime MAY interpret the path relative to a runtime-determined location in the cgroup hierarchy.
+* ... not specified, the runtime CAN define the default cgroup path.
+Runtimes CAN consider certain `cgroupsPath` values to be invalid, and MUST generate an error if this is the case.


“CAN” → “MAY”. RFC 2119 does not define CAN. And you probably want to drop the comma.

Clarify some of the confusion with cgroupsPath. Due to systemd, we cannot require that relative paths be treated in any specific way. In addition, add a line stating that not all values of cgroupsPath are required to be valid (and that runtimes must error out if they have an invalid cgroup path). However, any given value of cgroupsPath should provide consistent results. Signed-off-by: Aleksa Sarai <[email protected]>

Make explicit that runtimes only have to attach to the bare minimum number of cgroups in order to fulfil the users' requirements. However, runtimes are of course allowed to attach to more than the bare minimum. Signed-off-by: Aleksa Sarai <[email protected]>

The example section looks very sparse otherwise. Signed-off-by: Aleksa Sarai <[email protected]>

cyphar · 2016-07-22T18:27:44Z

@wking Addressed the rest of your comments.

wking · 2016-07-22T18:34:10Z

19c2a92 → 4ed839e looks good to me.

There's still a comma 1 and “supported by the system” 2 that I
think we don't need, but neither of those have semantic impact.

cyphar · 2016-07-22T18:37:09Z

/cc @opencontainers/runtime-spec-maintainers

crosbymichael · 2016-07-22T22:36:56Z

LGTM

hqhq · 2016-07-25T11:46:31Z

LGTM

vishh reviewed Jun 8, 2016
View reviewed changes

This was referenced Jun 9, 2016

Allow Updates #17

Closed

set default cgroup path to /runc opencontainers/runc#895

Closed

cyphar force-pushed the linux-refresh-cgroup branch from 4e7a196 to e1d173f Compare June 10, 2016 14:43

wking reviewed Jun 10, 2016
View reviewed changes

cyphar force-pushed the linux-refresh-cgroup branch from e1d173f to cb96d84 Compare July 7, 2016 07:43

wking reviewed Jul 7, 2016
View reviewed changes

cyphar force-pushed the linux-refresh-cgroup branch from cb96d84 to 219ceaf Compare July 21, 2016 15:27

config-linux: cleanup cgroup wording

9ffd724

Some of the wording was a bit clumsy (and incorrect, by conflating different concepts in control groups as "cgroups"). Signed-off-by: Aleksa Sarai <[email protected]>

wking reviewed Jul 21, 2016
View reviewed changes

cyphar force-pushed the linux-refresh-cgroup branch from 219ceaf to 19c2a92 Compare July 22, 2016 14:45

wking reviewed Jul 22, 2016
View reviewed changes

cyphar force-pushed the linux-refresh-cgroup branch from 19c2a92 to 4ed839e Compare July 22, 2016 16:41

cyphar added 3 commits July 23, 2016 02:46

config-linux: add example of cgroup resource limits

4ed839e

The example section looks very sparse otherwise. Signed-off-by: Aleksa Sarai <[email protected]>

hqhq merged commit 03a6f8c into opencontainers:master Jul 25, 2016

hqhq mentioned this pull request Jul 25, 2016

Update cgroupsPath to cgroupName and clarify the semantics around that. #270

Closed

cyphar deleted the linux-refresh-cgroup branch July 25, 2016 11:48

wking mentioned this pull request Aug 1, 2016

runtimetest: add validation of cgroups opencontainers/runtime-tools#93

Merged

wking mentioned this pull request May 18, 2017

Clarify cgroups path handling behavior #834

Closed

wking mentioned this pull request Dec 9, 2017

Add definitions for RDMA controller/cgroup of Linux kernel 4.11 #942

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config-linux: clarify cgroup requirements #493

config-linux: clarify cgroup requirements #493

cyphar commented Jun 8, 2016 •

edited

Loading

vishh Jun 8, 2016

cyphar Jun 9, 2016

wking Jun 9, 2016

cyphar Jun 10, 2016

wking Jun 10, 2016

wking Jun 10, 2016 via email

cyphar Jun 10, 2016

wking Jun 10, 2016

cyphar Jun 10, 2016

wking Jun 10, 2016

crosbymichael Jun 10, 2016

wking Jun 10, 2016

cyphar Jun 11, 2016

wking Jun 11, 2016

Mashimiao commented Jul 7, 2016

cyphar commented Jul 7, 2016

wking Jul 7, 2016

cyphar Jul 7, 2016

cyphar commented Jul 21, 2016

wking Jul 21, 2016

cyphar Jul 22, 2016

wking Jul 22, 2016

cyphar Jul 22, 2016

wking Jul 22, 2016

cyphar commented Jul 22, 2016

wking commented Jul 22, 2016

cyphar commented Jul 22, 2016

crosbymichael commented Jul 22, 2016 •

edited

Loading

hqhq commented Jul 25, 2016 •

edited

Loading

config-linux: clarify cgroup requirements #493

config-linux: clarify cgroup requirements #493

Conversation

cyphar commented Jun 8, 2016 • 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

Choose a reason for hiding this comment

wking Jun 10, 2016 via email

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

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

Choose a reason for hiding this comment

Mashimiao commented Jul 7, 2016

cyphar commented Jul 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar commented Jul 21, 2016

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

Choose a reason for hiding this comment

cyphar commented Jul 22, 2016

wking commented Jul 22, 2016

cyphar commented Jul 22, 2016

crosbymichael commented Jul 22, 2016 • edited Loading

hqhq commented Jul 25, 2016 • edited Loading

cyphar commented Jun 8, 2016 •

edited

Loading

crosbymichael commented Jul 22, 2016 •

edited

Loading

hqhq commented Jul 25, 2016 •

edited

Loading