On Tue, 10 May 2022 12:29:10 GMT, Severin Gehwolf <[email protected]> wrote:
> Please review this change to the cgroup v1 subsystem which makes it more
> resilient on some of the stranger systems. Unfortunately, I wasn't able to
> re-create a similar system as the reporter. The idea of using the longest
> substring match for the cgroupv1 file paths was based on the fact that on
> systemd systems processes run in separate scopes and the maven forked test
> runner might exhibit this property. For that it makes sense to use the common
> ancestor path. Nothing changes in the common cases where the `cgroup_path`
> matches `_root` and when the `_root` is `/` (container the former, host
> system the latter).
>
> In addition, the code paths which are susceptible to throw NPE have been
> hardened to catch those situations. Should it happen in the future it makes
> more sense (to me) to not have accurate container detection in favor of
> continuing to keep running.
>
> Finally, with the added unit-tests a bug was uncovered on the "substring"
> match case of cgroup paths in hotspot. `p` returned from `strstr` can never
> point to `_root` as it's used as the "needle" to find in "haystack"
> `cgroup_path` (not the other way round).
>
> Testing:
> - [x] Added unit tests
> - [x] GHA
> - [x] Container tests on cgroups v1 Linux. Continue to pass
I just started to look at the code so just one comment for now.
src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1SubsystemController.java
line 65:
> 63: path = mountPoint + cgroupSubstr;
> 64: }
> 65: } else {
Looks like `path` is still not set if the condition at line 61 `if
(cgroupPath.length() > root.length()) {` is false.
-------------
PR: https://git.openjdk.java.net/jdk/pull/8629
