[PING] RFR: 8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy

[Mandy Chung]

Hi Severin,

On 2/11/20 10:04 AM, Severin Gehwolf wrote:
> Updated webrev:
> Full: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/10/webrev/
> incremental: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/10/incremental/webrev/

Thanks for updating this.  This patch looks okay in general while I 
still think this is quite hard to tell whether the implementation 
matches the spec in the unavailable & unlimited cases.  It'd be good if 
this can be cleaned up in the future.    Thanks for the new test.

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java

92 logger.log(Level.WARNING, "Mixed cgroupv1 and cgroupv2 not supported. 
Metrics disabled.");

There isn't a clear guideline on what logging level to use.  I will opt 
for API not emit any message as this method returns null.  or this 
should throw an exception if this should be reported instead. So debug 
or trace level seems to be suitable for this message.

  130 return new CgroupMetrics(new CgroupV2Subsystem(unified));
  131         } else {
132 return new CgroupV1Metrics(CgroupV1Subsystem.getInstance());


CgroupV2Subsystem is instantiated with a constructor.  Do you expect 
multiple instances of CgroupV2Subsystem (as opposed to CgroupV1Subsystem 
is a singleton)?

src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1Subsystem.java

  391                 String match = "hierarchical_memory_limit";
392 retval = CgroupV1SubsystemController.getLongValueMatchingLine(memory,
  393                                                             "memory.stat",
394 match); Nit: the match variable is not needed. Similar pattern 
occurs in line 451-453. 
src/java.base/share/classes/jdk/internal/platform/MetricsCgroupV1.java

This is cgroup v1 specific. I still think it should be moved to 
linux/classes rather than share/classes.  The tests should only run on 
linux.

src/java.base/linux/classes/jdk/internal/platform/CgroupV1Metrics.java
    copyright header is missing

    You can define a private MetricsCgroupV1 subsystem field in this 
class to avoid the casting:

   15         return ((MetricsCgroupV1)subsystem).getMemoryMaxUsage();

So CgroupMetrics::subsystem field can stay as private if desire.

Side note:  CgroupV1Metrics is the implementation of MetricsCgroupV1.  
The naming is a bit strange.  CgroupV1Metrics sounds better for the 
interface and the implementation class could be CgroupV1MetricsImpl and 
it'd help a reader to understand their relationship.  I leave it for you 
to decide.

src/java.base/share/classes/sun/launcher/LauncherHelper.java

+ private static final long LONG_RETVAL_NOT_SUPPORTED = -2;


This is specific to printSystemMetrics.  I suggest to move this to 
printSystemMetrics as a local variable.  Then formatCpuVal and 
formatLimitString to take an additional "unavailable" parameter so that 
these methods will print "N/A" if limit == unavailable.

+ private static String formatBoolean(Boolean value, String prefix) { 
This is no longer needed. Please remove it.


test/lib/jdk/test/lib/containers/cgroup/MetricsTester.java
    The Oracle copyright is taken out and the copyright is also changed 
from GPL to GPL+CP.
    The Red Hat copyright can be added to the top of the file 
immediately before "DO NOT ALTER or REMOVE" line like [1].

[1] 
http://hg.openjdk.java.net/jdk/jdk/file/tip/test/hotspot/jtreg/compiler/onSpinWait/TestOnSpinWaitEnableDisable.java

test/jdk/jdk/internal/platform/docker/MetricsMemoryTester.java
     TestDockerMemoryMetrics requires docker.support.  I think any test 
depending on MetricsMemoryTester.java are filtered when running on 
platform other than linux.  Please verify that.

test/lib/jdk/test/lib/containers/cgroup/CPUSetsReader.java
   58         try(Stream<String> stream = Files.lines(Paths.get(path))) {

Nit: space between "try" and "(" is missing.

test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemController.java
    I expect the copyright header be placed at the top of the file 
before all imports.

thanks
Mandy