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

[Severin Gehwolf]

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?
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
> > > > > >