[PING] RFR: 8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy
Bob Vandette
bob.vandette at oracle.com
Fri Jan 3 20:37:00 UTC 2020
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 <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/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 <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 <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.
Bob.
> On Dec 20, 2019, at 9:50 AM, Severin Gehwolf <sgehwolf at 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 <sgehwolf at 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
>
More information about the core-libs-dev
mailing list