JDK-8293472: Incorrect container resource limit detection if manual cgroup fs mounts present by casparcwang · Pull Request #10193 · openjdk/jdk

casparcwang · 2022-09-07T08:35:47Z

Similar to JDK-8253435, when setting the mount path of cgroup controller "memory/cpu/cpuacct/pids", it should also discard duplicate items.

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-8293472: Incorrect container resource limit detection if manual cgroup fs mounts present

Reviewers

Severin Gehwolf (@jerboaa - Reviewer)
Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to c4109d65

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10193

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

Using diff file

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

bridgekeeper · 2022-09-07T08:37:50Z

👋 Welcome back casparcwang! 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 · 2022-09-07T08:41:06Z

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

hotspot-runtime

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 · 2022-09-07T08:45:12Z

Webrevs

jerboaa

Looks mostly good. https://bugs.openjdk.org/browse/JDK-8270087 seems to be an early sighting of this. It mentions a reproducer, manually mount /sys/fs/cgroup from the host into the container. Please write a container test asserting that no warnings get produced. Perhaps use test/hotspot/jtreg/containers/docker/DockerBasicTest.java and write a new test method javaVersionWithCgMounts or some such, which verifies no warning shows up on stdout.

jerboaa · 2022-09-07T12:36:09Z

My testing on cgv2 and cgv1 indicates that we might have a container resource limit detection problem depending on the order of mountinfo entries are observed at runtime. Please add memory and cpu limit container detection tests which do -v /sys/fs/cgroup:/cgroups-in:ro as a volume mount in addition. Thanks!

casparcwang · 2022-09-08T03:29:21Z

Looks mostly good. https://bugs.openjdk.org/browse/JDK-8270087 seems to be an early sighting of this. It mentions a reproducer, manually mount /sys/fs/cgroup from the host into the container. Please write a container test asserting that no warnings get produced. Perhaps use test/hotspot/jtreg/containers/docker/DockerBasicTest.java and write a new test method javaVersionWithCgMounts or some such, which verifies no warning shows up on stdout.

Thank you very much for the reviewing. I have not noticed JDK-8270087 before, sorry about filing a duplicate jdk bug.

My testing on cgv2 and cgv1 indicates that we might have a container resource limit detection problem depending on the order of mountinfo entries are observed at runtime. Please add memory and cpu limit container detection tests which do -v /sys/fs/cgroup:/cgroups-in:ro as a volume mount in addition. Thanks!

The container resource limit detection problem is also fixed when there are multiple cgroup fs mount entries, it is caused by multiple setting of root mount path in function CgroupSubsystemFactory::determine_type.

jerboaa

We definitely need regression tests for memory/cpu limits with those additional cgroup mounts.
Please take a look at test/hotspot/jtreg/containers/docker/TestCPUAwareness.java and test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java

For cgroups v2 the issues isn't fixed. The paths for cg v2 are being set here:
https://github.com/openjdk/jdk/blob/98da03af50e2372817a7b5e381eea5ee6f2cb919/src/hotspot/os/linux/cgroupSubsystem_linux.cpp#L303..L313.

Example of running (with your patch on a cg v2 system):

$ sudo podman run --rm -ti --memory=300M --memory-swap=300M -v /sys/fs/cgroup:/cgroup-in:ro -v $(pwd)/jdk20-jdk:/opt/jdk:z fedora:36 /opt/jdk/bin/java -Xlog:os+container=trace -version 2>&1 | grep 'Memory Limit'
[0.001s][trace][os,container] Memory Limit is: -2
[0.055s][trace][os,container] Memory Limit is: -2

Expected:

[0.001s][trace][os,container] Memory Limit is: 314572800
[0.068s][trace][os,container] Memory Limit is: 314572800

casparcwang · 2022-09-09T05:41:04Z

We definitely need regression tests for memory/cpu limits with those additional cgroup mounts. Please take a look at test/hotspot/jtreg/containers/docker/TestCPUAwareness.java and test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java

For cgroups v2 the issues isn't fixed. The paths for cg v2 are being set here: https://github.com/openjdk/jdk/blob/98da03af50e2372817a7b5e381eea5ee6f2cb919/src/hotspot/os/linux/cgroupSubsystem_linux.cpp#L303..L313.

Example of running (with your patch on a cg v2 system):
$ sudo podman run --rm -ti --memory=300M --memory-swap=300M -v /sys/fs/cgroup:/cgroup-in:ro -v $(pwd)/jdk20-jdk:/opt/jdk:z fedora:36 /opt/jdk/bin/java -Xlog:os+container=trace -version 2>&1 | grep 'Memory Limit'
[0.001s][trace][os,container] Memory Limit is: -2
[0.055s][trace][os,container] Memory Limit is: -2
Expected:
[0.001s][trace][os,container] Memory Limit is: 314572800
[0.068s][trace][os,container] Memory Limit is: 314572800

I have add a cgroup v2 test with duplicate mount info, now it's working for cgroup v2.

jerboaa

Thanks for the updates! A few more thoughts.

jerboaa

Looks good to me. Feel free to add the suggestion.

openjdk · 2022-09-14T01:13:54Z

⚠️ @casparcwang the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout JDK-8293472
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

openjdk · 2022-09-14T01:13:58Z

@casparcwang 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:

8293472: Incorrect container resource limit detection if manual cgroup fs mounts present

Reviewed-by: sgehwolf, iklam

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

1caba0f: 8292948: JEditorPane ignores font-size styles in external linked css-file
eeb625e: 8290169: adlc: Improve child constraints for vector unary operations
2057070: 8293815: P11PSSSignature.engineUpdate should not print debug messages during normal operation
7376c55: 8293769: RISC-V: Add a second temporary register for BarrierSetAssembler::load_at
d191e47: 8293768: Add links to JLS 19 and 20 from SourceVersion enum constants
a75ddb8: 8293122: (fs) Use file cloning in macOS version of Files::copy method
95c7c55: 8293402: hs-err file printer should reattempt stack trace printing if it fails
211fab8: 8291669: [REDO] Fix array range check hoisting for some scaled loop iv
7f3250d: 8293787: Linux aarch64 build fails after 8292591
2a38791: 8292755: Non-default method in interface leads to a stack overflow in JShell
... and 95 more: https://git.openjdk.org/jdk/compare/ef20ffe4d222d48f0bdba81a0b864d9fb455e9a6...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, @iklam) 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).

casparcwang · 2022-09-14T01:22:39Z

/integrate

openjdk · 2022-09-14T01:23:58Z

@casparcwang
Your change (at version 380054b) is now ready to be sponsored by a Committer.

casparcwang · 2022-09-14T08:49:10Z

Is it OK to be pushed? Or shall we wait for one more review?

jerboaa · 2022-09-14T09:12:27Z

Is it OK to be pushed? Or shall we wait for one more review?

A second review would be good. Perhaps @iklam has some cycles?

iklam

Looks good to me. Just a small nit on the test,

jerboaa

Still good for me.

jerboaa · 2022-09-15T08:11:32Z

@casparcwang Feel free to issue /integrate again and I'll sponsor. Thanks!

casparcwang · 2022-09-15T08:42:41Z

/integrate

casparcwang · 2022-09-15T08:43:07Z

@casparcwang Feel free to issue /integrate again and I'll sponsor. Thanks!

Thank you very much!

openjdk · 2022-09-15T08:44:11Z

@casparcwang
Your change (at version 8170214) is now ready to be sponsored by a Committer.

jerboaa · 2022-09-15T08:44:31Z

/sponsor

openjdk · 2022-09-15T08:47:18Z

Going to push as commit 8f3bbe9.
Since your change was applied there have been 105 commits pushed to the master branch:

1caba0f: 8292948: JEditorPane ignores font-size styles in external linked css-file
eeb625e: 8290169: adlc: Improve child constraints for vector unary operations
2057070: 8293815: P11PSSSignature.engineUpdate should not print debug messages during normal operation
7376c55: 8293769: RISC-V: Add a second temporary register for BarrierSetAssembler::load_at
d191e47: 8293768: Add links to JLS 19 and 20 from SourceVersion enum constants
a75ddb8: 8293122: (fs) Use file cloning in macOS version of Files::copy method
95c7c55: 8293402: hs-err file printer should reattempt stack trace printing if it fails
211fab8: 8291669: [REDO] Fix array range check hoisting for some scaled loop iv
7f3250d: 8293787: Linux aarch64 build fails after 8292591
2a38791: 8292755: Non-default method in interface leads to a stack overflow in JShell
... and 95 more: https://git.openjdk.org/jdk/compare/ef20ffe4d222d48f0bdba81a0b864d9fb455e9a6...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-15T08:47:34Z

@jerboaa @casparcwang Pushed as commit 8f3bbe9.

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

Fix crash of duplicate cg controller mountpoint

eb9c58b

Explicit use name

f989ad1

openjdk bot added the rfr Pull request is ready for review label Sep 7, 2022

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Sep 7, 2022

Fix typo

8730628

casparcwang changed the title ~~JDK-8293472: Cgroup: 'stomping of _mount_path' crash if manually mounted cpusets exist~~ JDK-8293472: Cgroup: 'stomping of _mount_path' crash if manually mounted cg controller exist Sep 7, 2022

Fix typo

3eed090

jerboaa suggested changes Sep 7, 2022

View reviewed changes

Comment thread src/hotspot/os/linux/cgroupSubsystem_linux.cpp Outdated

Comment thread src/hotspot/os/linux/cgroupSubsystem_linux.cpp Outdated

Change warning to debug log

338a095

casparcwang changed the title ~~JDK-8293472: Cgroup: 'stomping of _mount_path' crash if manually mounted cg controller exist~~ JDK-8293472: Potentially incorrect container resource limit detection if manual cgroup fs mounts present Sep 8, 2022

jerboaa suggested changes Sep 8, 2022

View reviewed changes

Comment thread test/hotspot/jtreg/containers/docker/DockerBasicTest.java Outdated

Comment thread test/hotspot/jtreg/containers/docker/DockerBasicTest.java

Fix for cgroup v2

5b73d9d

jerboaa reviewed Sep 9, 2022

View reviewed changes

root added 2 commits September 13, 2022 10:18

Simplify cg2 code

68008b0

Fix typo

20cc485

openjdk bot mentioned this pull request Sep 13, 2022

8293540: [Metrics] Incorrectly detected resource limits with additional cgroup fs mounts #10248

Closed

7 tasks

jerboaa approved these changes Sep 13, 2022

View reviewed changes

Comment thread src/hotspot/os/linux/cgroupSubsystem_linux.cpp

Re-instate the comment

380054b

casparcwang changed the title ~~JDK-8293472: Potentially incorrect container resource limit detection if manual cgroup fs mounts present~~ JDK-8293472: Incorrect container resource limit detection if manual cgroup fs mounts present Sep 14, 2022

openjdk bot added the ready Pull request is ready to be integrated label Sep 14, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 14, 2022

jerboaa approved these changes Sep 14, 2022

View reviewed changes

iklam approved these changes Sep 14, 2022

View reviewed changes

Comment thread test/hotspot/jtreg/containers/docker/DockerBasicTest.java Outdated

Fix test

68507d0

openjdk bot removed the sponsor Pull request is ready to be sponsored label Sep 15, 2022

root added 2 commits September 15, 2022 12:57

Use shouldNotMatch

9bc1f32

Use regular exp

c4109d6

iklam approved these changes Sep 15, 2022

View reviewed changes

Update full name

8170214

jerboaa approved these changes Sep 15, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 15, 2022

openjdk bot added the integrated Pull request has been integrated label Sep 15, 2022

openjdk bot closed this Sep 15, 2022

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 Sep 15, 2022

Conversation

casparcwang commented Sep 7, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Sep 7, 2022

Uh oh!

openjdk bot commented Sep 7, 2022

Uh oh!

mlbridge bot commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jerboaa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jerboaa commented Sep 7, 2022

Uh oh!

casparcwang commented Sep 8, 2022

Uh oh!

jerboaa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

casparcwang commented Sep 9, 2022

Uh oh!

jerboaa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jerboaa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openjdk bot commented Sep 14, 2022

Uh oh!

openjdk bot commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casparcwang commented Sep 14, 2022

Uh oh!

openjdk bot commented Sep 14, 2022

Uh oh!

casparcwang commented Sep 14, 2022

Uh oh!

jerboaa commented Sep 14, 2022

Uh oh!

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

jerboaa commented Sep 15, 2022

Uh oh!

casparcwang commented Sep 15, 2022

Uh oh!

casparcwang commented Sep 15, 2022

Uh oh!

openjdk bot commented Sep 15, 2022

Uh oh!

jerboaa commented Sep 15, 2022

Uh oh!

openjdk bot commented Sep 15, 2022

Uh oh!

openjdk bot commented Sep 15, 2022

Uh oh!

Reviewers

Assignees

Labels

casparcwang commented Sep 7, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 7, 2022 •

edited

Loading

openjdk bot commented Sep 14, 2022 •

edited

Loading