RFR [XS]: 8244500: jtreg test error in test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java

[jiefu(傅杰)]

Hi David,

This bug should be introduced by JDK-8242480.
I'm sorry for that.

And I think the logic in src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java should be fixed.

In my fix for JDK-8242480, I had ignored the case for kernels which don't support "--memory-swap 150M".
In that case, containerMetrics.getMemoryAndSwapLimit()[1] will return -1.

I didn't notice this bug because all our kernels support --memory-swap.
Fortunately, Matthias found this bug and will fix it.

Thanks a lot.
Best regards,
Jie

[1] http://hg.openjdk.java.net/jdk/jdk/file/a95911422005/src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java#l73

On 2020/5/18, 1:48 PM, "David Holmes" <david.holmes at oracle.com> wrote:

    Hi Matthias,
    
    On 16/05/2020 12:59 am, Baesken, Matthias wrote:
    > Hello David ,  I get  the warning  always on this linux machine (I think it depends on machine setup and/or kernel   version).
    > 
    >> Surely it must be specified to return 0  or some such sentinel in such a case?
    > 
    > The doc just says this :
    > 
    > 62    /**
    > 63     * Returns the total amount of swap space in bytes.
    > 64     *
    > 65     * @return the total amount of swap space in bytes.
    > 66     */
    > 67    public long getTotalSwapSpaceSize();
    > 
    > ( however  it returned previously 0 before the latest changes in the container support code came in ).
    
    That sounds like a bug was introduced to me then.
    
    > Regarding  the  getFreeSwapSpaceSize  issue  ,   Jie   provided a change to  src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java
    >   Because of this   I could remove  the try-construct .
    > 
    > New  webrev :
    > http://cr.openjdk.java.net/~mbaesken/webrevs/8244500.3/
    
    I'm quite confused by your changes. Your first version simply adjusted 
    the test to check for the warning. But now you are also changing the 
    main logic. I can't assess the validity of what you are doing sorry.
    
    I've cc'd Severin and Bob as I think they need to evaluate what the real 
    problem is here and when it was introduced.
    
    David
    ----
    
    > Best regards, Matthias
    > 
    > 
    > 
    > -----Original Message-----
    > From: David Holmes <david.holmes at oracle.com>
    > Sent: Donnerstag, 14. Mai 2020 07:02
    > To: Baesken, Matthias <matthias.baesken at sap.com>; 'hotspot-dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>
    > Cc: jiefu(傅杰) <jiefu at tencent.com>
    > Subject: Re: RFR [XS]: 8244500: jtreg test error in test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java
    > 
    > On 14/05/2020 12:28 am, Baesken, Matthias wrote:
    >> Hello, here is a new webrev removing the comment :
    >>
    >> http://cr.openjdk.java.net/~mbaesken/webrevs/8244500.1/
    >>
    >> It removes the comment , and it also  handles  0  for getFreeSwapSpaceSize
    >> (  this can occur  after   JDK-8242480 : Negative value may be returned by getFreeSwapSpaceSize() in the docker   was pushed  recently ).
    > 
    > Sorry Matthias but I still see this partly as a bug in the underlying
    > container support code. If you get the warning then what does
    > getTotalSwapSpaceSize() return? Surely it must be specified to return 0
    > or some such sentinel in such a case?
    > 
    > Other comments:
    > 
    > !         try {
    > !
    > out.shouldMatch("OperatingSystemMXBean\\.getFreeSwapSpaceSize:
    > [1-9][0-9]+");
    > !         } catch(RuntimeException ex) { // allow 0, see 8242480
    > !
    > out.shouldContain("OperatingSystemMXBean.getFreeSwapSpaceSize: 0");
    > !         }
    > 
    > can't you just change the original regex to match >= 0?
    > 
    > Style nit: avoid historical comments like "see 8242480" and "since
    > 8231111 ...".
    > 
    > Thanks,
    > David
    > 
    >> Best regards, Matthias
    >>
    >> -----Original Message-----
    >> From: Baesken, Matthias
    >> Sent: Mittwoch, 13. Mai 2020 10:11
    >> To: 'David Holmes' <david.holmes at oracle.com>; 'hotspot-dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>
    >> Subject: RE: RFR [XS]: 8244500: jtreg test error in test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java
    >>
    >>
    >>> But according to the comment immediately before your changes:
    >>>
    >>> // in case of warnings like : "Your kernel does not support swap limit
    >>> capabilities or the cgroup is not mounted. Memory limited without swap."
    >>> // the getTotalSwapSpaceSize does not return the expected result, but 0
    >>>
    >>> we should be returning zero. So this seems to be a bug in the
    >>> implementation not in the test.
    >>
    >>    
    >> Hi David , I think  in case of  "Your kernel does not support swap limit  capabilities   ... "    we just do not get the values
    >>    that are normally expected.
    >> Previously we indeed got "0" .
    >> Probably the comment should be deleted .
    >>
    >> Best regards, Matthias
    >>