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

jmtd · 2022-09-21T10:12:07Z

This is a backport of 4def210a22faaec6b47912dd314e6365ea48d28f for jdk8u-dev as part of an effort to backport cgroups v2 support.

It does not apply clean. Paths need unshuffling. A number of changes were needed for 8u support. I've structured the PR as separate commits, with each change made in a separate commit for (hopefully) ease of review.

Not all the new tests pass: TestDockerMemoryMetrics failing one, specifically:

Exception in thread "main" java.lang.RuntimeException: Memory and swap limit not equal, expected : [209715200, 1073741824], got : [209715200, 864026624]

I think this is fixed in a further patch to backport, and will confirm.

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-8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy
JDK-8275713: TestDockerMemoryMetrics test fails on recent runc
JDK-8228585: jdk/internal/platform/cgroup/TestCgroupMetrics.java - NumberFormatException because of large long values (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/121/head:pull/121
$ git checkout pull/121

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 121

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

Using diff file

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

bridgekeeper · 2022-09-21T10:12:56Z

👋 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-09-21T10:14:09Z

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

mlbridge · 2022-09-21T10:19:20Z

Webrevs

jmtd · 2022-09-22T14:52:29Z

I'm probably going to rebase this on #123 (8206456). I had conflicts in this backport due to that missing in 8u that I just resolved in this PR, but I've hit further conflicts in the next patch in series, so now addressing 8206456 properly (in #123).

jmtd · 2022-09-22T15:11:19Z

I'm probably going to rebase this on #123 (8206456).

Not necessary. Sorry for the noise!

openjdk-notifier · 2022-09-26T10:09:01Z

@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-09-27T08:10:39Z

I have ended up (artificially) making this PR depend upon pr/123, so that another PR, #124, can depend upon this one and get both - it depends upon both.

openjdk-notifier · 2022-10-21T16:34:13Z

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout 8231111-jdk8u-dev
git fetch https://git.openjdk.org/jdk8u-dev master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

jerboaa

Looks mostly good. A couple of suggestions.

jdk/test/jdk/internal/platform/docker/MetricsMemoryTester.java

jdk/test/lib/jdk/test/lib/containers/cgroup/CgroupMetricsTester.java

jdk/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV1.java

jmtd · 2022-11-01T20:22:12Z

/issue add JDK-8275713

jmtd · 2022-11-01T20:22:39Z

/issue add JDK-8228585

openjdk · 2022-11-01T20:22:47Z

@jmtd
Adding additional issue to issue list: 8275713: TestDockerMemoryMetrics test fails on recent runc.

openjdk · 2022-11-01T20:24:29Z

@jmtd
Adding additional issue to issue list: 8228585: jdk/internal/platform/cgroup/TestCgroupMetrics.java - NumberFormatException because of large long values (memory limit_in_bytes).

openjdk · 2022-11-04T10:00:09Z

⚠️ @jmtd This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk-notifier · 2022-11-04T10:00:30Z

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

openjdk-notifier · 2022-11-04T10:21:36Z

@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

Looks good to me.

jerboaa · 2022-12-01T09:15:16Z

@jmtd Please rebase this onto latest master. Right now it looks like changes from #127 are included here, which shouldn't be?

commit files newly introduced in 19f26f725fd (11u-dev backport of 4def210a22faaec6b47912dd314e6365ea48d28f), with paths rewritten: {src/java.base => jdk/src}/linux/classes/jdk/internal/platform Files not otherwise modified for 8u compatibility yet.

rm {src/java.base => jdk/src}/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java

conflicts in import lines, 11+ have module-related imports not applicable to 8u. Other hunks applied OK. No further inspection performed yet.

unshuffle, rename, resolve unclean hunks although I'm not sure why they didn't apply, they look trivial and the context is the same, will look closer

jdk8u doesn't have java.lang.System.Logger. There's no existing logging in place for other classes in the Metrics/platform/etc family that I can see, so remove it.

two files had fairly significant merge conflicts, eyeball only to resolve haven't run any of them yet -- will depend on the rest of the patch

…recent runc The main hunk from 8275713 was rolled up in the changes for 8231111. This line is also necessary.

jdk8u does not have Arrays.compare()

We need the testlibrary copy of FileUtils but the test.lib.util copy of Utils (method createTempDirectory is missing from the testlib copy)

jmtd · 2022-12-01T10:20:37Z

@jmtd Please rebase this onto latest master. Right now it looks like changes from #127 are included here, which shouldn't be?

Done.

openjdk · 2022-12-01T10:22:09Z

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

gnu-andrew

This looks ok, though again I don't see why two additional changes have been mixed in with this backport, when they could have been in follow-ups. Thanks to @jerboaa for catching that they were in there.

Approved for 8u.

jmtd · 2022-12-05T04:13:06Z

/integrate

openjdk · 2022-12-05T04:14:29Z

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

5510145: 8226236: win32: gc/metaspace/TestCapacityUntilGCWrapAround.java fails
77403ed: 8286624: Regression Test CoordinateTruncationBug.java fails on OL8.3
8356d04: 8295982: Failure in sun/security/tools/keytool/WeakAlg.java - ks: The process cannot access the file because it is being used by another process
fcd3c9c: 8247676: vcruntime140_1.dll is not needed on 32-bit Windows
1ec7984: 8242468: VS2019 build missing vcruntime140_1.dll
918a2ab: 8241087: Build failure with VS 2019 (16.5.0) due to C2039 and C2873
7b54850: 8297329: [8u] hotspot needs to recognise VS2019
e9acfce: 8295322: Tests for JDK-8271459 were not backported to 11u

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-05T04:14:52Z

@jmtd Pushed as commit c9007cd.

💡 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 4def210a22faaec6b47912dd314e6365ea48d28f~~ 8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy Sep 21, 2022

openjdk bot added backport rfr Pull request is ready for review labels Sep 21, 2022

jmtd changed the base branch from master to pr/123 September 26, 2022 08:45

jmtd force-pushed the 8231111-jdk8u-dev branch 2 times, most recently from 8f3570f to 0ea3c60 Compare September 26, 2022 10:08

jmtd mentioned this pull request Sep 26, 2022

8240189: [TESTBUG] Some cgroup tests are failing after JDK-8231111 #124

Closed

3 tasks

openjdk bot mentioned this pull request Oct 5, 2022

8230305: Cgroups v2: Container awareness #127

Closed

3 tasks

openjdk-notifier bot changed the base branch from pr/123 to master October 21, 2022 16:34

jerboaa reviewed Oct 25, 2022

View reviewed changes

jmtd changed the base branch from master to pr/127 November 4, 2022 09:55

jmtd force-pushed the 8231111-jdk8u-dev branch from f56af77 to a2bb169 Compare November 4, 2022 09:59

openjdk bot mentioned this pull request Nov 4, 2022

8253714: [cgroups v2] Soft memory limit incorrectly using memory.high #130

Closed

2 tasks

jmtd force-pushed the 8231111-jdk8u-dev branch from a2bb169 to 5849871 Compare November 4, 2022 10:17

jerboaa approved these changes Nov 4, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 30, 2022

jmtd added 16 commits December 1, 2022 10:15

step 0: new files

73fae17

commit files newly introduced in 19f26f725fd (11u-dev backport of 4def210a22faaec6b47912dd314e6365ea48d28f), with paths rewritten: {src/java.base => jdk/src}/linux/classes/jdk/internal/platform Files not otherwise modified for 8u compatibility yet.

step 1: files deleted by original commit

e2f337d

rm {src/java.base => jdk/src}/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java

rename+patch JNI function

371ecf6

OperatingSystemImpl.java - easy hunk

dabb348

LauncherHelper: resolve conflicts

43eeca5

conflicts in import lines, 11+ have module-related imports not applicable to 8u. Other hunks applied OK. No further inspection performed yet.

Metrics.java: easy commit, mostly doc-only hunks

5ecd93b

Metrics => CgroupV1Subsystem.java

d2a8980

unshuffle, rename, resolve unclean hunks although I'm not sure why they didn't apply, they look trivial and the context is the same, will look closer

CgroupSubsystemFactory: remove logging lines

e2b75c0

jdk8u doesn't have java.lang.System.Logger. There's no existing logging in place for other classes in the Metrics/platform/etc family that I can see, so remove it.

tests for the backport

8db62e3

two files had fairly significant merge conflicts, eyeball only to resolve haven't run any of them yet -- will depend on the rest of the patch

update mapfile for new JNI method name

0dee44d

incorporate (part of) 8275713: TestDockerMemoryMetrics test fails on …

b0b41de

…recent runc The main hunk from 8275713 was rolled up in the changes for 8231111. This line is also necessary.

Replace Arrays.compare with Arrays.equals

c2e17a5

jdk8u does not have Arrays.compare()

TestCgroupSubsystemController: fix jtreg @library path

e5d3ad9

CgroupSubsystemController: fix library paths

88e3960

We need the testlibrary copy of FileUtils but the test.lib.util copy of Utils (method createTempDirectory is missing from the testlib copy)

TestCgroupSubsystemController: rework use of Files.writeString

4ddd3d9

Address style nit

831ee9c

jmtd force-pushed the 8231111-jdk8u-dev branch from 5849871 to 831ee9c Compare December 1, 2022 10:20

jerboaa approved these changes Dec 1, 2022

View reviewed changes

gnu-andrew approved these changes Dec 5, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 5, 2022

openjdk bot closed this Dec 5, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jmtd commented Sep 21, 2022 •

edited by openjdk bot

bridgekeeper bot commented Sep 21, 2022

openjdk bot commented Sep 21, 2022

mlbridge bot commented Sep 21, 2022 •

edited

jmtd commented Sep 22, 2022

jmtd commented Sep 22, 2022

openjdk-notifier bot commented Sep 26, 2022

jmtd commented Sep 27, 2022

openjdk-notifier bot commented Oct 21, 2022

jerboaa left a comment

jmtd commented Nov 1, 2022

jmtd commented Nov 1, 2022

openjdk bot commented Nov 1, 2022

openjdk bot commented Nov 1, 2022

openjdk bot commented Nov 4, 2022

openjdk-notifier bot commented Nov 4, 2022

openjdk-notifier bot commented Nov 4, 2022

jerboaa left a comment

jerboaa commented Dec 1, 2022

jmtd commented Dec 1, 2022

openjdk bot commented Dec 1, 2022

gnu-andrew left a comment

jmtd commented Dec 5, 2022

openjdk bot commented Dec 5, 2022

openjdk bot commented Dec 5, 2022

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

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

Conversation

jmtd commented Sep 21, 2022 • edited by openjdk bot

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Sep 21, 2022

openjdk bot commented Sep 21, 2022

mlbridge bot commented Sep 21, 2022 • edited

Webrevs

jmtd commented Sep 22, 2022

jmtd commented Sep 22, 2022

openjdk-notifier bot commented Sep 26, 2022

jmtd commented Sep 27, 2022

openjdk-notifier bot commented Oct 21, 2022

jerboaa left a comment

Choose a reason for hiding this comment

jmtd commented Nov 1, 2022

jmtd commented Nov 1, 2022

openjdk bot commented Nov 1, 2022

openjdk bot commented Nov 1, 2022

openjdk bot commented Nov 4, 2022

openjdk-notifier bot commented Nov 4, 2022

openjdk-notifier bot commented Nov 4, 2022

jerboaa left a comment

Choose a reason for hiding this comment

jerboaa commented Dec 1, 2022

jmtd commented Dec 1, 2022

openjdk bot commented Dec 1, 2022

gnu-andrew left a comment

Choose a reason for hiding this comment

jmtd commented Dec 5, 2022

openjdk bot commented Dec 5, 2022

openjdk bot commented Dec 5, 2022

jmtd commented Sep 21, 2022 •

edited by openjdk bot

mlbridge bot commented Sep 21, 2022 •

edited