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

[Severin Gehwolf]

On Tue, 30 Jul 2024 07:47:55 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 two additional commits since the last revision:
> 
>  - Inline adjust_controller() twice
>  - Revert "Unify 4 copies of adjust_controller()"
>    
>    This reverts commit 77a81d07d74c8ae9bf34bfd8df9bcaca451ede9a.

Changes seem OK. The test needs some cleanup, though.

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

> 101:             Asserts.assertEQ(0, exitValue, "Process returned unexpected exit code: " + exitValue);
> 102:             return output;
> 103:         }

This could be simplified to just `ProcessTools.executeProcess(new ProcessBuilder(args));`

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

> 118:             args.add(jdkTool);
> 119:             args.add("-cp");
> 120:             args.add(System.getProperty("java.class.path"));

Should probably be `test.classes` instead of `java.class.path`.

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

> 143:                 }
> 144:                 throw e;
> 145:             }

This should no longer be needed since we have the `@requires` line.

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

> 191:             System.err.println(LINE_DELIM + " " + (isCgroup2 ? "cgroup2" : "cgroup1") + " mount point: " + sysFsCgroup);
> 192:             memory_max_filename = isCgroup2 ? "memory.max" : "memory.limit_in_bytes";
> 193:             Files.writeString(Path.of(sysFsCgroup + "/" + CGROUP_OUTER + "/" + memory_max_filename), "" + MEMORY_MAX_OUTER);

This still doesn't work on cgv1 (hybrid). The reg-ex pattern matching is wrong. I'd suggest to simplify this by using `cgget` and `cgset` with forked processes. Something like this (depending on the version use `memory.max` or `memory.limit_in_bytes`):


$ cgcreate -g memory:/jdktest128331
$ cgcreate -g memory:/jdktest128331/inner
$ cgset -r memory.limit_in_bytes=512000 /jdktest128331/inner
$ cgget -n -v -r memory.limit_in_bytes /jdktest128331/inner
512000

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

> 208:         public Limits hook(List<String> cgexec) throws IOException {
> 209:             // CgroupV1Subsystem::read_memory_limit_in_bytes considered hierarchical_memory_limit only when inner memory.limit_in_bytes is unlimited.
> 210:             Files.writeString(Path.of(sysFsCgroup + "/" + CGROUP_OUTER + "/" + CGROUP_INNER + "/" + memory_max_filename), "" + MEMORY_MAX_INNER);

This should be done using `cgset`.

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

> 215: 
> 216:             // KFAIL - verify the CgroupSubsystem::initialize_hierarchy() and jdk.internal.platform.CgroupSubsystem.initializeHierarchy() bug
> 217:             // TestTwoLimits does not see the lower MEMORY_MAX_OUTER limit.

Remove this obsolete(?) comment.

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

> 245:             self_verbose.add("-version");
> 246:             pSystem(self_verbose);
> 247:         }

Instead of disabling the memory controller that way, consider creating only the cpu controller (using `cgcreate`) and test for memory limits?

test/jtreg-ext/requires/VMProps.java line 141:

> 139:         map.put("vm.flagless", this::isFlagless);
> 140:         map.put("jdk.foreign.linker", this::jdkForeignLinker);
> 141:         map.put("vm.cgroup.tools", this::cgroupTools);

Why the `vm` prefix? Could be just `cgroup.tools` no? This isn't a JVM property.

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

PR Review: https://git.openjdk.org/jdk/pull/17198#pullrequestreview-2233709616
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714213497
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714218948
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714219641
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714181908
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714221143
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714221815
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714226775
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714228598