8327946: containers/docker/TestJFREvents.java fails when host kernel config vm.swappiness=0 after JDK-8325139 #18225

sendaoYan · 2024-03-12T09:06:45Z

Hi,

According to the docker document, the default value of --memory-swappiness is inherited from the host machine. So, when the the kernel config vm.swappiness=0 on the host machine, this testcase will fail, because of docker container can not use swap memory, the deafult value of --memory-swappiness is 0.

When the host kernel config "vm.swappiness = 0", In order to run this testcase passed , there are three methods:

change .shouldContain("totalSize = " + expectedTotalValue) to .shouldContain("totalSize = ", which ignored the expectedTotalValue, because the expectedTotalValue could be 0(swap memroy is disable when --memory-swappiness=0) or could be 104857600(300MB-200MB=100MB), it depends on the host machine config vm.swappiness
Change the default --memory-swappiness 0 to non-zero, such as 60.
Change the host kernel config vm.swappiness=0 to vm.swappiness=60. I think it's not a good idea.

Maybe the 2rd method seems more resonable.

Thanks,
-sendao

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8327946: containers/docker/TestJFREvents.java fails when host kernel config vm.swappiness=0 after JDK-8325139 (Bug - P4)

Reviewers

Severin Gehwolf (@jerboaa - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18225/head:pull/18225
$ git checkout pull/18225

Update a local copy of the PR:
$ git checkout pull/18225
$ git pull https://git.openjdk.org/jdk.git pull/18225/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18225

View PR using the GUI difftool:
$ git pr show -t 18225

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18225.diff

Webrev

Link to Webrev Comment

…config vm.swappiness=0 after 8325139 According to the docker document(https://docs.docker.com/config/containers/resource_constraints/#--memory-swappiness-details), the default value of --memory-swappiness is inherited from the host machine. So, when the the kervel config vm.swappiness=0 on the host machine, this testcase will fail, because of docker container can not use swap memory, the deafult value of --memory-swappiness is 0. Signed-off-by: sendaoYan <yansendao.ysd@alibaba-inc.com>

bridgekeeper · 2024-03-12T09:07:20Z

👋 Welcome back syan! 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 · 2024-03-12T09:08:46Z

@sendaoYan The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-03-12T09:11:21Z

Webrevs

openjdk · 2024-03-13T20:10:24Z

@sendaoYan 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 details.

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

8327946: containers/docker/TestJFREvents.java fails when host kernel config vm.swappiness=0 after JDK-8325139

Reviewed-by: 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 525 new commits pushed to the master branch:

cd4c3aa: 8330242: RISC-V: Simplify and remove CORRECT_COMPILER_ATOMIC_SUPPORT in atomic_linux_riscv.hpp
e57a322: 8329970: Update autoconf build-aux files with latest from 2024-01-01
c8702ed: 8330213: RISC-V: C2: assert(false) failed: bad AD file after JDK-8316991
f6f038a: 8317368: [JVMCI] SIGSEGV in JVMCIEnv::initialize_installed_code on libgraal
fb4cf1c: 8329636: Deprecate -XX:+PreserveAllAnnotations
d1c6cd1: 8330133: libj2pkcs11.so crashes on some pkcs#11 v3.0 libraries
76cbe48: 8329430: MetaspaceShared::preload_and_dump should clear pending exception
f7c8413: 8326116: JFR: Add help option to -XX:StartFlightRecording
941bee1: 8327640: Allow NumberFormat strict parsing
2ede143: 8330279: Typo in java.text.Bidi class description
... and 515 more: https://git.openjdk.org/jdk/compare/139681a7eb7a0a1207f66e72095766983e7b717b...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jerboaa) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

sendaoYan · 2024-03-14T02:04:53Z

Fix the testcase bug, the risk is low. Can anyone reivew this PR, thanks.

bridgekeeper · 2024-04-11T06:12:09Z

@sendaoYan This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

sendaoYan · 2024-04-11T06:23:48Z

Fix the testcase bug, the risk is low. Can anyone reivew this PR, thanks.

jerboaa

--memory-swappiness is cgv1 specific and needs to be handled for the cgv2 case.

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

jerboaa

I don't think we need Whitebox. A simpler check would be to use the Metrics class. See for example test/hotspot/jtreg/containers/docker/TestMemoryWithCgroupV1.java.

sendaoYan · 2024-04-12T09:50:29Z

I don't think we need Whitebox. A simpler check would be to use the Metrics class. See for example test/hotspot/jtreg/containers/docker/TestMemoryWithCgroupV1.java.

Thanks your advice. The Whitebox has been removed, and Use Metricsclass to get the cgroup version information.

jerboaa

Better. I don't think we need to duplicate that much of the test. All we'd need to do is to add the --memory-swappiness option only if we are on cg v1:

                                          opts.addDockerOpts("--memory=" + memValueToSet)
                                                  .addDockerOpts("--memory-swap=" + swapValueToSet)
                                                  .addClassOptions("jdk.SwapSpace"));
                                          if (isCgroupV1) {
                                              // With Cgroupv1, The default memory-swappiness vaule is inherited from the host machine, which maybe 0
                                              opts.addDockerOpts("--memory-swappiness=60");
                                          }
                                          out = DockerTestUtils.dockerRunJava(opts);
                                          ...

…ess=60"); 2. delete extra space at the beginning of the line in testSwapMemory

sendaoYan · 2024-04-12T15:30:22Z

Better. I don't think we need to duplicate that much of the test. All we'd need to do is to add the --memory-swappiness option only if we are on cg v1:

                                          opts.addDockerOpts("--memory=" + memValueToSet)
                                                  .addDockerOpts("--memory-swap=" + swapValueToSet)
                                                  .addClassOptions("jdk.SwapSpace"));
                                          if (isCgroupV1) {
                                              // With Cgroupv1, The default memory-swappiness vaule is inherited from the host machine, which maybe 0
                                              opts.addDockerOpts("--memory-swappiness=60");
                                          }
                                          out = DockerTestUtils.dockerRunJava(opts);
                                          ...

@jerboaa Thank you very much for your very professional opinions. The modification of the review opinions has been completed. In addition, an extra space at the beginning of the line was deleted.
The changed has been verified both on cgroupv2(fedora35) and cgroupv1(alinux3)

jerboaa

Looks OK to me.

jerboaa · 2024-04-12T16:30:31Z

If you merge latest master, GHA should be clean too.

sendaoYan · 2024-04-13T02:55:48Z

If you merge latest master, GHA should be clean too.

The GHA shows java/lang/String/StringRepeat.java#id1 fails on linux x86. It's a testcase bug which has been fixed in 8328524. The failure is unrelated to this PR.

sendaoYan · 2024-04-17T02:10:27Z

Looks OK to me.

Thanks for the review.

sendaoYan · 2024-04-17T02:10:57Z

/integrate

openjdk · 2024-04-17T02:23:06Z

@sendaoYan
Your change (at version 4e5ea66) is now ready to be sponsored by a Committer.

jerboaa · 2024-04-17T08:38:38Z

/sponsor

openjdk · 2024-04-17T08:39:14Z

Going to push as commit 7744b00.
Since your change was applied there have been 528 commits pushed to the master branch:

9d63fee: 8330262: C2: simplify transfer of GC barrier data from Ideal to Mach nodes
01bda27: 8330011: [s390x] update block-comments to make code consistent
2fe2f3a: 8323900: Avoid calling os::init_random() in CDS static dump
cd4c3aa: 8330242: RISC-V: Simplify and remove CORRECT_COMPILER_ATOMIC_SUPPORT in atomic_linux_riscv.hpp
e57a322: 8329970: Update autoconf build-aux files with latest from 2024-01-01
c8702ed: 8330213: RISC-V: C2: assert(false) failed: bad AD file after JDK-8316991
f6f038a: 8317368: [JVMCI] SIGSEGV in JVMCIEnv::initialize_installed_code on libgraal
fb4cf1c: 8329636: Deprecate -XX:+PreserveAllAnnotations
d1c6cd1: 8330133: libj2pkcs11.so crashes on some pkcs#11 v3.0 libraries
76cbe48: 8329430: MetaspaceShared::preload_and_dump should clear pending exception
... and 518 more: https://git.openjdk.org/jdk/compare/139681a7eb7a0a1207f66e72095766983e7b717b...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-17T08:39:20Z

@jerboaa @sendaoYan Pushed as commit 7744b00.

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

sendaoYan · 2024-04-17T16:19:05Z

/sponsor

Thanks.

openjdk bot added the rfr Pull request is ready for review label Mar 12, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Mar 12, 2024

sendaoYan changed the title ~~8327946: containers/docker/TestJFREvents.java fails when host kernel config vm.swappiness=0 after 8325139~~ 8327946: containers/docker/TestJFREvents.java fails when host kernel config vm.swappiness=0 after JDK-8325139 Mar 13, 2024

jerboaa suggested changes Apr 11, 2024

View reviewed changes

test/hotspot/jtreg/containers/docker/TestJFREvents.java Outdated Show resolved Hide resolved

sendaoYan added 2 commits April 11, 2024 23:20

add a space before //

4a9f388

If isCgroupV1() return true, add docker run opts --memory-swappiness=60

ecb7159

jerboaa reviewed Apr 12, 2024

View reviewed changes

use Metrics.class get the CgroupV1 information

bd144b7

jerboaa reviewed Apr 12, 2024

View reviewed changes

1. if (isCgroupV1) only contains opts.addDockerOpts("--memory-swappin…

4e5ea66

…ess=60"); 2. delete extra space at the beginning of the line in testSwapMemory

jerboaa approved these changes Apr 12, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 12, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 17, 2024

openjdk bot added the integrated Pull request has been integrated label Apr 17, 2024

openjdk bot closed this Apr 17, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 17, 2024

8327946: containers/docker/TestJFREvents.java fails when host kernel config vm.swappiness=0 after JDK-8325139 #18225

8327946: containers/docker/TestJFREvents.java fails when host kernel config vm.swappiness=0 after JDK-8325139 #18225

Uh oh!

Conversation

sendaoYan commented Mar 12, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 12, 2024

Uh oh!

openjdk bot commented Mar 12, 2024

Uh oh!

mlbridge bot commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

openjdk bot commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sendaoYan commented Mar 14, 2024

Uh oh!

bridgekeeper bot commented Apr 11, 2024

Uh oh!

sendaoYan commented Apr 11, 2024

Uh oh!

jerboaa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jerboaa left a comment

Choose a reason for hiding this comment

Uh oh!

sendaoYan commented Apr 12, 2024

Uh oh!

jerboaa left a comment

Choose a reason for hiding this comment

Uh oh!

sendaoYan commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerboaa left a comment

Choose a reason for hiding this comment

Uh oh!

jerboaa commented Apr 12, 2024

Uh oh!

sendaoYan commented Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sendaoYan commented Apr 17, 2024

Uh oh!

sendaoYan commented Apr 17, 2024

Uh oh!

openjdk bot commented Apr 17, 2024

Uh oh!

jerboaa commented Apr 17, 2024

Uh oh!

openjdk bot commented Apr 17, 2024

Uh oh!

openjdk bot commented Apr 17, 2024

Uh oh!

sendaoYan commented Apr 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

sendaoYan commented Mar 12, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 12, 2024 •

edited

Loading

openjdk bot commented Mar 13, 2024 •

edited

Loading

sendaoYan commented Apr 12, 2024 •

edited

Loading

sendaoYan commented Apr 13, 2024 •

edited

Loading