RFR: 8349988: Change cgroup version detection logic to not depend on /proc/cgroups

[Thomas Fitzsimmons]

On Fri, 28 Feb 2025 10:21:09 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> Hmm, this logic was there before my changes; I am pretty sure I did keep `cpuset` required for `cgroups v1`.
>> 
>> To attempt to confirm my patch had not changed the `cgroups v1` `cpuset` optionality logic, I wrote a test case for "cgroups v1, cpuset not present in /proc/cgroups".
>> 
>> (To avoid assertion failures I created fake `/proc/self/cgroup` and `/proc/self/mountinfo` files that also do not have `cpuset` lines.)
>> 
>> I adapted the test case to before my patch:
>> 
>> https://github.com/fitzsim/jdk/commit/cab0000598b522d38865452575f95ed368b5935b
>> 
>> and after my patch:
>> 
>> https://github.com/fitzsim/jdk/commit/d8537baed5d56b5f36ce628b8c180ce18ce6f6e6
>> 
>> and it passes in both cases.  i.e., for `cgroups v1`, `cpuset` is required, so `determine_type` produces `INVALID_CGROUPS_V1 (4)`, from this logic:
>> 
>> 
>>   if (!cg_infos[CPUSET_IDX]._data_complete) {
>>     log_debug(os, container)("Required cgroup v1 cpuset subsystem not found");
>>     cleanup(cg_infos);
>>     *flags = INVALID_CGROUPS_V1;
>>     return false;
>>   }
>> 
>> 
>> The test fails in both cases if I remove ` && i != CPUSET_IDX`, with  `determine_type` returning `INVALID_CGROUPS_GENERIC (6)`:
>> 
>> 
>> Required cpuset controller missing in /proc/cgroups. Invalid. expected: 4 but was: 6
>> 
>> 
>> due to this block (which my patch left alone):
>> 
>> 
>>   if (!all_required_controllers_enabled) {
>>     // one or more required controllers disabled, disable container support
>>     log_debug(os, container)("One or more required controllers disabled at kernel level.");
>>     cleanup(cg_infos);
>>     *flags = INVALID_CGROUPS_GENERIC;
>>     return false;
>>   }
>> 
>> 
>> No existing test checks this logic as far as I can tell, so if you think the above makes sense, I can add this test to this pull request.
>
> You are right, that if any of the required controllers aren't enabled at the kernel level we fail with `NVALID_CGROUPS_GENERIC`. However, the `if` condition is within the cgroups v1 branch while it used to be outside a version specific branch.
> 
> Also note that `/proc/cgroup` containing (last digit `0`, indicating the enabled flag):
> 
> 
> cpuset  3   1   0\n
> 
> 
> ... is semantically equivalent to it being missing entirely from `proc/cgroup`. But keeping `if (i != PIDS_IDX && i != CPUSET_IDX) {` above, would keep `all_required_controllers_enabled == true` which is not correct. Yes, we should keep/add a test like you suggest, but amend the patch to something like this: https://github.com/jerboaa/jdk/commit/26f765db9fef6f1d7be79452da701987274117c5

Makes sense, will do.  I will also simplify the test case as you suggest.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23811#discussion_r1975345816