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

[Bob Vandette]

Actually, I still think we might have an issue with your change on cgroupv2 systems when using podman.
I just realized that we need the exact error string to match and I’m not seeing the same one.  The error
message you are matching may be coming from docker.  There are a few other issues prohibiting us from
fully testing on podman but it’s definitely a goal to get there.  I wonder if you should skip the string check?
I don’t even think we can guarantee this error message would occur on all Linux systems with podman since
this one has the OCI string in it (Oracle Cloud Infrastructure).

# podman run  --memory 128m --memory-swap 300m -it ubuntu bash
Error: writing file `memory.swap.max`: Permission denied: OCI runtime permission denied error

Bob.

> On Jun 18, 2020, at 12:57 PM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
> 
> On Thu, 2020-06-18 at 12:09 -0400, Bob Vandette wrote:
>> 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.
> 
> Thank you Bob!
> 
>> I agree that the fix for JDK-8236617 is no longer needed.
> 
> OK. I've filed another bug to track this:
> https://bugs.openjdk.java.net/browse/JDK-8247863
> 
>> Sorry for the delay,
> 
> No worries at all.
> 
> Thanks,
> Severin
> 
>> 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
>>>>> 
>