New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8245543: Cgroups: Incorrect detection logic on some systems (still reproducible) #485
Conversation
|
@jerboaa The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
@bulasevich Do you want to give this a spin? |
Webrevs
|
src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
Outdated
Show resolved
Hide resolved
src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
Outdated
Show resolved
Hide resolved
The change looks reasonable. I checked the fail - it is gone with the change! And both jtreg tests passed. |
Thanks for the review and for testing it! |
@bobvandette Could you perhaps have a look at this? Many thanks in advance! |
src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
Outdated
Show resolved
Hide resolved
5e2ca2d
to
e72ac21
Compare
@bobvandette How about this? |
src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
Outdated
Show resolved
Hide resolved
Thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me. Consider a minor nit below.
src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
Outdated
Show resolved
Hide resolved
@jerboaa This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
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 8 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the
|
c18be73
to
7a988bf
Compare
/integrate |
@jerboaa Since your change was applied there have been 8 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 2bbf8a2. |
An issue similar to JDK-8239559 has been discovered. On the affected system,
/proc/self/mountinfo
contains a line such as this one:
Note that
/proc/cgroups
looks like this:This is in fact a cgroups v1 system with systemd put into a separate namespace via FS type
cgroup
. That mountinfo line confuses the cgroups version detection heuristic. It only looked whether or not there is any entry in mountinfo of FS-typecgroup
. That's wrong. A better heuristic would be looking at controllers we care about:cpu
,cpuacct
,memory
,cpuset
for hotspot and some more for the Java side. The proposed fix on the hotspot side is to setany_cgroup_mounts_found
to true only if one of those controllers show up in mountinfo. Otherwise, we know it's cgroup v2 due to the zero hierarchy ids. The fix on the Java side is similar.For the Java side the proposal is also to do the parsing of the cgroup files in the factory now (single pass of files). No longer in the version specific objects. In order to not regress here, I've added more tests.
Testing
Progress
Testing
Failed test tasks
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/485/head:pull/485
$ git checkout pull/485