config-linux: add systemd cgroup path convention #1115
Conversation
There was a problem hiding this comment.
We should just have a documentation for the existing implementation of systemd cgroupsPath, e.g. user.slice:nerdctl:deadbeef
https://github.com/containerd/nerdctl/blob/07c3ca23743308987aa3b26b1fb7b560804ff9a4/cmd/nerdctl/run_cgroup_linux.go#L53
I don't think we need to add new fields to the spec.
@AkihiroSuda Thanks for the comments! Actually I thought over this a bit. My original intention was trying to make it consistent for all implementations, since I'm not certain if they all follow the exactly same cgroups path convention and use it the same way. I went through runc, crun and youki to double check:
I agree it's fine to only add docs, just need more clarification. Let me do an update. Besides, do you think it's reasonable to add a field for enabling/disabling "systemd-cgroup"? Thanks! |
kailun-qin
force-pushed
the
add-systemd-cgroup-path
branch
from
32e5fd9 to
4716058
Compare
@AkihiroSuda Updated. PTAL, thanks! |
kailun-qin
changed the title
|
@kolyshkin PTAL |
There was a problem hiding this comment.
This is somewhat similar to work I did documenting the runc systemd cgroup driver; see https://github.com/opencontainers/runc/blame/main/docs/systemd.md
In fact, I see that some wording is similar so this might be a derived work. If that is the case, perhaps it makes sense to at least refer to the source document(s) in the commit message.
What's missing from this description is
- all three components are optional;
- what the defaults for omitted components are (or can be)
config-linux.md
Outdated
| @@ -176,11 +176,31 @@ For more information, see the [kernel cgroups documentation][cgroup-v1]. | |||
| **`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` MUST be either an absolute path or a relative path. | |||
| If the runtime creates cgroups and sets cgroup limits on its own (aka. fs cgroup driver mode), the value of `cgroupsPath` MUST be either an absolute path or a relative path. | |||
There was a problem hiding this comment.
I would rephrase it as
If the runtime manages cgroups on its own (i.e. works with cgroupfs directly), the value of ...
config-linux.md
Outdated
|
|
||
| * In the case of an absolute path (starting with `/`), the runtime MUST take the path to be relative to the cgroups mount point. | ||
| * In the case of a relative path (not starting with `/`), the runtime MAY interpret the path relative to a runtime-determined location in the cgroups hierarchy. | ||
|
|
||
| If the runtime use systemd cgroup driver to create cgroups and set cgroup limits, the value of `cgroupsPath` MUST be in the "slice:prefix:name" form (e.g. "system.slice:runtime:434234"). |
There was a problem hiding this comment.
If the runtime manages cgroups indirectly, via systemd, the value of ...
4716058 to
8fe6eee
Compare
kailun-qin
requested review from
crosbymichael,
cyphar,
dqminh,
hqhq,
mrunalp,
tianon and
vbatts
as code owners
Added in the commit message.
Updated and reflected in the spec descriptions. |
The systemd cgroup path convention currently implemented in runtimes like `runc/crun` should be added to the spec. For more information, please kindly refer to e.g. runc systemd cgroup driver doc: https://github.com/opencontainers/runc/blame/main/docs/systemd.md. This patch adds the systemd cgroup convention for `Linux.CgroupsPath` which is in the `slice:prefix:name` form and clarifies the detailed usage. Fixes opencontainers#1021 Signed-off-by: Kailun Qin <[email protected]>
|
@kailun-qin thanks! Have you checked this against crun? IOW, is crun fully conformant to this spec? |
| Note that `slice` can contain dashes to denote a sub-slice (e.g. `user-1000.slice` is a correct | ||
| notation, meaning a subslice of `user.slice`), but it must not contain slashes (e.g. | ||
| `user.slice/user-1000.slice` is invalid). A `slice` of `-` represents a root slice. | ||
| If not specified, it can default to: |
There was a problem hiding this comment.
| If not specified, it can default to: | |
| If not specified, it SHOULD default to: |
The systemd cgroup path convention currently implemented in runtimes like
runc/crunshould be added to the spec. For more information, please kindly refer to e.g. runc systemd cgroup driver doc:https://github.com/opencontainers/runc/blame/main/docs/systemd.md.
This patch adds the systemd cgroup convention for
Linux.CgroupsPathwhich is in theslice:prefix:nameform and clarifies the detailed usage.Fixes #1021