[jdk8u-dev] RFR: 8230305: Cgroups v2: Container awareness [v2]

[Severin Gehwolf]

On Wed, 5 Oct 2022 08:26:01 GMT, Jonathan Dowland <jdowland at openjdk.org> wrote:

>> This is a backport of 8230305 (Cgroups v2: Container awareness) for JDK8u, working from the jdk11u-dev backport as part of an effort to backport cgroups v2 to jdk8u-dev.
>> 
>> The patch does not apply clean after unshuffling and path fixing:
>> 
>>  * mostly copyright line issues
>>  * different package name for jdk.test.lib.process.OutputAnalyzer
>>  * Most of hotspot/src/os/linux/vm/cgroupSubsystem_linux.cpp failed due to context changes, manually resolved (mostly deletions)
>> 
>> A few other changes were made
>> 
>>  *  Rework use of newer logging API (log_debug etc) to use guarded tty->print_cr
>>  *  replace os::strerror with the libc strerror (which is thread unsafe, but that should be addressed by backporting 8148425 separately)
>> 
>> patch touched hotspot/test/runtime/containers/docker/TestCPUAwareness.java which passes afterwards; I'm running further tests now (and awaiting GitHub CI doing the same)
>
> Jonathan Dowland has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
> 
>  - substitute os::strerror for strerror
>    
>    os::strerror was introduced in 8148425 (strerror() function is not
>    thread-safe), which is probably out of scope for backporting (at least
>    as part of this cgroups v2 effort). Replace uses of os::strerror with
>    (thread unsafe) strerror, in common with the rest of JDK8u.
>  - Rework use of newer logging API to tty->print_cr
>    
>    Remove includes of log.hpp that doesn't exist in jdk8u
>    
>    log_(debug|trace) to tty->print_cr guarded by PrintContainerInfo
>  - 8230305: Cgroups v2: Container awareness
>    
>    Implement Cgroups v2 container awareness in hotspot
>    
>    Reviewed-by: bobv, dholmes

This seems promising, thanks. Please use explicit parenthesis for `PrintContainerInfo` log lines throughout. Also with a space before and after the `if`. We need to account for https://bugs.openjdk.org/browse/JDK-8227006 (which is in 8u) and https://bugs.openjdk.org/browse/JDK-8232207 (not currently in 8u), though. This patch would include both.

hotspot/src/os/linux/vm/cgroupSubsystem_linux.cpp line 112:

> 110:     // one or more controllers disabled, disable container support
> 111:     if(PrintContainerInfo)
> 112:       tty->print_cr("One or more required controllers disabled at kernel level.");

Style: Please use:


if (PrintContainerInfo) {
    tty-print_cr("...");
}


This repeats many times.

hotspot/src/os/linux/vm/cgroupSubsystem_linux.cpp line 367:

> 365:   // [See 8227006].
> 366:   CachingCgroupController* contrl = cpu_controller();
> 367:   CachedMetric* cpu_limit = contrl->metrics_cache();

For the processor counts we do want the caching so as to not regress JDK-8227006. For memory on the other hand we should probably remove it in the 8u backport.

hotspot/src/os/linux/vm/cgroupSubsystem_linux.cpp line 431:

> 429:   if (!memory_limit->should_check_metric()) {
> 430:     return memory_limit->value();
> 431:   }

This is actually code added with [JDK-8232207](https://bugs.openjdk.org/browse/JDK-8232207) which shouldn't affect JDK 8. So I'm not sure about this. Add it to 8u anyway (and have less divergence) or remove and deal with the code difference.

Whatever the way, we need to make this clear by: a) adding the issue using `/issue add` or b) restructuring this code to account for it. I'm slightly in favor of b).

hotspot/src/os/linux/vm/cgroupSubsystem_linux.hpp line 52:

> 50:  * [2] https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_ContainerDefinition.html
> 51:  * [3] https://github.com/apache/mesos/blob/3478e344fb77d931f6122980c6e94cd3913c441d/src/docker/docker.cpp#L648
> 52:  *     https://github.com/apache/mesos/blob/3478e344fb77d931f6122980c6e94cd3913c441d/src/slave/containerizer/mesos/isolators/cgroups/constants.hpp#L30

This is part of `JDK-8216366` which isn't yet in 8u. Please add the issue to the commit message by doing:

hotspot/src/os/linux/vm/cgroupV2Subsystem_linux.cpp line 42:

> 40:   // Convert default value of 100 to no shares setup
> 41:   if (shares == 100) {
> 42:     tty->print_cr("CPU Shares is: %d", -1);

This line should be guarded with `if (PrintContainerInfo) { ... }`

hotspot/src/os/linux/vm/cgroupV2Subsystem_linux.cpp line 62:

> 60:   if ( x <= PER_CPU_SHARES ) {
> 61:      // will always map to 1 CPU
> 62:      tty->print_cr("CPU Shares is: %d", x);

Same here: Missing guard of `PrintContainerInfo`.

hotspot/src/os/linux/vm/osContainer_linux.hpp line 69:

> 67: 
> 68: inline bool OSContainer::is_containerized() {
> 69:   assert(_is_initialized, "OSContainer not initialized");

This hunk got removed with `JDK-8229202` which is not yet part of 8u. Please add this issue by:

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

Changes requested by sgehwolf (Reviewer).

PR: https://git.openjdk.org/jdk8u-dev/pull/127