support pids cgroup

[yylt]

Fix #75

[klihub]

@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]

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]

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?

[AkihiroSuda]

We have been converting memory and cpu, so converting pids SGTM

[klihub]

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

• pkg/api: add ContainerAdjustment setter, probably SetLinuxPidLimits(int64)
• pkg/adaptation: add collection, conflict detection and other handling for the new pid limits to result
• pkg/runtime-tools/generate: implement pid limit adjustment in the OCI Spec in Generator.AdjustResources()
• pkg/runtime-tools/generate(/generate_suite_test.go:"Adjustment" ginkgo test case): preferably also add a test case here for the new functionality

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

[klihub]

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.

[yylt]

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

[yylt]

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

[mikebrow]

LGTM with the open issue of adding a v2 to v1 conversion ... helper

[yylt]

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]

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]

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.