8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 … #342

hseigel · 2020-09-24T18:32:38Z

Please review this change to fix memory docker tests failures on some Linux kernels w/o cgroupv1 swap limit capabilities. The fix works by detecting that swap limit capabilities are not available and returning non-swap related information. For example, if memory and swap usage is requested, and swap limit capabilities are not available, then only memory usage is returned.

The fix was tested by running container tests on systems with and without swap limit capabilities. Additionally, the changes were regression tested by running tier1 and tier2 tests on Windows, Linux x64, and Mac OS, and running tier3 - tier5 tests on Linux x64.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 swap limit capabilities ⚠️ Title mismatch between PR and JBS.

Reviewers

Bob Vandette (@bobvandette - Committer) ⚠️ Review applies to 2746225
Severin Gehwolf (@jerboaa - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/342/head:pull/342
$ git checkout pull/342

…swap limit capabilities

hseigel · 2020-09-24T18:33:15Z

/label hotspot-runtime

bridgekeeper · 2020-09-24T18:34:26Z

👋 Welcome back hseigel! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2020-09-24T18:35:10Z

@hseigel
The hotspot-runtime label was successfully added.

…swap limit capabilities

hseigel · 2020-09-24T19:19:02Z

This fix was proposed by Bob V.

mlbridge · 2020-09-24T19:23:37Z

Webrevs

01: Full - Incremental (bcddedd)
00: Full (2746225)

mlbridge · 2020-09-24T19:56:03Z

Mailing list message from Bob Vandette on hotspot-runtime-dev:

Looks good to me.

Bob.

test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java

jerboaa · 2020-09-25T13:06:15Z

test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java

-            out.shouldMatch("OperatingSystemMXBean.getTotalSwapSpaceSize: [1-9][0-9]+");
-            out.shouldContain("Metrics.getMemoryLimit() == " + expectedMemory);
-            out.shouldContain("Metrics.getMemoryAndSwapLimit() == -1");
+            out.shouldMatch("OperatingSystemMXBean.getTotalSwapSpaceSize: -?([0-9]+)");


Is the optional - intentional? My understanding is that it should never be negative, should it not?

The optional - is intential in case UNLIMITED is return (-1).

Hmm, but the OperatingSystemMXBean impl falls back to returning the system (host) values if the container limits are unlimited. So the internal Metrics value of -1 for UNLIMITED should never be seen. Has this been actually seen in some tests somewhere?

jdk/src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java

Line 63 in a75edc2

if (limit >= 0 && memLimit >= 0) {

The -1 has not been seen. I removed the option '-' from the pattern. Please see the changes in the latest commit. Thanks!

I thought we were seeing -1 but perhaps this was a previous result before we decided to change Swap+Memory to report Memory instead of failure. I'm going to rerun the tests with and without swap enabled to verify.

All container tests continue to pass with and without swap enabled after removing the "-".

…swap limit capabilities

jerboaa

I'll also run this through testing and will report back.

jerboaa · 2020-09-25T15:47:46Z

test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java

-            out.shouldMatch("OperatingSystemMXBean.getTotalSwapSpaceSize: [1-9][0-9]+");
-            out.shouldContain("Metrics.getMemoryLimit() == " + expectedMemory);
-            out.shouldContain("Metrics.getMemoryAndSwapLimit() == -1");
+            out.shouldMatch("OperatingSystemMXBean.getTotalSwapSpaceSize: [0-9]+");


This should probably be:

out.shouldMatch("OperatingSystemMXBean\\.getTotalSwapSpaceSize: [0-9]+");

Heh, it was pre-existing :)

This isn't critical for this patch. As an pre-existing issue I won't insist on this. Either way is fine for me.

jerboaa

Good to go with or without the tiny fixup.

jerboaa · 2020-09-25T17:11:10Z

test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java

-            out.shouldMatch("OperatingSystemMXBean.getTotalSwapSpaceSize: [1-9][0-9]+");
-            out.shouldContain("Metrics.getMemoryLimit() == " + expectedMemory);
-            out.shouldContain("Metrics.getMemoryAndSwapLimit() == -1");
+            out.shouldMatch("OperatingSystemMXBean.getTotalSwapSpaceSize: [0-9]+");


This isn't critical for this patch. As an pre-existing issue I won't insist on this. Either way is fine for me.

openjdk · 2020-09-25T17:13:43Z

@hseigel This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for more details.

After integration, the commit message for the final commit will be:

8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 …

Reviewed-by: bobv, sgehwolf

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 14 new commits pushed to the master branch:

a75edc2: 8251188: Update LDAP tests not to use wildcard addresses
1f5a033: 8253555: Make ByteSize and WordSize typed scoped enums
f62eefc: 8253469: ARM32 Zero: replace usages of __sync_synchronize() with OrderAccess::fence
1b79326: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode
dc1ef58: 8253631: Remove unimplemented CompileBroker methods after JEP-165
27d0a70: 8253633: Remove unimplemented TieredThresholdPolicy::set_carry_if_neccessary
e12d94a: 8253594: Remove CollectedHeap::supports_tlab_allocation
cfa3f74: 8245527: LDAP Channel Binding support for Java GSS/Kerberos
37b7028: 8240556: Abort concurrent mark after effective eager reclamation of humongous objects
dcde95b: 8253586: C2: Clean up unused PhaseIterGVN::init_worklist()
... and 4 more: https://git.openjdk.java.net/jdk/compare/9ac162e896dcb07071c7a2146e8eecbd75115edb...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

hseigel · 2020-09-25T17:16:02Z

/integrate

openjdk · 2020-09-25T17:16:48Z

@hseigel Since your change was applied there have been 14 commits pushed to the master branch:

a75edc2: 8251188: Update LDAP tests not to use wildcard addresses
1f5a033: 8253555: Make ByteSize and WordSize typed scoped enums
f62eefc: 8253469: ARM32 Zero: replace usages of __sync_synchronize() with OrderAccess::fence
1b79326: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode
dc1ef58: 8253631: Remove unimplemented CompileBroker methods after JEP-165
27d0a70: 8253633: Remove unimplemented TieredThresholdPolicy::set_carry_if_neccessary
e12d94a: 8253594: Remove CollectedHeap::supports_tlab_allocation
cfa3f74: 8245527: LDAP Channel Binding support for Java GSS/Kerberos
37b7028: 8240556: Abort concurrent mark after effective eager reclamation of humongous objects
dcde95b: 8253586: C2: Clean up unused PhaseIterGVN::init_worklist()
... and 4 more: https://git.openjdk.java.net/jdk/compare/9ac162e896dcb07071c7a2146e8eecbd75115edb...master

Your commit was automatically rebased without conflicts.

Pushed as commit 0187567.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 …

9d2332d

…swap limit capabilities

hseigel marked this pull request as ready for review September 24, 2020 18:34

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Sep 24, 2020

8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 …

2746225

…swap limit capabilities

openjdk bot added the rfr Pull request is ready for review label Sep 24, 2020

bobvandette approved these changes Sep 25, 2020

View reviewed changes

jerboaa reviewed Sep 25, 2020

View reviewed changes

8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 …

bcddedd

…swap limit capabilities

jerboaa suggested changes Sep 25, 2020

View reviewed changes

jerboaa approved these changes Sep 25, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 25, 2020

openjdk bot closed this Sep 25, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated labels Sep 25, 2020

openjdk bot removed the rfr Pull request is ready for review label Sep 25, 2020

8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 … #342

8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 … #342

hseigel commented Sep 24, 2020 •

edited by openjdk bot

hseigel commented Sep 24, 2020

bridgekeeper bot commented Sep 24, 2020

openjdk bot commented Sep 24, 2020

hseigel commented Sep 24, 2020

mlbridge bot commented Sep 24, 2020 •

edited

mlbridge bot commented Sep 24, 2020

jerboaa Sep 25, 2020

hseigel Sep 25, 2020

jerboaa Sep 25, 2020

jerboaa Sep 25, 2020

hseigel Sep 25, 2020

bobvandette Sep 25, 2020

bobvandette Sep 25, 2020 •

edited

jerboaa left a comment

jerboaa Sep 25, 2020

jerboaa Sep 25, 2020

jerboaa Sep 25, 2020

jerboaa left a comment

jerboaa Sep 25, 2020

openjdk bot commented Sep 25, 2020

hseigel commented Sep 25, 2020

openjdk bot commented Sep 25, 2020

8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 … #342

8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 … #342

Conversation

hseigel commented Sep 24, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

hseigel commented Sep 24, 2020

bridgekeeper bot commented Sep 24, 2020

openjdk bot commented Sep 24, 2020

hseigel commented Sep 24, 2020

mlbridge bot commented Sep 24, 2020 • edited

Webrevs

mlbridge bot commented Sep 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobvandette Sep 25, 2020 • edited

Choose a reason for hiding this comment

jerboaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerboaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Sep 25, 2020

hseigel commented Sep 25, 2020

openjdk bot commented Sep 25, 2020

hseigel commented Sep 24, 2020 •

edited by openjdk bot

mlbridge bot commented Sep 24, 2020 •

edited

bobvandette Sep 25, 2020 •

edited