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

[Bob Vandette]

I fired up my cgroupv2 configured system and confirmed that getMemoryAndSwapLimit will indeed return -1
if the kernel does not have swap accounting enabled so I’m ok with your change.

The memory.swap.* files are not present in the /sys/fs/cgroup file system which causes -1 to be returned.

I agree that the fix for JDK-8236617 is no longer needed.

Sorry for the delay,
Bob.


> On Jun 17, 2020, at 10:29 AM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
> 
> On Wed, 2020-06-17 at 09:09 -0400, Bob Vandette wrote:
>> Should we just accept the kernel error message rather than add all the additional value checking?
>> 
>>     "out.shouldContain("Your kernel does not support swap limit capabilities”);
> 
> I'd prefer to keep the other checks as well. The bug explains why that
> is and if we see a config which fails this as well, something is wrong.
> Hopefully that makes some sense.
> 
>> Why isn’t the try block around the check for .shouldMatch("OperatingSystemMXBean\\.getFreeSwapSpaceSize: [1-9][0-9]+”);
> 
> Why should it? For most set ups this catch block is never reached. The
> only setup we are aware of which reaches this is the system Matthias
> runs tests on. I've never heard of a system where this fails as well.
> 
>> Why doesn’t this call fail as well??
> 
> Because the system values are being used for
> OperatingSystemMXBean.getTotalSwapSpaceSize() when:
> Metrics.getMemoryLimit() > 0 && Metrics.getMemoryAndSwapLimit() == -1
> 
>> Have you tried this test on a system with cgrouopv1 and v2 that doesn’t have swap enabled?
> 
> I don't think so. I don't have access to such a system.
> 
> How do you mean, though? A system which has swap disabled on the system
> level or a container without swap? I believe the former is exactly the
> case of Matthias' system. But I don't know. The latter should also be
> covered via a test Jie added a while back.
> 
>> I’m concerned that we’ll be back to fixing this when some other configuration combination fails.
> 
> I understand but find it unlikely. Those are systems which are going to
> be hard to detect correctly and do-the-right-thing. As mentioned in the
> bug there is no way to distinguish between unlimited swap + a memory
> limit from kernel-doesn't-support-swap-limits + a memory limit.
> 
> Note that https://bugs.openjdk.java.net/browse/JDK-8236617 "fixed" this
> for the old implementation. Unfortunately, it doesn't seem like it was
> investigated why a 0 limit was being returned. That was when the old
> implementation was still in place (prior cgroups v2 support). In the
> old impl, when files were missing 0 was returned for some cases. In
> fact, the fix of JDK-8236617 with the old impl would have wrongly
> returned 0 for getTotalSwapSpaceSize() for a system which got
> configured with unlimited swap size (--memory-swap=-1). In the new
> impl, this returns physical swap size which seems better.
> 
> On that note, the code of JDK-8236617 seems unreachable now.
> 
> Thanks,
> Severin
> 
> 
>> Bob.
>> 
>> 
>>> On Jun 17, 2020, at 5:13 AM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
>>> 
>>> On Fri, 2020-06-05 at 16:07 +0200, Severin Gehwolf wrote:
>>>> On Fri, 2020-06-05 at 10:30 +0000, Baesken, Matthias wrote:
>>>>>> Here would be my suggestion
>>>>>> (hopefully this one isn't too restrictive again):
>>>>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8244500/03/webrev/
>>>>> 
>>>>> Hi Severin, this worked nicely .
>>>>> Thanks for providing this .
>>>> 
>>>> Great. If somebody else is willing to OK this, I'll push it.
>>> 
>>> Could I have a second review of this, please? I believe it's the best
>>> we can do for this test failure Matthias is seeing.
>>> 
>>> Thanks,
>>> Severin
>>> 
>