Hi Mandy,
I think in my previous reply I missed to answer one of the questions from your
email.
>> getFreeSwapSpaceSize retry for a few times. What special about this method
>> but not others like getFreeMemorySize?
The specific of method getFreeSwapSpaceSize is that MemoryAndSwapUsage and
MemoryUsage metrics it reads are related
( MemoryAndSwapUsage includes MemoryUsage) and they are not constant. Since
these metrics are not read atomically it could be
that they change their values between these 2 reads. On the contrary, some
other metrics, such as MemoryLimit, are constant. They are set
when the container starts and are supposed to return the same value over the
whole time the JVM runs. The other methods don't use
more than one such nonconstant metric, so the only place where this potential
issue with not atomic reads could happen is getFreeSwapSpaceSize method.
Best regards,
Daniil
On 12/3/19, 7:34 PM, "Daniil Titov" <daniil.x.ti...@oracle.com> wrote:
Hi Mandy,
Thank you for your comments, please find my answers below.
>> src/java.base/linux/classes/jdk/internal/platform/cgroupv1/Metrics.java
>> this should wrap the security-sensitive operations with doPrivileged.
jdk.management is trusted and it has all permissions.
I will include this change in the next webrev, thank you.
>> src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c
>> Formatting nit: line 346-355: JDK native source uses 4-space
identation convention. A space is missing between "if" and "(".
I will correct this, thanks.
>>Under what circumstance that limit or memLimit is < 0?
The memory limit metrics is not available if JVM runs on Linux host ( not
in a docker container) or if a docker container was started without
specifying a memory limit ( without '--memory=' Docker option) . In
latter there is no limit on how much memory the container can use and
it can use as much memory as the host's OS allows.
>> Is it worth specifying this case?
I believe yes, since it covers the cases when JVM runs on a Linux host or
a docker container was started without memory limitation.
>> It fallbacks to return the system's total swap space size - this is not
really what it should report.
For the case when JVM runs on a Linux host it is exactly what we want. The
only problematic case is if JVM runs inside a docker container without a memory
limit set.
However, I am not sure how we could differentiate these 2 cases.
>> Similarly, getFreeMemorySize and getTotalMemorySize and getCpuLoad.
For getTotalMemorySize I think we are good here. If limit is not set then
all memory the host's OS have is available.
For getFreeMemorySize the problematic case is if is the memory limit is set
but the memory usage for some reason is not available
(containerMetrics.getMemoryUsage() returns 0).
Probably in this case we should just return -1 as currently
getFreePhysicalMemorySize0() does if it cannot retrieve a valid result.
For getCpuLoad() the problematic case if CPU quotas are active but
CpuPeriod, CpuNumPeriods , or getCpuUsage are unavailable or if a valid CPU
load for some CPU was
not retrieved, or if all retrieved CPU load values happen to be zeros.
Probably we should just return -1 in these cases rather then falling back to
getSystemCpuLoad0()
>>src/jdk.management/windows/classes/com/sun/management/internal/OperatingSystemImpl.java
>> There is no strong need to make the deprecated methods as default
methods. If they were default methods, they only need to be implemented once
as opposed to in all OS-specific implementations.
I could make these methods defaults if you feel it is a better approach
here.
>>CheckOperatingSystemMXBean.java
>> System.out.println(String.format(...)) can simply be replaced with
System.out.format.
I will include this change in the next webrev, thank you!
Best regards,
Daniil
From: Mandy Chung <mandy.ch...@oracle.com>
Date: Tuesday, December 3, 2019 at 4:10 PM
To: Daniil Titov <daniil.x.ti...@oracle.com>
Cc: OpenJDK Serviceability <serviceability-dev@openjdk.java.net>,
"jmx-...@openjdk.java.net" <jmx-...@openjdk.java.net>, Bob Vandette
<bob.vande...@oracle.com>
Subject: Re: RFR: 8226575: OperatingSystemMXBean should be made container
aware
On 12/3/19 11:42 AM, Daniil Titov wrote:
Please review the change that makes OperatingSystemMXBean methods return
container specific informationrather than the host based data.
The webrev also takes into account the case when
java.security.AccessControlException exception is thrown
during the initialization of the container subsystem ( e.g. when
java.policy doesn’t grant "read" access to "/proc/self/mountinfo" file).
Instead of failing to access /proc/self/mountinfo, I expect this to wrap
the call with doPrivileged so that it can report the metrics independent of the
security policy. The jdk default security policy should grant proper
permission to do so.
CSR for the spec changes [3] is approved.
Testing: Mach5 tiers1, tiers2, tiers3, tier4, tier5 (including
open/test/hotspot/jtreg/containers/docker), and tier6 tests passed .
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8226575/webrev.02/
[2] Jira issue :https://bugs.openjdk.java.net/browse/JDK-8226575
[3] CSR https://bugs.openjdk.java.net/browse/JDK-8228428
src/java.base/linux/classes/jdk/internal/platform/cgroupv1/Metrics.java
this should wrap the security-sensitive operations with doPrivileged.
jdk.management is trusted and it has all permissions.
src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c
Formatting nit: line 346-355: JDK native source uses 4-space
identation convention. A space is missing between "if" and "(".
src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java
59 if (limit >= 0 && memLimit >= 0) {
60 return limit - memLimit;
61 }
Under what circumstance that limit or memLimit is < 0? It fallbacks to
return the system's total swap space size - this is not really what it should
report. Is it worth specifying this case? Similarly, getFreeMemorySize and
getTotalMemorySize and getCpuLoad.
getFreeSwapSpaceSize retry for a few times. What special about this method
but not others like getFreeMemorySize?
src/jdk.management/windows/classes/com/sun/management/internal/OperatingSystemImpl.java
There is no strong need to make the deprecated methods as default
methods. If they were default methods, they only need to be implemented once
as opposed to in all OS-specific implementations.
CheckOperatingSystemMXBean.java
System.out.println(String.format(...)) can simply be replaced with
System.out.format.
Mandy
