Severin, The changes look ok to me. I assume you’ve run the container tests on a cgroupv2 and cgroupv1 enabled system, right?
You’re going to have to find a “R” reviewer. What’s the status of the hotspot cgroupv2 changes. Have these been reviewed? Bob. > On Jan 9, 2020, at 2:51 PM, Severin Gehwolf <sgehw...@redhat.com> wrote: > > Hi, > > On Fri, 2020-01-03 at 15:37 -0500, Bob Vandette wrote: >> Here are a few comments from scanning the webrev. >> >> >> It looks like you can remove line 151 >> >> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/06-incremental/webrev/src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java.sdiff.html >> >> 151 int[] ints = new int[0]; >> >> ————— >> >> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/06-incremental/webrev/src/java.base/share/classes/jdk/internal/platform/Metrics.java.sdiff.html >> >> There are several comments stating that -2 == unlimited. This is not >> the case. >> >> @return count of elapsed periods or -2 if the quota is unlimited. >> >> ————— >> >> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/06/webrev/test/jdk/jdk/internal/platform/docker/TestDockerMemoryMetrics.java.sdiff.html >> >> Shouldn’t you just check for cgroupv2 before trying to run the >> testKernelMemoryLimit and testOomKillFlag tests? >> >> ————— >> >> There are a few places where getPerCpuUsage is returning “new >> long[0]” but you changed the interface to expect null >> for not supported. >> >> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/06/webrev/src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java.html >> >> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/06/webrev/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1Subsystem.java.html >> >> You probably need to check all users of the APIs which used to return >> array[0] which now return null to make sure you >> don’t get null pointer exceptions. >> >> One example is testCpuConsumption. >> >> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/06/webrev/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV1.java.html >> >> Also, >> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/06/webrev/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV2.java.html:167 >> >> >> You’ll also have to update copyrights to 2020. > > Thanks for the review! Should all be fixed now. Updated webrev: > > incremental: > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/07/incremental/webrev/ > full: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/07/webrev/ > > Note: I'll go through touched files and update copyright dates to 2020. > Not all have been updated in the full webrev. It'll be done though. > > Thanks, > Severin > >> Bob. >> >> >>> On Dec 20, 2019, at 9:50 AM, Severin Gehwolf <sgehw...@redhat.com> >>> wrote: >>> >>> Hi Bob, >>> >>> Sorry for the delay to get this updated. >>> >>> Changes done in this latest webrev: >>> >>> 1. Rebased on top of 8226575: OperatingSystemMXBean should be >>> made >>> container aware. File read ops now use privileged code. >>> 2. Warning for mixed cgroup controllers and returning null for >>> metrics. >>> 3. Added implementations for getBlkIOServiceCount() and >>> getBlkIOServiced() using sum of rios/wios and rbytes/wbytes >>> across >>> devices. Added impl for getTcpMemoryUsage() >>> 4. Returning -2 for not supported (if the metric would return >>> long). >>> boolean metrics now return Boolean and null if not supported. >>> Same >>> for array return types. null if not supported for symmetry. >>> 5. -XshowSettings:system output has been updated to return "N/A" >>> for >>> when a metric is not available. E.g. "Kernel OOM killer >>> enabled" >>> Boolean. >>> 6. Tests have been updated to reflect this. >>> >>> webrev: >>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/06/webrev/ >>> incremental webrev: >>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/06-incremental/webrev/ >>> >>> Testing: Docker tests and podman testing on cgroupsv2. I'll run it >>> through jdk/submit as well. >>> >>> Hopefully this can get pushed with 8230305 early on in the jdk 15 >>> cycle >>> :) >>> >>> A couple of more inline comments below. >>> >>> On Mon, 2019-12-02 at 12:13 -0500, Bob Vandette wrote: >>>> Sorry for the delay in responding. See comments below: >>>> >>>>> On Nov 29, 2019, at 4:05 AM, Severin Gehwolf < >>>>> sgehw...@redhat.com> wrote: >>>>> >>>>> On Fri, 2019-11-15 at 17:51 +0100, Severin Gehwolf wrote: >>>>>> Hi, >>>>>> >>>>>> Could I please get reviews of these core-libs, Linux-only, >>>>>> changes to >>>>>> the Metrics subsystem? This patch implements cgroupv2 support >>>>>> for >>>>>> Metrics which are currently cgroupv1-only. Fedora 31 switched >>>>>> to >>>>>> cgroupv2 by default so it's time to get OpenJDK recognize it. >>>>>> >>>>>> Note that a couple of metrics are no longer supported with >>>>>> cgroupv2. >>>>>> Most notably (not an exhaustive list, though): >>>>>> >>>>>> Metrics.getKernel*() family of methods. >>>>>> Metrics.getTcp*() family of methods. >>>>>> Metrics.getBlkIO*() family of methods. >>>>>> Metrics.isMemoryOOMKillEnabled() >>>>>> >>>>>> A couple of open questions with regards to that: >>>>>> >>>>>> 1) >>>>>> Most API docs of Metrics make no distiction between >>>>>> "unlimited" and >>>>>> "not supported", both returning -1 for longs, for example. >>>>>> This is a >>>>>> problem, because output of "java -XshowSettings:system >>>>>> -version" will >>>>>> not distinguish between unlimited and not supported metrics. >>>>>> Would it >>>>>> be acceptable to change the API to distinguish those cases so >>>>>> that >>>>>> LauncherHelper could display them appropriately? >>>>>> >>>>>> 2) >>>>>> How should we deal with "not supported" for booleans/arrays, >>>>>> etc.? >>>>>> Would it make sense to return record objects from the Metrics >>>>>> API so >>>>>> that this could be dealt with? E.g. >>>>>> >>>>>> Metrics m = ... >>>>>> MetricResult<int[]> result = m.getCpuSetCpus(); >>>>>> switch(result.getType()) { >>>>>> case NOT_SUPPORTED: /* do something */; break; >>>>>> case SUPPORTED: int[] val = result.get(); break; >>>>>> ... >>>>>> } >>>>>> >>>>>> I'm bringing this up, because the proposed patch doesn't deal >>>>>> with the >>>>>> above open questions as of yet. With that being said, it's >>>>>> mostly >>>>>> identical to the proposed hotspot changes in [1]. >>>> >>>> For issue 1 and 2 … >>>> >>>> I wonder if we should change the API to throw >>>> UnsupportedOperationException for those methods >>>> that are not available. This exception is used quite a lot in >>>> the java/nio and java/net packages >>>> for dealing with functionality not available on a host platform. >>>> >>>> As an alternate suggestion, we already return -2 for "not >>>> supported" for most APIs in the hotspot >>>> implementation. We could use this for non boolean values in the >>>> Metrics API. For boolean >>>> values, we could change the API to return “Boolean”. A null >>>> return would signify not >>>> implemented. >>> >>> This alternative has been implemented in the latest webrev. >>> LauncherHelper has been updated to print "N/A" if some property >>> being >>> printed is not supported. Example output for cgroupsv2: >>> >>> $ ./bin/java -XshowSettings:system -version >>> Operating System Metrics: >>> Provider: cgroupv2 >>> Effective CPU Count: 4 >>> CPU Period: 100000us >>> CPU Quota: -1 >>> CPU Shares: -1 >>> List of Processors: N/A >>> List of Effective Processors: N/A >>> List of Memory Nodes: N/A >>> List of Available Memory Nodes: N/A >>> CPUSet Memory Pressure Enabled: N/A >>> Memory Limit: Unlimited >>> Memory Soft Limit: Unlimited >>> Memory & Swap Limit: Unlimited >>> Kernel Memory Limit: N/A >>> TCP Memory Limit: N/A >>> Out Of Memory Killer Enabled: N/A >>> >>> openjdk version "15-internal" 2020-09-15 >>> OpenJDK Runtime Environment (build 15-internal+0- >>> adhoc.sgehwolf.openjdk-head-2) >>> OpenJDK 64-Bit Server VM (build 15-internal+0- >>> adhoc.sgehwolf.openjdk-head-2, mixed mode, sharing) >>> >>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8231111 >>>>>> webrev: >>>>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/04/webrev/ >>>>>> >>>> >>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/04/webrev/src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java.html >>>> >>>> Should we check to make sure that there are no mixed cgroupv1 and >>>> cgroupv2 mounted subsystems since >>>> you are not supporting mixing. >>> >>> Done. null is returned for metrics and a warning printed to stderr. >>> >>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/04/webrev/src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java.html >>>> >>>> It looks looks like there may be alternate ways of reporting some >>>> of the metrics that you have classified as RETVAL_NOT_SUPPORTED. >>>> >>>> BlkIOServicedCount (io.stat) >>>> KernelMemory (memory.stat) >>>> TcpMemory (memory.stat) >>> >>> The latest webrev has getBlkIOService* and getTcpMemoryUsage impls. >>> I've left out kernel memory metrics as it wasn't clear what this >>> would >>> be. We can add that in a later patch. The size of this patch is >>> already >>> rather big. >>> >>>> You could try the same trick for getMemoryAndSwapMaxUsage which >>>> keeps track of the highest returned value. >>> >>> We've decided to not do this. getMemoryAndSwapMaxUsage and >>> getMemoryMaxUsage is being returned as not supported in cgroups v2. >>> >>> Thanks, >>> Severin >>> >>>> for the benefit of other reviewers, you should provide links to >>>> the cgroupv1 and v2 docs. >>>> >>>> https://www.kernel.org/doc/Documentation/cgroup-v2.txt >>>> >>>>>> Testing: jdk/submit and platform docker tests on Linux x86_64 >>>>>> (with >>>>>> hybrid hierarchy, docker/podman) and on Linux x86_64 with >>>>>> unified >>>>>> hierarchy (podman only). >>>>>> >>>>>> Thoughts? Suggestions? >>>> >>>> Do you think we should check the docker version being used for >>>> the tests to make sure it >>>> supports cgroupv2? I assume a fairly recent version is required >>>> to work with cgroupv2. >>>> >>>> Bob. >>>> >>>> >>>> >>>>> Ping? >>>>> >>>>>> Thanks, >>>>>> Severin >>>>>> >>>>>> [1] >>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2019-November/039909.html >
