On Wed, 14 Aug 2024 02:59:33 GMT, Jan Kratochvil <jkratoch...@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
