RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v20]

[Severin Gehwolf]

On Wed, 14 Aug 2024 02:59:33 GMT, Jan Kratochvil <jkratochvil at openjdk.org> wrote:

>> The testcase requires root permissions.
>> 
>> Fix by  Severin Gehwolf.
>> Testcase by Jan Kratochvil.
>
> Jan Kratochvil has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Testcase update upon review by Severin Gehwolf

I've got my quibbles with the NestedCgroup test. In a nutshell, `TestTwoLimits` and `TestNoController` are the same. Only for the latter the `cpu` controller is enabled, we set the same `memory` limits for that tree and then assert the same as for `TestTwoLimits`. I have to ask: why is `TestNoController` useful? It might have been in some version of the patch, but not in its current form. In OpenJDK we just assume that certain controllers (`cpu` and `memory` are enabled). If they aren't then we return unlimited. We don't look at `cgroup.subtree_control` files.

That leaves `TestTwoLimits` which essentially tests for a system that's badly configured and we haven't seen in the wild. For what purpose? So that we encode in a test what we know we don't yet support, because "you ain't gonna need it". If you really insist to keep it, then please clean it up. I've spent way too many cycles understanding it and would like to spare somebody else needing to do the same.

test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 91:

> 89: 
> 90:         public Test(String controller_) throws Exception {
> 91:             controller = controller_;

You are re-assigning a static class value's value in a constructor. This is bound to break.

test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 109:

> 107: 
> 108:             // Alpine Linux 3.20.1 needs cgcreate1 otherwise:
> 109:             // cgcreate: can't create cgroup [...]: No such file or directory

Suggestion:

            // Create the parent cgroup hierarchy

test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 118:

> 116:              || output.contains("cgcreate: can't create cgroup " + CGROUP_OUTER + ": Cgroup, requested group parameter does not exist")) {
> 117:                 throw new SkippedException("Missing root permission: " + output.getStderr());
> 118:             }

That we are the UID 0 ought to be checked in `NestedCgroup.main` before any test is being run. Not here somewhere down the line. Use `Platform.isRoot()`.

test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 139:

> 137:             } else {
> 138:               throw new IllegalArgumentException();
> 139:             }

This can be done once in `NestedCgroup.main` and then passed in on instantiation of `Test*` classes.

test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 146:

> 144: 
> 145:             HookResult hookResult = hook();
> 146:             // C++ CgroupController

Please remove this comment (or rephrase). It's not useful as it is.

test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 160:

> 158:             HookResult hookResult = new HookResult();
> 159: 
> 160:             // CgroupV1Subsystem::read_memory_limit_in_bytes considered hierarchical_memory_limit only when inner memory.limit_in_bytes is unlimited.

There is no `hierarchical_memory_limit` any more. Please remove the comment. It might get stale soon. If you really want a comment add a more high level one.

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

PR Review: https://git.openjdk.org/jdk/pull/17198#pullrequestreview-2237962572
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1717191803
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1717095471
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1716837525
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1717193195
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1717096997
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1716803597