config-linux.md: add cgroupsPath entry and some modifications#823
Conversation
config-linux.md
Outdated
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
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
wking
left a comment
wking
left a comment
There was a problem hiding this comment.
Also “smome” → “some” in the commit message summary and PR topic.
config-linux.md
Outdated
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
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
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 |
5 participants
Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com