config-linux.md: add cgroupsPath entry and some modifications #823
Conversation
config-linux.md
Outdated
| * ... 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 MAY define the default cgroup path. | ||
| #### <a name="configLinuxCgroupsPath" />Cgroups Path |
There was a problem hiding this comment.
The previous section is ## Control groups, so you only need ### here (not ####).
There was a problem hiding this comment.
I just want keep consistent with blkio, cpu, and so on. Those sections all start with ####.
Do you mean we should modify them?
There was a problem hiding this comment.
Do you mean we should modify them?
Yes, and I've filed #832 to do that independent of content changes.
config-linux.md
Outdated
| * ... not specified, the runtime MAY define the default cgroup path. | ||
| #### <a name="configLinuxCgroupsPath" />Cgroups Path | ||
|
|
||
| **`cgroupsPath`** (string, OPTIONAL) path to the cgroups. It can be used to either control the cgroups hierarchy for containers or to run a new process in an existing container.The value of `cgroupsPath` is either an absolute path or a relative path. |
There was a problem hiding this comment.
I'd recommend one sentence per line, e..g see #811 where you split multi-sentence lines in a few places. And if you don't split the sentences onto their own lines, you'll want a space before “The value”.
Mashimiao
force-pushed
the
add-cgroupspath-entry
branch
from
aa995f7
to
f871f49
Compare
config-linux.md
Outdated
| * an absolute path (starting with `/`), the runtime MUST take the path to be relative to the cgroups mount point. | ||
| * a relative path (not starting with `/`), the runtime MAY interpret the path relative to a runtime-determined location in the cgroups hierarchy. | ||
|
|
||
| If the value is specified, the runtime MUST consistently attach to the same place in the cgroup hierarchy given the same value of `cgroupsPath`. |
There was a problem hiding this comment.
I have no preference on “cgroup hierarchy” vs “cgroups hierarchy”, but you have “cgroup hierarchy” here and “cgroups hierarchy” on line 177 (as of f871f49).
Mashimiao
force-pushed
the
add-cgroupspath-entry
branch
from
f871f49
to
1ddc135
Compare
Mashimiao
changed the title
config-linux.md
Outdated
| It can be used to either control the cgroups hierarchy for containers or to run a new process in an existing container. | ||
|
|
||
| The value of `cgroupsPath` is either an absolute path or a relative path. | ||
| * an absolute path (starting with `/`), the runtime MUST take the path to be relative to the cgroups mount point. |
There was a problem hiding this comment.
Suggest adding "In the case of" in front of this and the next line.
Mashimiao
force-pushed
the
add-cgroupspath-entry
branch
from
1ddc135
to
5f988bc
Compare
config-linux.md
Outdated
| **`cgroupsPath`** (string, OPTIONAL) path to the cgroups. | ||
| It can be used to either control the cgroups hierarchy for containers or to run a new process in an existing container. | ||
|
|
||
| The value of `cgroupsPath` is either an absolute path or a relative path. |
There was a problem hiding this comment.
“is” → “MUST be”? And we can punt to POSIX for formal definitions of absolute and relative paths.
And do we want to clarify how the empty string is handled ("cgroupsPath": "")? POSIX doesn't come down firmly on this point, and allows empty-string paths which may result in errors or be synonyms for .. I'd rather avoid that ambiguity in this spec and forbid explicit empty strings here (although see similar discussion in the rejected #843).
There was a problem hiding this comment.
empty string means the cgroupsPath is not specified, runtime MAY define the default cgroups path as the following sentence says
There was a problem hiding this comment.
empty string means the cgroupsPath is not specified…
Does the spec support that? I think "cgroupsPath": "" is specifying cgroupsPath, because the cgroupsPath key exists in the linux object. If we want to conflate "" with “unset” for this property (or in general?), we'd need to land spec wording to that effect.
Personally, I think life is just as easy for config authors and simpler for runtimes if we forbid empty-string values where they don't make sense. Runtimes don't have to validate configs if they don't want, so making empty strings illegal would mean that runtimes could handle those configs however they like (erroring out or conflating them with whatever other case they liked).
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
Mashimiao
force-pushed
the
add-cgroupspath-entry
branch
from
5f988bc
to
1742fbf
Compare
There seems to be confusion about whether the empty string (and possible other values that match Go's zero values) qualifies as "set" or not [1]. This commit clarifies that Go zero values are not relevant to the spec, but it does not address how they should be handled (I'm leaving that to follow-up work). [1]: opencontainers#823 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
There seems to be confusion about whether the empty string (and possible other values that match Go's zero values) qualifies as "set" or not [1]. This commit clarifies that Go zero values are not relevant to the spec, but it does not address how they should be handled (I'm leaving that to follow-up work). [1]: opencontainers#823 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
|
We are currently reworking the cgroupsPath description #834. |
|
@crosbymichael yeah, I noticed that, but I don't think we should close this PR. In this PR, I mainly want to add cgroupsPath entry. As you know, there is not it yet in the spec. |
|
LGTM |
1 similar comment
|
LGTM |
Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com