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

[Bob Vandette]

> On Jun 18, 2020, at 2:48 PM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
> 
> On Thu, 2020-06-18 at 14:21 -0400, Bob Vandette wrote:
>> 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
> 
> This version omits the check for the precise error message. Would this
> be OK?

Yes, that works.  

Bob.

> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8244500/04/webrev
> 
> Thanks,
> Severin
> 
> 
> 
>> 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
>>>>>>> 
>