On Thu, 20 Jun 2024 12:06:43 GMT, Severin Gehwolf <[email protected]> wrote:
>> Please review this enhancement to the container detection code which allows
>> it to figure out whether the JVM is actually running inside a container
>> (`podman`, `docker`, `crio`), or with some other means that enforces
>> memory/cpu limits by means of the cgroup filesystem. If neither of those
>> conditions hold, the JVM runs in not containerized mode, addressing the
>> issue described in the JBS tracker. For example, on my Linux system
>> `is_containerized() == false" is being indicated with the following trace
>> log line:
>>
>>
>> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false
>> because no cpu or memory limit is present
>>
>>
>> This state is being exposed by the Java `Metrics` API class using the new
>> (still JDK internal) `isContainerized()` method. Example:
>>
>>
>> java -XshowSettings:system --version
>> Operating System Metrics:
>> Provider: cgroupv1
>> System not containerized.
>> openjdk 23-internal 2024-09-17
>> OpenJDK Runtime Environment (fastdebug build
>> 23-internal-adhoc.sgehwolf.jdk-jdk)
>> OpenJDK 64-Bit Server VM (fastdebug build
>> 23-internal-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing)
>>
>>
>> The basic property this is being built on is the observation that the cgroup
>> controllers typically get mounted read only into containers. Note that the
>> current container tests assert that `OSContainer::is_containerized() ==
>> true` in various tests. Therefore, using the heuristic of "is any memory or
>> cpu limit present" isn't sufficient. I had considered that in an earlier
>> iteration, but many container tests failed.
>>
>> Overall, I think, with this patch we improve the current situation of
>> claiming a containerized system being present when it's actually just a
>> regular Linux system.
>>
>> Testing:
>>
>> - [x] GHA (risc-v failure seems infra related)
>> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2
>> (including gtests)
>> - [x] Some manual testing using cri-o
>>
>> Thoughts?
>
> Severin Gehwolf has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove problem listing of PlainRead which is reworked here
Seems okay. I don't understand the depths of V1 vs V2 controller files as well
as you do, @jerboaa , but I trust you there. The mechanics seem fine.
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 373:
> 371: * (11) mount source: matched with '%*s' and discarded
> 372: * (12) super options: matched with '%*s' and discarded
> 373: */
Thanks for the good comment. That scanf line is a brain teaser.
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 422:
> 420: * (12) super options: matched with '%s' and captured in
> 'tmpcgroups'
> 421: */
> 422: if (sscanf(p, "%*d %*d %*d:%*d %s %s %s%*[^-]- %s %*s %s", tmproot,
> tmpmount, mount_opts, tmp_fs_type, tmpcgroups) == 5) {
The only difference to v1 is that we parse the super options (12), right? Could
we factor out the parsing into a helper function? Or, alternatively, at least
`#define` the scanf format somewhere up top, including the nice comment, and
reuse that format string?
-------------
Marked as reviewed by stuefe (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18201#pullrequestreview-2130943896
PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1647881202
PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1647925120
