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

[Severin Gehwolf]

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