> On Dec 6, 2019, at 2:49 AM, David Holmes <david.hol...@oracle.com> wrote: > > Hi Daniil, > > I'm not familiar with all the details of the various API's involved here so > just a few general comments in places. I do have one major issue flagged > below. > > --- > > src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c > > ! static int initialized=1; > > Am I reading this right that the code currently fails to actually do the > initialization because of this ??? > > Style nit: if(perfInit() > > space after "if" > > --- > > src/jdk.management/share/classes/com/sun/management/OperatingSystemMXBean.java > > The changes to allow for a return of -1 are somewhat more extensive than we > have previously discussed. These methods previously were (per the spec) > guaranteed to return some (assumably) meaningful value but now they are > effectively allowed to fail by returning -1. No existing code is expecting to > have to handle a return of -1 so I see this as a significant compatibility > issue. Surely there must always be some information available from the > operating environment? I see from the impl file: > > // the host data, value 0 indicates that something went wrong while the > metric was read and > // in this case we return "information unavailable" code -1. > > I don't agree with this. If the container metrics are messed up somehow we > should either fallback to the host value or else abort with some kind of > exception. Returning -1 is not an option here IMO.
I agree with David on the compatibility concern. I originally thought that -1 was already a specified return for all of these methods. Since the 0 return failure from the Metrics API should only occur if one of the cgroup subsystems is not enabled while others are, I’d suggest we keep Daniil’s original logic to fall back to the host value since a disabled subsystem is equivalent to no limits. Bob. > > --- > > src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java > src/jdk.management/windows/classes/com/sun/management/internal/OperatingSystemImpl.java > > Can you please rename the legacy methods so that, for example, > getTotalMemorySize() calls getTotalMemorySize0() rather than > getTotalPhysicalMemorySize0(). That way we relegate the legacy names to the > interface only. > > --- > > test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java > > System.out.println(String.format(...) > > Why not simply > > System.out.printf(..) > > ? > > --- > > Thanks, > David > ----- > > On 6/12/2019 11:03 am, Daniil Titov wrote: >> Hi Mandy and Bob, >> Thank you for your comments. Please review a new version of the fix [1] that >> makes >> OperatingSystemImpl methods return -1 if one of the metric has value 0. >> As Mandy recommended I also updated the Javadoc for OperatingSystemMXBean >> indicating that methods could return -1 if the information is not available. >> There were no changes in CSR [3] yet, I plan to proceed with them after the >> fix is >> reviewed. >>> In >>> http://cr.openjdk.java.net/~dtitov/8226575/webrev.03/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java.sdiff.html >>> Shouldn’t you keep the IOException catch clauses in case the file is not >>> found? >> There is no need in keeping IOException catch in these 2 places where it >> used to be (getLongValueMatchingLine and getLongEntry methods). >> As I understand IOException catch was required only because File.lines() and >> File. readAllLines() can throw IOException. >> Now these calls are performed inside >> AccessController.doPrivileged(PrivilegedExceptionAction) that wraps >> all checked exceptions in PrivilegedActionException that we are catching >> now instead of IOException. >> Here is the sampe of the stacktrace: >> java.security.PrivilegedActionException: java.io.FileNotFoundException >> at >> java.base/java.security.AccessController.doPrivileged(AccessController.java:558) >> at >> java.base/jdk.internal.platform.cgroupv1.SubSystem.getLongValueMatchingLine(SubSystem.java:113) >> at >> java.base/jdk.internal.platform.cgroupv1.Metrics.getMemoryLimit(Metrics.java:390) >> at >> jdk.management/com.sun.management.internal.OperatingSystemImpl.getTotalMemorySize(OperatingSystemImpl.java:109) >> at CheckOperatingSystemMXBean.main(CheckOperatingSystemMXBean.java:36) >> Caused by: java.io.FileNotFoundException >> at >> java.base/jdk.internal.platform.cgroupv1.SubSystem.lambda$getLongValueMatchingLine$1(SubSystem.java:116) >> at >> java.base/java.security.AccessController.doPrivileged(AccessController.java:554) >> In getStringValue method the whole code block is now executed inside >> AccessController.doPrivileged() so we still need either catch >> IOException inside this code block or convert this block to >> PrivilegedExceptionAction and then put AccessController.doPrivileged >> call inside new try/catch Block to catch PrivilegedExceptionAction. The >> former approach looked more preferable. >> Testing: Mach5 tier1-tier3 and open/test/hotspot/jtreg/containers/docker >> tests passed. Tier4-tier6 tests are still running. >> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8226575/webrev.04/ >> [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8226575 >> [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8228428 >> Thanks, >> Daniil >> On 12/5/19, 12:59 PM, "Mandy Chung" <mandy.ch...@oracle.com> wrote: >> On 12/5/19 12:50 PM, Bob Vandette wrote: >> > >> >>>> It may worth considering adding Metrics::getSwapLimit and >> >>>> Metrics::getSwapUsage and move the computation to the >> implementation of >> >>>> Metrics. Bob may have an opinion. >> >> There was no any new input regarding this so I decided to leave it >> unchanged. >> > Sorry, I didn’t respond to this. Since the calculation required for >> getFreeSwapSpaceSize requires retries >> > due to the access of multiple changing values, I think it’s best to >> leave things as they are so the caller of >> > these methods understands the limitations of the API. >> OK with me. >> > Also, the fact that swap size metrics include memory sizes is fully >> documented in both the cgroup and docker >> > online documentation so it’s probably best to be consistent. >> > >> >>>> Also it seems correct for the memory related methods to check if >> >>>> (containerMetrics != null && containerMetrics.getMemoryLimit() >= >> 0). >> >>>> BTW what does it mean if limit == 0? >> >> Per Docker docs the minimum allowed value for memory limit (--memory >> option) is 4 megabytes. >> >> And if memory limit is unset the return value is -1. Thus, in my >> understanding the value 0 is only possible >> >> if something went wrong while retrieving this metric. >> > That is true but shouldn’t you return -1 in that case? >> > >> > I originally thought it was ok to fall back to the host data for 0 >> values but I think its better to return unavailable (-1) >> > I think you might want to change all >= 0 to > 0 and return -1 if any >> of the values are 0. This would be more consistent. >> +1 >> The javadoc should be changed and returns -1 when it's unavailable >> and >> the CSR should also be updated to reflect this. I'm sure Joe can >> re-approve the CSR quickly when the fix is reviewed and approved. >> > You should only fall back to the original logic (host values) if >> container values are set to unlimited. >> > >> +1 >> Mandy >>
