RFR: 8343191: Cgroup v1 subsystem fails to set subsystem path [v11]
Sergey Chernyshev
schernyshev at openjdk.org
Fri Feb 21 19:56:17 UTC 2025
On Mon, 17 Feb 2025 11:28:38 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:
>> Sergey Chernyshev has updated the pull request incrementally with one additional commit since the last revision:
>>
>> added details in the log message
>
> test/hotspot/jtreg/containers/docker/TestMemoryWithSubgroups.java line 60:
>
>> 58:
>> 59: Common.prepareWhiteBox();
>> 60: DockerTestUtils.buildJdkContainerImage(imageName);
>
> This can be moved out of the condition. It's there in both cases.
Done
> test/hotspot/jtreg/containers/docker/TestMemoryWithSubgroups.java line 85:
>
>> 83: } else {
>> 84: System.out.println("Metrics are from neither cgroup v1 nor v2, skipped for now.");
>> 85: }
>
> Suggestion:
>
> throw new RuntimeException("cgroup version not known? was: " + metrics.getProvider());
I made it with `jtreg.SkippedException`
> test/hotspot/jtreg/containers/docker/TestMemoryWithSubgroups.java line 98:
>
>> 96: opts.addDockerOpts("--privileged")
>> 97: .addDockerOpts("--cgroupns=" + (privateNamespace ? "private" : "host"))
>> 98: .addDockerOpts("--memory", "1g");
>
> Please extract this "larger" memory limit out of this function. The expectation is to have:
>
>
> Container memory limit => X
> Some lower limit in a moved path => Y
>
> Expect that the lower limit of Y is being detected.
>
>
> For example:
>
>
> testMemoryLimitSubgroupV1("1g", "100m", "104857600", false);
done.
> test/hotspot/jtreg/containers/docker/TestMemoryWithSubgroups.java line 118:
>
>> 116: opts.addDockerOpts("--privileged")
>> 117: .addDockerOpts("--cgroupns=" + (privateNamespace ? "private" : "host"))
>> 118: .addDockerOpts("--memory", "1g");
>
> Same here with the `1g` container memory limit. Move it to a parameter.
done. extracted the parameter.
> test/jdk/jdk/internal/platform/docker/TestDockerMemoryMetricsSubgroup.java line 72:
>
>> 70: } else {
>> 71: System.out.println("Metrics are from neither cgroup v1 nor v2, skipped for now.");
>> 72: }
>
> Please `throw` here.
Done
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1966094564
PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1966094301
PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1966093295
PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1966092984
PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1966092288
More information about the core-libs-dev
mailing list