8230305: Cgroups v2: Container awareness by jmtd · Pull Request #127 · openjdk/jdk8u-dev

jmtd · 2022-10-03T09:57:22Z

This is a backport of 8230305 (Cgroups v2: Container awareness) for JDK8u, working from the jdk11u-dev backport as part of an effort to backport cgroups v2 to jdk8u-dev.

The patch does not apply clean after unshuffling and path fixing:

mostly copyright line issues
different package name for jdk.test.lib.process.OutputAnalyzer
Most of hotspot/src/os/linux/vm/cgroupSubsystem_linux.cpp failed due to context changes, manually resolved (mostly deletions)

A few other changes were made

Rework use of newer logging API (log_debug etc) to use guarded tty->print_cr
replace os::strerror with the libc strerror (which is thread unsafe, but that should be addressed by backporting 8148425 separately)

patch touched hotspot/test/runtime/containers/docker/TestCPUAwareness.java which passes afterwards; I'm running further tests now (and awaiting GitHub CI doing the same)

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

Issues

JDK-8230305: Cgroups v2: Container awareness
JDK-8216366: Add rationale to PER_CPU_SHARES define
JDK-8229202: Docker reporting causes secondary crashes in error handling
JDK-8232207: Linux os::available_memory re-reads cgroup configuration on every invocation
JDK-8254997: Remove unimplemented OSContainer::read_memory_limit_in_bytes

Reviewers

Severin Gehwolf (@jerboaa - Reviewer)
Andrew John Hughes (@gnu-andrew - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev pull/127/head:pull/127
$ git checkout pull/127

Update a local copy of the PR:
$ git checkout pull/127
$ git pull https://git.openjdk.org/jdk8u-dev pull/127/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 127

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/127.diff

bridgekeeper · 2022-10-03T09:58:14Z

👋 Welcome back jdowland! 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-10-03T09:59:48Z

This backport pull request has now been updated with issue and summary from the original commit.

mlbridge · 2022-10-03T10:04:29Z

Webrevs

jmtd · 2022-10-03T10:05:05Z

Local test failure observed in runtime/containers/docker/TestMemoryAwareness.java:

JavaTest Message: Test threw exception: java.lang.RuntimeException: 'Memory Soft Limit.*524288000' missing from stdout/stderr

this might be JDK-8253714, which is on the list to backport, I'll look at this next.

openjdk-notifier · 2022-10-05T08:22:22Z

@jmtd Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

jerboaa · 2022-10-17T13:19:08Z

@jmtd Any particular reason this PR depends on the Metrics PR? In JDK 11u and JDK head this was the first one of the cg v2 patch series. If you agree, please drop the dependency, targeting master directly.

jerboaa

This seems promising, thanks. Please use explicit parenthesis for PrintContainerInfo log lines throughout. Also with a space before and after the if. We need to account for https://bugs.openjdk.org/browse/JDK-8227006 (which is in 8u) and https://bugs.openjdk.org/browse/JDK-8232207 (not currently in 8u), though. This patch would include both.

jmtd · 2022-10-18T09:50:38Z

@jmtd Any particular reason this PR depends on the Metrics PR? In JDK 11u and JDK head this was the first one of the cg v2 patch series. If you agree, please drop the dependency, targeting master directly.

I collapsed the patch dependency tree into a linear list, and as a consequence some patches have GitHub PR dependencies which are artificial. This one (depending on 8231111 (pr/121)) is an example; it's because the downstream bug 8253714 (pr/130) depends on both.

I could flip the artifical dependency around, and make 8231111 (metrics, pr/121) depend on this (8230305, pr/127) instead (and follow that change down the chain). Would you prefer that?

jerboaa · 2022-10-18T10:09:18Z

I could flip the artifical dependency around, and make 8231111 (metrics, pr/121) depend on this (8230305, pr/127) instead (and follow that change down the chain). Would you prefer that?

Yes, please. That's matching how patches got integrated in JDK head and JDK 11 updates.

jmtd · 2022-10-18T10:13:17Z

Please use explicit parenthesis for PrintContainerInfo log lines throughout. Also with a space before and after the if

Sure! I'm assuming you mean spaces and braces like this

@@ -64,9 +64,10 @@ CgroupSubsystem* CgroupSubsystemFactory::create() {
    */
   cgroups = fopen("/proc/cgroups", "r");
   if (cgroups == NULL) {
-      if(PrintContainerInfo)
+      if (PrintContainerInfo) {
         tty->print_cr("Can't open /proc/cgroups, %s",
                                  strerror(errno));
+      }
       return NULL;
   }

(although I'm not sure what you mean by spaces before the if) and I'm working that through now.

jmtd · 2022-10-18T10:34:21Z

/issue add JDK-8216366

openjdk · 2022-10-18T10:36:17Z

@jmtd
Adding additional issue to issue list: 8216366: Add rationale to PER_CPU_SHARES define.

jmtd · 2022-10-18T10:37:17Z

/issue add JDK-8229202

openjdk · 2022-10-18T10:37:58Z

@jmtd
Adding additional issue to issue list: 8229202: Docker reporting causes secondary crashes in error handling.

jmtd · 2022-10-31T14:32:04Z

/issue add JDK-8232207

@jerboaa , if you disagree with adding this issue and want it backed out afterall, please let me know!

openjdk · 2022-10-31T14:33:43Z

@jmtd
Adding additional issue to issue list: 8232207: Linux os::available_memory re-reads cgroup configuration on every invocation.

jmtd · 2022-10-31T14:36:59Z

As discussed, and now that all open review notes were addressed, I've re-parented this PR onto master.

openjdk-notifier · 2022-10-31T14:40:47Z

@jmtd Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

jmtd · 2022-11-01T10:53:47Z

After reparenting this PR to master last night, I've inadvertently pulled in the commits from the previous base (PR for 8231111). I'm working on filtering them out now.

Implement Cgroups v2 container awareness in hotspot Reviewed-by: bobv, dholmes

Remove includes of log.hpp that doesn't exist in jdk8u log_(debug|trace) to tty->print_cr guarded by PrintContainerInfo

os::strerror was introduced in 8148425 (strerror() function is not thread-safe), which is probably out of scope for backporting (at least as part of this cgroups v2 effort). Replace uses of os::strerror with (thread unsafe) strerror, in common with the rest of JDK8u.

This causes issues with pre-C++11 compilers (of which jdk8u-dev ostensibly supports). Issue has materialised on ARM32. I haven't introduced initialiser lists to the constructor because all of these are assigned to in the body of the constructor.

openjdk-notifier · 2022-11-01T14:23:53Z

@jmtd Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

jerboaa

This seems fine now.

openjdk · 2022-11-04T13:32:47Z

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

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

8230305: Cgroups v2: Container awareness
8216366: Add rationale to PER_CPU_SHARES define
8229202: Docker reporting causes secondary crashes in error handling
8232207: Linux os::available_memory re-reads cgroup configuration on every invocation
8254997: Remove unimplemented OSContainer::read_memory_limit_in_bytes

Implement Cgroups v2 container awareness in hotspot

Reviewed-by: sgehwolf, andrew

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

3b1bbef: 8297739: Bump update version of OpenJDK: 8u372
41159a5: 8255559: Leak File Descriptors Because of ResolverLocalFilesystem#engineResolveURI()
df44cbf: 8073464: GC workers do not have thread names
9f2f0c2: 6885993: Named Thread: introduce print() and print_on(outputStream* st) methods
c501bfa: 8269039: Disable SHA-1 Signed JARs
5a32484: 8297141: Fix hotspot/test/runtime/SharedArchiveFile/DefaultUseWithClient.java for 8u
da0c382: 8241086: Test runtime/NMT/HugeArenaTracking.java is failing on 32bit Windows
91d8b89: 8129827: [TEST_BUG] Test java/awt/Robot/RobotWheelTest/RobotWheelTest.java fails
9cb8752: 8079255: [TEST_BUG] [macosx] Test closed/java/awt/Robot/RobotWheelTest/RobotWheelTest fails for Mac only
b98d485: 8197859: VS2017 Complains about UINTPTR_MAX definition in globalDefinitions_VisCPP.hpp
... and 18 more: https://git.openjdk.org/jdk8u-dev/compare/f04ad96cf53385c9f8aa071a4167ad7790cb8466...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.

jmtd · 2022-11-16T10:26:16Z

/issue add 8254997

This PR has most of 8232207 in it, so we added that issue. 8232207 originally added a line in osContainer_linux.hpp which was later removed in 8254997. This PR does not add the line, so we address 8254997 at the same time.

openjdk · 2022-11-16T10:27:19Z

@jmtd
Adding additional issue to issue list: 8254997: Remove unimplemented OSContainer::read_memory_limit_in_bytes.

gnu-andrew

Backport looks good. It's not clear to me why JDK-8254997 needed to be bundled into this change rather than a follow-up, but it's not worth changing now.

Approved for 8u.

jmtd · 2022-11-30T20:07:15Z

/integrate

openjdk · 2022-11-30T20:09:12Z

Going to push as commit 7eb2803.
Since your change was applied there have been 28 commits pushed to the master branch:

3b1bbef: 8297739: Bump update version of OpenJDK: 8u372
41159a5: 8255559: Leak File Descriptors Because of ResolverLocalFilesystem#engineResolveURI()
df44cbf: 8073464: GC workers do not have thread names
9f2f0c2: 6885993: Named Thread: introduce print() and print_on(outputStream* st) methods
c501bfa: 8269039: Disable SHA-1 Signed JARs
5a32484: 8297141: Fix hotspot/test/runtime/SharedArchiveFile/DefaultUseWithClient.java for 8u
da0c382: 8241086: Test runtime/NMT/HugeArenaTracking.java is failing on 32bit Windows
91d8b89: 8129827: [TEST_BUG] Test java/awt/Robot/RobotWheelTest/RobotWheelTest.java fails
9cb8752: 8079255: [TEST_BUG] [macosx] Test closed/java/awt/Robot/RobotWheelTest/RobotWheelTest fails for Mac only
b98d485: 8197859: VS2017 Complains about UINTPTR_MAX definition in globalDefinitions_VisCPP.hpp
... and 18 more: https://git.openjdk.org/jdk8u-dev/compare/f04ad96cf53385c9f8aa071a4167ad7790cb8466...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-30T20:09:36Z

@jmtd Pushed as commit 7eb2803.

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

openjdk Bot changed the title ~~Backport d462a6b5c9bd3dae5257cca42ea38c19cb742e3c~~ 8230305: Cgroups v2: Container awareness Oct 3, 2022

openjdk Bot added backport Port of a pull request already in a different code base rfr Pull request is ready for review labels Oct 3, 2022

openjdk Bot mentioned this pull request Oct 4, 2022

8237479: 8230305 causes slowdebug build failure #128

Closed

2 tasks

jmtd changed the base branch from master to pr/121 October 5, 2022 08:19

jmtd force-pushed the 8230305-jdk8u-dev branch from d76d21d to d217ab9 Compare October 5, 2022 08:19

poonamparhar reviewed Oct 7, 2022

View reviewed changes

Comment thread hotspot/src/os/linux/vm/cgroupV1Subsystem_linux.hpp Outdated

jerboaa suggested changes Oct 17, 2022

View reviewed changes

jmtd changed the base branch from pr/121 to master October 31, 2022 14:34

jmtd force-pushed the 8230305-jdk8u-dev branch from e6fcc66 to 6947cdb Compare October 31, 2022 14:36

jerboaa and others added 2 commits November 1, 2022 14:20

8230305: Cgroups v2: Container awareness

2ab8d41

Implement Cgroups v2 container awareness in hotspot Reviewed-by: bobv, dholmes

Rework use of newer logging API to tty->print_cr

d927a16

Remove includes of log.hpp that doesn't exist in jdk8u log_(debug|trace) to tty->print_cr guarded by PrintContainerInfo

jmtd added 4 commits November 1, 2022 14:20

Whitespace and braces around PrintContainerInfo

77c66b6

Guard two more tty->print_cr with PrintContainerInfo

fbc29df

jmtd force-pushed the 8230305-jdk8u-dev branch from 6947cdb to 198cd27 Compare November 1, 2022 14:21

openjdk Bot mentioned this pull request Nov 4, 2022

8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy #121

Closed

3 tasks

jerboaa approved these changes Nov 4, 2022

View reviewed changes

openjdk Bot added the ready Pull request is ready to be integrated label Nov 4, 2022

gnu-andrew approved these changes Nov 30, 2022

View reviewed changes

openjdk Bot added the integrated Pull request has been integrated label Nov 30, 2022

openjdk Bot closed this Nov 30, 2022

openjdk Bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 30, 2022

Uh oh!

Conversation

jmtd commented Oct 3, 2022 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented Oct 3, 2022

Uh oh!

openjdk Bot commented Oct 3, 2022

Uh oh!

mlbridge Bot commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jmtd commented Oct 3, 2022

Uh oh!

openjdk-notifier Bot commented Oct 5, 2022

Uh oh!

Uh oh!

jerboaa commented Oct 17, 2022

Uh oh!

jerboaa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmtd commented Oct 18, 2022

Uh oh!

jerboaa commented Oct 18, 2022

Uh oh!

jmtd commented Oct 18, 2022

Uh oh!

jmtd commented Oct 18, 2022

Uh oh!

openjdk Bot commented Oct 18, 2022

Uh oh!

jmtd commented Oct 18, 2022

Uh oh!

openjdk Bot commented Oct 18, 2022

Uh oh!

jmtd commented Oct 31, 2022

Uh oh!

openjdk Bot commented Oct 31, 2022

Uh oh!

jmtd commented Oct 31, 2022

Uh oh!

openjdk-notifier Bot commented Oct 31, 2022

Uh oh!

jmtd commented Nov 1, 2022

Uh oh!

openjdk-notifier Bot commented Nov 1, 2022

Uh oh!

jerboaa left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk Bot commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmtd commented Nov 16, 2022

Uh oh!

openjdk Bot commented Nov 16, 2022

Uh oh!

gnu-andrew left a comment

Choose a reason for hiding this comment

Uh oh!

jmtd commented Nov 30, 2022

Uh oh!

openjdk Bot commented Nov 30, 2022

Uh oh!

openjdk Bot commented Nov 30, 2022

Uh oh!

Reviewers

jmtd commented Oct 3, 2022 •

edited by openjdk Bot

Loading

mlbridge Bot commented Oct 3, 2022 •

edited

Loading

openjdk Bot commented Nov 4, 2022 •

edited

Loading