Skip to content

Conversation

@wking
Copy link
Contributor

@wking wking commented Nov 16, 2015

This should help clarify the cgroupsPath setting added in #137, which
was the subject of some confusion in opencontainers/runc#397. Issues
I'm trying to clarify here:

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
to be even clearer. I stayed away from the “neither are
set” case, since I covered that fairly directly in #237, which that
was punted back to the list and has seen no further interest. So
I'm not clear on what the intended semantics are there, although
Mrunal's wording seems to agree with the proposal in #237.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, if it's general facets for cgroup, I don't think it's typical enough and helps much.
I suggest we remove this hunk, the others look good to me.

ping @vishh @mrunalp WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Nov 18, 2015 at 03:35:28AM -0800, Qiang Huang wrote:

+There are a few facets to this:
+
+* Managing the existence of cgroups within the hierarchy (cgroupsPath).
+* Assigning processes to cgroups.
+* Configuring cgroups to apply resource limits (resources).

I don't know, if it's general facets for cgroup, I don't think it's
typical enough and helps much. I suggest we remove this hunk, the
others look good to me.

I think the lack of a distinction between “what the cgroup is called”
and “how the cgroup is configured” is part of the confusion we've seen
around ‘cgroupsPath’ and ‘resources’. I was trying to clarify that
distinction here, but if it doesn't need to be clarified (or there's a
better way to clarify it), I'm happy drop (or replace) this hunk.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand what's the meaning of "Managing the existence of cgroups within the hierarchy", does this includes:

  • Assigning processes to cgroups (where to assign to)
  • Configuring cgroups to apply resource limits (what kind of resources limits)

Because above two are almost all we can do to manage cgroup. I think the three facets are not on the same page, that's why I think is not clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Thu, Dec 10, 2015 at 11:16:03PM -0800, Qiang Huang wrote:

+There are a few facets to this:
+
+* Managing the existence of cgroups within the hierarchy (cgroupsPath).
+* Assigning processes to cgroups.
+* Configuring cgroups to apply resource limits (resources).
+

I don't quite understand what's the meaning of "Managing the
existence of cgroups within the hierarchy", does this includes:

  • Assigning processes to cgroups (where to assign to)
  • Configuring cgroups to apply resource limits (what kind of
    resources limits)

No, those are both separate facets in my list.

Because above two are almost all we can do to manage cgroup. I think
the three facets are not on the same page, that's why I think is not
clear.

“Adding process to a cgroup” and “applying resource limits” are both
independent of where the cgroup lives in the hierarchy
(cgroupsPath). For example, the cgroup setup is something like:

$ CGROUP=/sys/fs/cgroup/cpuset/a/b/c
$ mkdir -p "${CGROUP}"
$ echo 123 >"${CGROUP}/cgroup.procs
$ echo 2-3 >"${CGROUP}/cpuset.cpus

the first two lines are “managing the existence of cgroups within the
hierarchy”, the third line is “assigning processes to cgroups”, and the
fourth line is “configuring cgroups to apply resource limits”.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wking That explains, thanks. In my understanding, "Assigning processes to cgroups (where to assign to)" includes where you say "managing the existence of cgroups within the hierarchy", but it makes sense to separate them.

@mrunalp mrunalp added this to the v0.2.0 milestone Dec 9, 2015
@wking
Copy link
Contributor Author

wking commented Dec 10, 2015

I see this got tagged for v0.2.0. I haven't heard anything about it since posting the motivation for the facet list (but as I said there, I'm happy to drop/replace the hunk if that motivation doesn't sound convincing). So I'm not aware of anything I can do to help push this forward, and I think it's just blocked on further maintainer review and merging. If there is something I can do to help push this forward, just let me know :).

@hqhq
Copy link
Contributor

hqhq commented Dec 14, 2015

LGTM

1 similar comment
@mrunalp
Copy link
Contributor

mrunalp commented Dec 16, 2015

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

within what hierarchy?
cgroupsPath is the path to a cgroups hierarchy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume he means the unified hierarchy, since subsequently it mentions the spec does not support split, but would be good to make that explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Dec 16, 2015 at 10:23:07AM -0800, Vish Kannan wrote:

+* Managing the existence of cgroups within the hierarchy (cgroupsPath).

within what hierarchy? cgroupsPath is the path to a cgroups
hierarchy.

This spec requires a unified cgroups hierarchy 1. cgroupsPath
locates the cgroup for this process within that hierarchy. The
runtime creates cgroups as needed to make that happen, and removes any
cgroups it created (but which are no longer used) when it cleans up
after itself. That makes the “within the hierarchy” phrasing clear to
me, but if you have an alternative that you prefer I'll switch to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Dec 16, 2015 at 11:02:59AM -0800, Jonathan Boulle wrote:

+* Managing the existence of cgroups within the hierarchy (cgroupsPath).

I assume he means the unified hierarchy, since subsequently it
mentions the spec does not support split, but would be good to make
that explicit

I can make “the hierarchy” a link to 1. But as far as I know, we
don't actually require you to use that unified-hierarchy
implementation. We just don't allow you to give per-controller paths.
I think that's too complicated an idea for this summary-list, but I'll
happily add more detail to the existing split-hierarchy reference to
clarify this point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As is, this could allow for either, though can you point me to where unified-hierarchy is explicitly required for this spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Tue, Jan 05, 2016 at 12:14:37PM -0800, Vincent Batts wrote:

As is, this could allow for either, though can you point me to where
unified-hierarchy is explicitly required for this spec?

The wording (from 429f936, #137) is:

The Spec does not support split hierarchy

And the surrounding discussion 1 was (paraphrased):

me: cgexec(1) lets you set per-controller paths, do we want that?
@vishh: Won't this break with unified hierarchy 2?
me: I see. Can we link to those kernel docs?

I could see someone reading that as either:

  • “we require a __DEVEL__sane_behavior mount” or
  • “you only get to specify one cgroup path, not per-controller paths.
    Whether the runtime implements that via an __DEVEL__sane_behavior
    mount or not is undefined”.

Personally I'm in favor of the latter interpretation, but I think the
current wording could go either way. In either case, there is only
one name for all controllers, so I feel comfortable saying “the
hierarchy” here.

@wking wking force-pushed the cgroups-path-modify-join branch 2 times, most recently from e79f8dc to 734205b Compare December 16, 2015 18:51
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]>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also affected by cgroupsPath, right? Actually I find this list pretty unclear as-is, I think it should explain in more detail something like: "there are multiple aspects (facets) to cgroups ... the spec interacts with them in different ways based on certain configuration parameters ... bla bla"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Dec 16, 2015 at 11:06:00AM -0800, Jonathan Boulle wrote:

+* Moving processes to respective cgroups.

This is also affected by cgroupsPath, right?

I don't think so (see my breakdown here 1)

Actually I find this list pretty unclear as-is…

And so has everyone else that has reviewed it ;). So I think we need
to either drop it completely or replace it with something like the
shell example in 2.

@vbatts
Copy link
Member

vbatts commented Jan 5, 2016

where do we stand on this?

@wking
Copy link
Contributor Author

wking commented Jan 5, 2016

On Tue, Jan 05, 2016 at 12:09:26PM -0800, Vincent Batts wrote:

where do we stand on this?

I need to reroll to replace the facets section with a shell-example
1, so we have a more concrete anchor for the ideas than my current
handwaving. And @vishh needs to clarify whether or not he wants more
more clarification for my “existing container” entries.

@vbatts vbatts modified the milestones: v0.3.0, v0.2.0 Jan 5, 2016
@vbatts
Copy link
Member

vbatts commented Jan 5, 2016

bumping this, so we can move on from a v0.2.0

@crosbymichael
Copy link
Member

The existing content in this section is already confusing to users and these additional changes do not clarify as well as what we could. I'll try to rework this section as a whole and clarify some of the cgroups and resource information as a whole.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants