search

containerd / nri

Node Resource Interface
Apache License 2.0
238 stars 62 forks source link

support pids cgroup #76

Closed yylt closed 1 month ago

yylt commented 5 months ago

Fix https://github.com/containerd/nri/issues/75

klihub commented 5 months ago

@yylt: Do you have a use case for this ? I mean, AFAICT this PR tries to add cgroup v1 support for something that we already should support for cgroup v2 using LinuxResources.Unified["pids.max"]. Also, this PR only adds the placeholder for the cgroup v1 max pids setting in the wire protocol. It does not implement doing anything with the actual value if it happens to be set.

We have been trying for a while now to avoid adding anything new cgroup v1-specific bits to NRI.

yylt commented 5 months ago

There are still many distributions, like Rocky Linux 8, kylin linux , and the Rocky Linux 8 uses the 4.18 kernel. Since cgroup v2 recommends a kernel version higher than 5.2, I think it's still necessary to add support for the v1 version of cgroups.

Additional, maybe nri should follow the OCI runtime spec more closely. If the runtime spec doesn't support a specific cgroup, then nri should remove or not add it

mikebrow commented 5 months ago

There are still many distributions, like Rocky Linux 8, kylin linux , and the Rocky Linux 8 uses the 4.18 kernel. Since cgroup v2 recommends a kernel version higher than 5.2, I think it's still necessary to add support for the v1 version of cgroups.

Additional, maybe nri should follow the OCI runtime spec more closely. If the runtime spec doesn't support a specific cgroup, then nri should remove or not add it

Just adding a note here regarding possibility of v2 to v1 conversions ..

https://github.com/opencontainers/runtime-spec/blob/main/config-linux.md#example-12

see example ^ and description: "If a controller is enabled on the cgroup v2 hierarchy but the configuration is provided for the cgroup v1 equivalent controller, the runtime MAY attempt a conversion."

@AkihiroSuda thoughts? Should we be enabling both v1 and v2/unified and exposing the runtime meta at this plugin layer... or push the NRI plugins to cgroupv2 and do any nec. conversions either in CRI/shim or runtime?

klihub commented 2 months ago

@yylt IMO, for making this really usable and getting it merged, IMO we'd need to put in a few more missing details:

The currently open PR #94 can be used as a very good example of a similar functional addition in full detail.

yylt commented 2 months ago

@klihub , thank you response. If the PID can be modified by NRI, the remaining parts will be improved accordingly.

yylt commented 2 months ago

There are a few remaining bits missing for this to become really useful and mergeable. PR #94, which adds similar functionality for OOM Score adjustment, can be used as an example/template on what needs to be done.

Done

yylt commented 1 month ago

have been converting memory and cpu

sorry, I didn't quite understand the part about 'have been converting memory and CPU.' I haven't seen any related program in cri / runc.

Could you provide some examples?

mikebrow commented 1 month ago

have been converting memory and cpu

sorry, I didn't quite understand the part about 'have been converting memory and CPU.' I haven't seen any related program in cri / runc.

Could you provide some examples?

Will let @AkihiroSuda chime in :) But I believe this is an example: https://github.com/opencontainers/runc/blob/ad5b481dace5cda8ca7c659b7717a15517333198/libcontainer/cgroups/utils_test.go#L541-L634

yylt commented 1 month ago

have been converting memory and cpu

sorry, I didn't quite understand the part about 'have been converting memory and CPU.' I haven't seen any related program in cri / runc. Could you provide some examples?

Will let @AkihiroSuda chime in :) But I believe this is an example: https://github.com/opencontainers/runc/blob/ad5b481dace5cda8ca7c659b7717a15517333198/libcontainer/cgroups/utils_test.go#L541-L634

In the examples, the focus is on the conversion from v1 to v2, primarily due to the addition of new fields or changes in the meaning in v2, necessitating the conversion.

Currently, the pids only has a single limit setting (type u64), without any changes in meaning or new fields being added, so there are no any need changed here?

Regarding the conversion from v2 to v1, it appears that this was initially prevented from the outset in v1, as seen in https://github.com/opencontainers/runc/blob/ad5b481dace5cda8ca7c659b7717a15517333198/libcontainer/cgroups/fs/fs.go#L175.

If there are any issues or corrections needed, please feel free to point them out. Thank you.