Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]
On Wed, 22 May 2024 08:07:59 GMT, Magnus Ihse Bursie wrote: >> Actually, this is a bit strange. I thought jcheck would look for missing >> newline at EOF, and that properties files were included in the check >> nowadays. I'll need to check this out. > > I did some testing and it turns out that this is indeed not checked. I > believe this is a miss in the Skara reimplementation of jcheck. I've opened > https://bugs.openjdk.org/browse/SKARA-2265 to track this. > > Nevertheless, it would be good if you could fix this. :) Sure. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1609856505
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 110 commits: - Merge branch 'master' into jdk-8311302-jmodless-link - Fix new line in jlink.properties - Merge branch 'master' into jdk-8311302-jmodless-link - Simplify JLINK_JDK_EXTRA_OPTS var handling - Only add runtime track files for linkable runtimes - Generate the runtime image link diff at jlink time But only do that once the plugin-pipeline has run. The generation is enabled with the hidden option '--generate-linkable-runtime' and the resource pools available at jlink time are being used to generate the diff. - Merge branch 'master' into jdk-8311302-jmodless-link - Use shorter name for the build tool - Review feedback from Erik J. - Remove dependency on CDS which isn't needed anymore - ... and 100 more: https://git.openjdk.org/jdk/compare/4f1a10f8...e1e3f02f - Changes: https://git.openjdk.org/jdk/pull/14787/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=28 Stats: 3424 lines in 36 files changed: 3272 ins; 110 del; 42 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]
On Wed, 22 May 2024 12:25:09 GMT, Severin Gehwolf wrote: >> I did some testing and it turns out that this is indeed not checked. I >> believe this is a miss in the Skara reimplementation of jcheck. I've opened >> https://bugs.openjdk.org/browse/SKARA-2265 to track this. >> >> Nevertheless, it would be good if you could fix this. :) > > Sure. Should be fixed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1609943363
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Thu, 16 May 2024 13:47:20 GMT, Alan Bateman wrote: >>> If I understand you correctly, this would be no longer a build-time only >>> approach to produce the "linkable runtime"? It would be some-kind of >>> jlink-option driven approach (as it would run some code that should only >>> run when producing a linkable runtime is requested)? Other than that, it >>> should be fine as the previous iteration basically did that but at a >>> different phase. Also note that the previous iteration used a build-only >>> jlink plugin so as to satisfy the build-time only approach, yet it ran as >>> part of the plugin-pipeline which wasn't desired either. But it was >>> something similar to what you seem to be describing now. Did I get >>> something wrong? >> >> I think it continues to build time, it's just using some hidden jlink >> option. So yes, it similar to a previous iteration except that it doesn't >> run as a plugin the pipeline and the delta goes to the lib directory. >> >> Let's see what @mlchung says. You've put a lot of work into this so let's >> see if we can converge to avoid too many more rounds. > >> @AlanBateman @mlchung The latest update now uses the `jlink` build time >> option `--generate-linkable-runtime` to add needed resources to the `jimage` >> when a runtime linkable JDK image is being asked for using the configure >> option. This now runs outside the plugin-pipeline. I think this is what you >> meant. Sorry it took longer to get back to this... > > I think you've got this to a good place and I think the overall solution is > good. It may be that JDK should move to this by default in the future, and at > the same time re-visit the restriction on generating an image containing > jdk.jlink, but let's see if any issues come up. > > I've added myself as Reviewer to the CSR and I'm working through the code > changes. @AlanBateman > I'm working through the code changes. Do you think you'll be able to review this next week? - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2127614784
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Thu, 23 May 2024 18:52:53 GMT, Alan Bateman wrote: > Yes, I want to help you get this one over the line. Thanks! Appreciate that. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2133375454
Re: RFR: 8333301: Remove static builds using --enable-static-build
On Thu, 30 May 2024 19:14:43 GMT, Magnus Ihse Bursie wrote: > The original way of building static libraries in the JDK was to use the > configure argument --enable-static-build, which set the value of the make > variable STATIC_BUILD. (Note that this is not the same as the source code > definition STATIC_BUILD, which will be set by other means as well.) > > This method only ever worked on macOS, and has long since stopped working. It > was originally introduced for the Mobile Project, but I've now learn that not > even they use it anymore. > > It just adds clutter to the build system, and should be removed. Looks OK to me. - Marked as reviewed by sgehwolf (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19487#pullrequestreview-2091001402
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v3]
On Fri, 3 May 2024 16:05:30 GMT, Severin Gehwolf wrote: >> Please review this enhancement to the container detection code which allows >> it to figure out whether the JVM is actually running inside a container >> (`podman`, `docker`, `crio`), or with some other means that enforces >> memory/cpu limits by means of the cgroup filesystem. If neither of those >> conditions hold, the JVM runs in not containerized mode, addressing the >> issue described in the JBS tracker. For example, on my Linux system >> `is_containerized() == false" is being indicated with the following trace >> log line: >> >> >> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false >> because no cpu or memory limit is present >> >> >> This state is being exposed by the Java `Metrics` API class using the new >> (still JDK internal) `isContainerized()` method. Example: >> >> >> java -XshowSettings:system --version >> Operating System Metrics: >> Provider: cgroupv1 >> System not containerized. >> openjdk 23-internal 2024-09-17 >> OpenJDK Runtime Environment (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing) >> >> >> The basic property this is being built on is the observation that the cgroup >> controllers typically get mounted read only into containers. Note that the >> current container tests assert that `OSContainer::is_containerized() == >> true` in various tests. Therefore, using the heuristic of "is any memory or >> cpu limit present" isn't sufficient. I had considered that in an earlier >> iteration, but many container tests failed. >> >> Overall, I think, with this patch we improve the current situation of >> claiming a containerized system being present when it's actually just a >> regular Linux system. >> >> Testing: >> >> - [x] GHA (risc-v failure seems infra related) >> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 >> (including gtests) >> - [x] Some manual testing using cri-o >> >> Thoughts? > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 12 additional > commits since the last revision: > > - Add doc for mountinfo scanning. > - Unify naming of variables > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - jcheck fixes > - Fix tests > - Implement Metrics.isContainerized() > - Some clean-up > - Drop cgroups testing on plain Linux > - Implement fall-back logic for non-ro controller mounts > - ... and 2 more: https://git.openjdk.org/jdk/compare/88976cae...434430ca Keep open bot. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2144706137
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Sun, 2 Jun 2024 17:41:55 GMT, Alan Bateman wrote: > I've been looking through the changes. One thing that I'm wondering about is > whether --generate-runtime-link-image should disable the keeping of packaged > modules (set JLINK_KEEP_PACKAGED_MODULES to false). It seems surprising to > use this configure option and have the jlink command in the build also copy > the packaged modules into the image. @AlanBateman IMO those are orthogonal concepts. Note that the configure options are `--enable-packaged-modules` and `--enable-runtime-link-image`. The corresponding `jlink` options are `--keep-packaged-modules` and `--generate-linkable-runtime`. My mental model is that with this patch it allows a more flexible distribution of the JDK build. The testing story also seems easier in its current form. All non-linkable runtime tests run as-is - with a linkable runtime build - but also run linkable runtime tests (those have appropriate `@requires` tags). We've had some discussion around this already in this review thread. I'm arguing that both configure options make sense independently and in combination. The user can configure it to their liking. What I'd try to avoid is needing to produce two different builds whether or not the JDK is runtime linkable or not. That is because our prime use-case is to make `jmods` optional when `jlink` is being used post build and test. I've tried to explain it earlier [here](https://github.com/openjdk/jdk/pull/14787#issuecomment-1999307995) and [here](https://github.com/openjdk/jdk/pull/14787#issuecomment-2003848668). @mlchung seemed OK with it [here](https://github.com/openjdk/jdk/pull/14787#issuecomment-2004605507) and @erikj79 was ok with it as well [here](https://github.com/openjdk/jdk/pull/14787#issuecomment-2004761747). Is there a specific reason this needs to be done? With the current patch `--enable-runtime-link-image` influences how the `jimage` in `lib/modules` looks like (adds some metadata) nothing else. `--enable-packaged-modules` influences copying of the packaged modules. Right now, what you are suggesting could be achieved with these configure options: `--enable-runtime-link-image --disable-packaged-modules` Is that not sufficient? Thoughts? - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2144808992
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Mon, 3 Jun 2024 12:55:54 GMT, Alan Bateman wrote: > So I think we may have the wrong default. Yes, they are separate configure > and jlink options but I'm wondering if it would be more sensible to > opt-in(not opt-out) to keep the packaged modules when configured with > --enable-runtime-link-image. OK. I'll rework it so that we'll have: | config opts | effect | equivalent to | | -|--|-| | `--enable-runtime-link-image` | produces a linkable runtime **without** packaged modules even though the default of `--enable-packaged-modules` is otherwise `true`.| `--enable-runtime-link-image --disable-packaged-modules`| | `--enable-runtime-link-image --enable-packaged-modules`| produces a linkable runtime **with** packaged modules, overriding the default of packaged modules not being enabled when `--enable-runtime-link-image` is being used otherwise | N/A| | `--disable-runtime-link-image` | Default as of today. Adds packaged modules, with no run time link supporting jimage | `--disable-runtime-link-image --enable-packaged-modules`| | `--disable-runtime-link-image --disable-packaged-modules` | No linkable jimage runtime, no packaged modules in the resulting JDK | N/A | Does that proposal sound good? - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2145540168
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Tue, 4 Jun 2024 09:04:30 GMT, Magnus Ihse Bursie wrote: > > Does that proposal sound good? > > What you basically is saying is that the default value of `packaged-modules` > should be `! runtime-link-image` (i.e. the inverse)? Yes. **default** of the `packaged-modules` value being the key. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2147004354
RFR: 8333446: Add tests for hierarchical container support
Please review this PR which adds test support for systemd slices so that bugs like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be verified. The added test, `SystemdMemoryAwarenessTest` currently passes on cgroups v1 and fails on cgroups v2 due to the way how [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented when JDK 13 was a thing. Therefore immediately problem-listed. It should get unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) merges. I'm adding those tests in order to not regress another time. Testing: - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to JDK-8322420) - [x] GHA - Commit messages: - Fix comments - 8333446: Add tests for hierarchical container support Changes: https://git.openjdk.org/jdk/pull/19530/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8333446 Stats: 489 lines in 8 files changed: 482 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19530.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19530/head:pull/19530 PR: https://git.openjdk.org/jdk/pull/19530
Re: RFR: 8333446: Add tests for hierarchical container support
On Mon, 3 Jun 2024 17:28:09 GMT, Severin Gehwolf wrote: > Please review this PR which adds test support for systemd slices so that bugs > like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be > verified. The added test, `SystemdMemoryAwarenessTest` currently passes on > cgroups v1 and fails on cgroups v2 due to the way how > [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented > when JDK 13 was a thing. Therefore immediately problem-listed. It should get > unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) > merges. > > I'm adding those tests in order to not regress another time. > > Testing: > - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. > - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to JDK-8322420) > - [x] GHA GHA failure of macos-x64 seems infra related and not related to this patch. - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2147097626
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Mon, 3 Jun 2024 19:10:22 GMT, Alan Bateman wrote: > I've read through most of the changes now. Overall I think it's looking good, > just a few terminology and minor points that I'll add as comments. @AlanBateman I don't see those comments. Should I? - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2147116736
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Mon, 3 Jun 2024 19:10:22 GMT, Alan Bateman wrote: > > Does that proposal sound good? > > That table is useful, I think it's right. No change to default behavior. If > someone configures with --enable-runtime-image then they get a JDK run-time > image that supports jlink (with some limitations) but is significantly small > as it doesn't include the packaged modules. https://github.com/openjdk/jdk/pull/14787/commits/5fef297ba63d60452f098193d2cba33a941b implements this. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2147609799
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 113 commits: - Mark some tests with requiring packaged modules - Correctly set packaged modules default - Merge branch 'master' into jdk-8311302-jmodless-link - Merge branch 'master' into jdk-8311302-jmodless-link - Fix new line in jlink.properties - Merge branch 'master' into jdk-8311302-jmodless-link - Simplify JLINK_JDK_EXTRA_OPTS var handling - Only add runtime track files for linkable runtimes - Generate the runtime image link diff at jlink time But only do that once the plugin-pipeline has run. The generation is enabled with the hidden option '--generate-linkable-runtime' and the resource pools available at jlink time are being used to generate the diff. - Merge branch 'master' into jdk-8311302-jmodless-link - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d - Changes: https://git.openjdk.org/jdk/pull/14787/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=29 Stats: 3473 lines in 41 files changed: 3312 ins; 117 del; 44 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Sun, 2 Jun 2024 17:41:55 GMT, Alan Bateman wrote: > (Doing that would of course mean that several existing jlink tests would need > some changes, either to `@requires` or to remove the checks for the `jmods` > directory) I've added a couple of `@requires jlink.packagedModules` (new with this patch) so that jlink tests don't start to fail with it. As a follow-up I've filed https://bugs.openjdk.org/browse/JDK-8333541. This change is becoming large enough as it is and would like to get this in before RDP1 on Thursday. The initial work was submitted almost a year ago. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2147709606
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Wed, 5 Jun 2024 13:52:43 GMT, Alan Bateman wrote:
>> Severin Gehwolf has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 113 commits:
>>
>> - Mark some tests with requiring packaged modules
>> - Correctly set packaged modules default
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Fix new line in jlink.properties
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Simplify JLINK_JDK_EXTRA_OPTS var handling
>> - Only add runtime track files for linkable runtimes
>> - Generate the runtime image link diff at jlink time
>>
>>But only do that once the plugin-pipeline has run. The generation is
>>enabled with the hidden option '--generate-linkable-runtime' and
>>the resource pools available at jlink time are being used to generate
>>the diff.
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d
>
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourceDiff.java
> line 39:
>
>> 37:
>> 38: /**
>> 39: * Class representing a difference between a jimage resource. For all
>> intents
>
> A difference between a jimage resource and ??? Someone might wonder about
> that.
I've updated the comment a bit to make it clearer.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourcePoolReader.java
> line 33:
>
>> 31: import jdk.tools.jlink.plugin.ResourcePool;
>> 32:
>> 33: @SuppressWarnings("try")
>
> Is this needed?
Yes. Otherwise this warning is produced:
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourcePoolReader.java:32:
warning: [try] auto-closeable resource ResourcePoolReader has a member method
close() that could throw InterruptedException
public class ResourcePoolReader implements ImageResource {
^
error: warnings found and -Werror specified
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1628112307
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1628111856
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Wed, 5 Jun 2024 13:54:07 GMT, Alan Bateman wrote:
>> Severin Gehwolf has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 113 commits:
>>
>> - Mark some tests with requiring packaged modules
>> - Correctly set packaged modules default
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Fix new line in jlink.properties
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Simplify JLINK_JDK_EXTRA_OPTS var handling
>> - Only add runtime track files for linkable runtimes
>> - Generate the runtime image link diff at jlink time
>>
>>But only do that once the plugin-pipeline has run. The generation is
>>enabled with the hidden option '--generate-linkable-runtime' and
>>the resource pools available at jlink time are being used to generate
>>the diff.
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d
>
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourceDiff.java
> line 215:
>
>> 213: }
>> 214: } catch (IOException e) {
>> 215: e.printStackTrace();
>
> Is this IOException swallowed by jlink when not running with the debugging
> option? I'm wondering about the printStackTrace here.
I think this is a left-over of some devevelopment work. Removed now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1628115979
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Wed, 5 Jun 2024 13:46:59 GMT, Alan Bateman wrote: >> Severin Gehwolf has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 113 commits: >> >> - Mark some tests with requiring packaged modules >> - Correctly set packaged modules default >> - Merge branch 'master' into jdk-8311302-jmodless-link >> - Merge branch 'master' into jdk-8311302-jmodless-link >> - Fix new line in jlink.properties >> - Merge branch 'master' into jdk-8311302-jmodless-link >> - Simplify JLINK_JDK_EXTRA_OPTS var handling >> - Only add runtime track files for linkable runtimes >> - Generate the runtime image link diff at jlink time >> >>But only do that once the plugin-pipeline has run. The generation is >>enabled with the hidden option '--generate-linkable-runtime' and >>the resource pools available at jlink time are being used to generate >>the diff. >> - Merge branch 'master' into jdk-8311302-jmodless-link >> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d > > src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line > 119: > >> 117: >> 118: err.runtime.link.not.linkable.runtime=The current run-time image does >> not support run-time linking. >> 119: err.runtime.link.jdk.jlink.prohibited=Including jdk.jlink module for >> run-time image based links is not allowed. > > The phrase "run-time image based links" in this error message (and in the > value of err.runtime.link.packaged.mods) is a bit unusual. As developers will > see this message then maybe it could say that this jlink in the current > run-time image doesn't contain packaged modules and cannot be used to create > another run-time image that include the jdk.jlink module. I've used alternative phrasing. Hopefully better now. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1628146154
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v31]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request incrementally with six additional commits since the last revision: - Move JImageHelper - Update wording on multi-hop. - Remove printStackTrace() - Fix comment. Stream.toList() - Use pattern matching for instanceof in JRTArchive::equals - Rename JLINK_PRODUCE_RUNTIME_LINK_JDK to JLINK_PRODUCE_LINKABLE_RUNTIME - Changes: - all: https://git.openjdk.org/jdk/pull/14787/files - new: https://git.openjdk.org/jdk/pull/14787/files/0eb1e48d..b72648ba Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=30 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=29-30 Stats: 28 lines in 10 files changed: 4 ins; 7 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Wed, 5 Jun 2024 13:21:20 GMT, Alan Bateman wrote:
>> Severin Gehwolf has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 113 commits:
>>
>> - Mark some tests with requiring packaged modules
>> - Correctly set packaged modules default
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Fix new line in jlink.properties
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Simplify JLINK_JDK_EXTRA_OPTS var handling
>> - Only add runtime track files for linkable runtimes
>> - Generate the runtime image link diff at jlink time
>>
>>But only do that once the plugin-pipeline has run. The generation is
>>enabled with the hidden option '--generate-linkable-runtime' and
>>the resource pools available at jlink time are being used to generate
>>the diff.
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d
>
>> [5fef297](https://github.com/openjdk/jdk/commit/5fef297ba63d60452f098193d2cba33a941b)
>> implements this.
>
> I think it was surprising that --enable-runtime-link-image still included the
> packaged modules so thanks for taking the feedback on this point.
Thanks for the review, @AlanBateman! Updates pushed.
> test/jdk/tools/lib/tests/JImageHelper.java line 38:
>
>> 36: * JDK Modular image iterator
>> 37: */
>> 38: public class JImageHelper {
>
> This is only usable in tests that use `@modules` to open
> jdk.internal.jimage.*. It might be better co-locate this with the jlink
> tests for now. It may be that we do have test infra structure for jimage in
> the future but starting out with the supporting test lib in the jlink test
> tree should be okay.
Sure.
-
PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2150589379
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1628156069
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Tue, 4 Jun 2024 09:04:30 GMT, Magnus Ihse Bursie wrote: >>> Does that proposal sound good? >> >> That table is useful, I think it's right. No change to default behavior. If >> someone configures with --enable-runtime-image then they get a JDK run-time >> image that supports jlink (with some limitations) but is significantly small >> as it doesn't include the packaged modules. >> >> I've read through most of the changes now. Overall I think it's looking >> good, just a few terminology and minor points that I'll add as comments. > >> Does that proposal sound good? > > What you basically is saying is that the default value of `packaged-modules` > should be `! runtime-link-image` (i.e. the inverse)? @magicus @erikj79 Do you mind re-reviewing the build changes? - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2150590534
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision: Fix default description of keep-packaged-modules - Changes: - all: https://git.openjdk.org/jdk/pull/14787/files - new: https://git.openjdk.org/jdk/pull/14787/files/b72648ba..7a8f839e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=31 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=30-31 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v31]
On Thu, 6 Jun 2024 09:33:43 GMT, Magnus Ihse Bursie wrote: > As Erik says. You need to add something like: `DEFAULT_DESC: [the inverse of > --enable-runtime-link-image]`. https://github.com/openjdk/jdk/pull/14787/commits/7a8f839e55c5109deeb5022d2338b37387c95c85 does that. Sorry it clashed with your comment. It sets it to `enabled by default unless --enable-runtime-link-image is set`. Hopefully that is OK as well. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2151859093
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
On Thu, 6 Jun 2024 10:42:20 GMT, Alan Bateman wrote: >> Severin Gehwolf has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix default description of keep-packaged-modules > > I've read through all src changes. I think Sundar is looking at the code > changes too. > > The overall design seems solid. I know it took a long time to get there but > this is nature of a feature like this. At this point I regret that there > isn't a JEP. A JEP would have captured the motivation, outlined the design, > the reasoning for the restrictions, and document the design > choices/directions that have been prototyped. If there isn't a JEP then I > suppose it can come later if the feature is progressed and there is > discussion about making it the default rather than opt-in at build configure > time. > > As cleanup, I think we will need to bring some consistency to the terminology > and phrasing in documentation, code and comments. Right now there is > "run-time linking", "linkable run-time", "run-time linkable JDK image", > "linkable jimage". > > Also as cleanup, I think the code needs more comments. There is no JEP right > now with an authoritive description of the feature so anyone maintaining this > code will have to figure out a lot of details. It feels like there should be > somehting that documents the effect of --enable-runtime-link-image, how the > diffs are generated and saved, and how they are used by jlink. There is also > a lot of new code in ImageFileCreator and JlinkTask that is asking for some > method descriptions so that anyone touching this code can quickly understand > what these methods are doing. I don't want to get into code style issues now > but I think it would be helpful for future maintainers to avoid more of the > overfly long lines if possible (some of them are 150, 160, 170+ and really > hard to look at code side-by-side). @AlanBateman Sure, I appreciate the feedback. Do I understand it correctly that this won't get into JDK 23 then? If so, perhaps the better way would be to draft a JEP for JDK 24 and get that proposed early. What I'd like to avoid is for this change to don't see much movement for a long time between now and RDP 1 of JDK 24 and have a similar crunch when JDK 24 is close to forking. You mention clean-up a lot. Is that suggesting it *can* go into JDK 23 and clean-up in ramp-down? I'm confused... Some clarity on how to best approach getting this integrated that would be acceptable for all involved would be appreciated. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2152248595
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
On Thu, 6 Jun 2024 09:47:30 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Fix default description of keep-packaged-modules OK. I'm aware about the timelines for JDK 23. Thanks for bringing clarity to this. My aim will be to bring this into JDK 24 with a JEP then. Hopefully we can bring this to a successful conclusion that way. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2152833052
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v4]
> Please review this enhancement to the container detection code which allows > it to figure out whether the JVM is actually running inside a container > (`podman`, `docker`, `crio`), or with some other means that enforces > memory/cpu limits by means of the cgroup filesystem. If neither of those > conditions hold, the JVM runs in not containerized mode, addressing the issue > described in the JBS tracker. For example, on my Linux system > `is_containerized() == false" is being indicated with the following trace log > line: > > > [0.001s][debug][os,container] OSContainer::init: is_containerized() = false > because no cpu or memory limit is present > > > This state is being exposed by the Java `Metrics` API class using the new > (still JDK internal) `isContainerized()` method. Example: > > > java -XshowSettings:system --version > Operating System Metrics: > Provider: cgroupv1 > System not containerized. > openjdk 23-internal 2024-09-17 > OpenJDK Runtime Environment (fastdebug build > 23-internal-adhoc.sgehwolf.jdk-jdk) > OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk, > mixed mode, sharing) > > > The basic property this is being built on is the observation that the cgroup > controllers typically get mounted read only into containers. Note that the > current container tests assert that `OSContainer::is_containerized() == true` > in various tests. Therefore, using the heuristic of "is any memory or cpu > limit present" isn't sufficient. I had considered that in an earlier > iteration, but many container tests failed. > > Overall, I think, with this patch we improve the current situation of > claiming a containerized system being present when it's actually just a > regular Linux system. > > Testing: > > - [x] GHA (risc-v failure seems infra related) > - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 (including > gtests) > - [x] Some manual testing using cri-o > > Thoughts? Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 13 commits: - Merge branch 'master' into jdk-8261242-is-containerized-fix - Add doc for mountinfo scanning. - Unify naming of variables - Merge branch 'master' into jdk-8261242-is-containerized-fix - Merge branch 'master' into jdk-8261242-is-containerized-fix - jcheck fixes - Fix tests - Implement Metrics.isContainerized() - Some clean-up - Drop cgroups testing on plain Linux - ... and 3 more: https://git.openjdk.org/jdk/compare/40b2fbd8...02884c70 - Changes: https://git.openjdk.org/jdk/pull/18201/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18201&range=03 Stats: 406 lines in 19 files changed: 301 ins; 78 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/18201.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18201/head:pull/18201 PR: https://git.openjdk.org/jdk/pull/18201
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v4]
On Fri, 7 Jun 2024 12:59:26 GMT, Severin Gehwolf wrote: >> Please review this enhancement to the container detection code which allows >> it to figure out whether the JVM is actually running inside a container >> (`podman`, `docker`, `crio`), or with some other means that enforces >> memory/cpu limits by means of the cgroup filesystem. If neither of those >> conditions hold, the JVM runs in not containerized mode, addressing the >> issue described in the JBS tracker. For example, on my Linux system >> `is_containerized() == false" is being indicated with the following trace >> log line: >> >> >> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false >> because no cpu or memory limit is present >> >> >> This state is being exposed by the Java `Metrics` API class using the new >> (still JDK internal) `isContainerized()` method. Example: >> >> >> java -XshowSettings:system --version >> Operating System Metrics: >> Provider: cgroupv1 >> System not containerized. >> openjdk 23-internal 2024-09-17 >> OpenJDK Runtime Environment (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing) >> >> >> The basic property this is being built on is the observation that the cgroup >> controllers typically get mounted read only into containers. Note that the >> current container tests assert that `OSContainer::is_containerized() == >> true` in various tests. Therefore, using the heuristic of "is any memory or >> cpu limit present" isn't sufficient. I had considered that in an earlier >> iteration, but many container tests failed. >> >> Overall, I think, with this patch we improve the current situation of >> claiming a containerized system being present when it's actually just a >> regular Linux system. >> >> Testing: >> >> - [x] GHA (risc-v failure seems infra related) >> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 >> (including gtests) >> - [x] Some manual testing using cri-o >> >> Thoughts? > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 13 commits: > > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Add doc for mountinfo scanning. > - Unify naming of variables > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - jcheck fixes > - Fix tests > - Implement Metrics.isContainerized() > - Some clean-up > - Drop cgroups testing on plain Linux > - ... and 3 more: https://git.openjdk.org/jdk/compare/40b2fbd8...02884c70 [tools/javac/annotations/typeAnnotations/api/ArrayCreationTree](https://github.com/jerboaa/jdk/actions/runs/9417350160#user-content-tools_javac_annotations_typeannotations_api_arraycreationtree) test failure in GHA on 32 bit Linux seems unrelated. - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2162580613
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
On Thu, 6 Jun 2024 15:35:20 GMT, Severin Gehwolf wrote: > My aim will be to bring this into JDK 24 with a JEP then. Hopefully we can > bring this to a successful conclusion that way. @AlanBateman JEP draft is here: https://openjdk.org/jeps/8333799 Could you please help review it? Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2163504796
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
On Fri, 14 Jun 2024 06:49:34 GMT, Alan Bateman wrote: > Yes, on my list. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2167591811
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v11]
On Wed, 22 May 2024 15:02:18 GMT, Jan Kratochvil wrote: >> The testcase requires root permissions. >> >> Designed by Severin Gehwolf, implemented by Jan Kratochvil. > > Jan Kratochvil has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 74 commits: > > - . > - . > - Merge branch 'jdk-8331560-cgroup-controller-delegation-merge-diamond' into > jdk-8331560-cgroup-controller-delegation-merge-cgroup > - diamond > - Merge branch 'jerboaarefactor-merge-cgroup' into > jdk-8331560-cgroup-controller-delegation-merge-cgroup > - Merge branch 'jerboaarefactor-merge' into jerboaarefactor-merge-cgroup > - Merge branch 'master' into jerboaarefactor-merge > - whitespace fix > - centos7 compat > - 64a5feb6: > - ... and 64 more: https://git.openjdk.org/jdk/compare/3d511ff6...25c0287d Keep open. - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2179330671
Re: RFR: 8333446: Add tests for hierarchical container support [v2]
> Please review this PR which adds test support for systemd slices so that bugs > like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be > verified. The added test, `SystemdMemoryAwarenessTest` currently passes on > cgroups v1 and fails on cgroups v2 due to the way how > [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented > when JDK 13 was a thing. Therefore immediately problem-listed. It should get > unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) > merges. > > I'm adding those tests in order to not regress another time. > > Testing: > - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. > - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to JDK-8322420) > - [x] GHA Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Fix comments - 8333446: Add tests for hierarchical container support - Changes: - all: https://git.openjdk.org/jdk/pull/19530/files - new: https://git.openjdk.org/jdk/pull/19530/files/98d780ac..00b528ae Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=00-01 Stats: 45352 lines in 1129 files changed: 26950 ins; 13694 del; 4708 mod Patch: https://git.openjdk.org/jdk/pull/19530.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19530/head:pull/19530 PR: https://git.openjdk.org/jdk/pull/19530
Re: RFR: 8333446: Add tests for hierarchical container support [v2]
On Thu, 20 Jun 2024 08:34:45 GMT, Severin Gehwolf wrote: >> Please review this PR which adds test support for systemd slices so that >> bugs like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be >> verified. The added test, `SystemdMemoryAwarenessTest` currently passes on >> cgroups v1 and fails on cgroups v2 due to the way how >> [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented >> when JDK 13 was a thing. Therefore immediately problem-listed. It should get >> unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) >> merges. >> >> I'm adding those tests in order to not regress another time. >> >> Testing: >> - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. >> - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to >> JDK-8322420) >> - [x] GHA > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Fix comments > - 8333446: Add tests for hierarchical container support Anyone willing to review this? - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2180477503
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v5]
> Please review this enhancement to the container detection code which allows > it to figure out whether the JVM is actually running inside a container > (`podman`, `docker`, `crio`), or with some other means that enforces > memory/cpu limits by means of the cgroup filesystem. If neither of those > conditions hold, the JVM runs in not containerized mode, addressing the issue > described in the JBS tracker. For example, on my Linux system > `is_containerized() == false" is being indicated with the following trace log > line: > > > [0.001s][debug][os,container] OSContainer::init: is_containerized() = false > because no cpu or memory limit is present > > > This state is being exposed by the Java `Metrics` API class using the new > (still JDK internal) `isContainerized()` method. Example: > > > java -XshowSettings:system --version > Operating System Metrics: > Provider: cgroupv1 > System not containerized. > openjdk 23-internal 2024-09-17 > OpenJDK Runtime Environment (fastdebug build > 23-internal-adhoc.sgehwolf.jdk-jdk) > OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk, > mixed mode, sharing) > > > The basic property this is being built on is the observation that the cgroup > controllers typically get mounted read only into containers. Note that the > current container tests assert that `OSContainer::is_containerized() == true` > in various tests. Therefore, using the heuristic of "is any memory or cpu > limit present" isn't sufficient. I had considered that in an earlier > iteration, but many container tests failed. > > Overall, I think, with this patch we improve the current situation of > claiming a containerized system being present when it's actually just a > regular Linux system. > > Testing: > > - [x] GHA (risc-v failure seems infra related) > - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 (including > gtests) > - [x] Some manual testing using cri-o > > Thoughts? Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 14 commits: - Merge branch 'master' into jdk-8261242-is-containerized-fix - Merge branch 'master' into jdk-8261242-is-containerized-fix - Add doc for mountinfo scanning. - Unify naming of variables - Merge branch 'master' into jdk-8261242-is-containerized-fix - Merge branch 'master' into jdk-8261242-is-containerized-fix - jcheck fixes - Fix tests - Implement Metrics.isContainerized() - Some clean-up - ... and 4 more: https://git.openjdk.org/jdk/compare/01ee4241...7c163cb2 - Changes: https://git.openjdk.org/jdk/pull/18201/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18201&range=04 Stats: 406 lines in 19 files changed: 301 ins; 78 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/18201.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18201/head:pull/18201 PR: https://git.openjdk.org/jdk/pull/18201
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v6]
> Please review this enhancement to the container detection code which allows > it to figure out whether the JVM is actually running inside a container > (`podman`, `docker`, `crio`), or with some other means that enforces > memory/cpu limits by means of the cgroup filesystem. If neither of those > conditions hold, the JVM runs in not containerized mode, addressing the issue > described in the JBS tracker. For example, on my Linux system > `is_containerized() == false" is being indicated with the following trace log > line: > > > [0.001s][debug][os,container] OSContainer::init: is_containerized() = false > because no cpu or memory limit is present > > > This state is being exposed by the Java `Metrics` API class using the new > (still JDK internal) `isContainerized()` method. Example: > > > java -XshowSettings:system --version > Operating System Metrics: > Provider: cgroupv1 > System not containerized. > openjdk 23-internal 2024-09-17 > OpenJDK Runtime Environment (fastdebug build > 23-internal-adhoc.sgehwolf.jdk-jdk) > OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk, > mixed mode, sharing) > > > The basic property this is being built on is the observation that the cgroup > controllers typically get mounted read only into containers. Note that the > current container tests assert that `OSContainer::is_containerized() == true` > in various tests. Therefore, using the heuristic of "is any memory or cpu > limit present" isn't sufficient. I had considered that in an earlier > iteration, but many container tests failed. > > Overall, I think, with this patch we improve the current situation of > claiming a containerized system being present when it's actually just a > regular Linux system. > > Testing: > > - [x] GHA (risc-v failure seems infra related) > - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 (including > gtests) > - [x] Some manual testing using cri-o > > Thoughts? Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision: Remove problem listing of PlainRead which is reworked here - Changes: - all: https://git.openjdk.org/jdk/pull/18201/files - new: https://git.openjdk.org/jdk/pull/18201/files/7c163cb2..3d98cbc2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18201&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18201&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18201.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18201/head:pull/18201 PR: https://git.openjdk.org/jdk/pull/18201
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Tue, 16 Apr 2024 18:25:52 GMT, Thomas Stuefe wrote: >> Severin Gehwolf has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains ten additional >> commits since the last revision: >> >> - Merge branch 'master' into jdk-8261242-is-containerized-fix >> - jcheck fixes >> - Fix tests >> - Implement Metrics.isContainerized() >> - Some clean-up >> - Drop cgroups testing on plain Linux >> - Implement fall-back logic for non-ro controller mounts >> - Make find_ro static and local to compilation unit >> - 8261242: [Linux] OSContainer::is_containerized() returns true > > I am not enough of a container expert to judge if the basic approach is right > - I trust you on this. This is just a technical code review. @tstuefe Do you mind to take another look? Thank you! - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2180504024
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v33]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 137 commits: - JLinkDedupTestBatchSizeOne.java test fix Run only the packaged modules version if we have a linkable JDK runtime with packaged modules available as well in the JDK install. - Enable IncludeLocalesPluginTest - Enable GenerateJLIClassesPluginTest.java test - Enable JLinkReproducibleTest.java for linkable JDK images - Remove restriction on directory based modules Enable the following tests: - JLink100Modules.java - JLinkDedupTestBatchSizeOne.java - Comment fix in JRTArchive. Long line in JlinkTask - Comment fixes in ImageFileCreator - Comments and clean-up in JlinkTask - Helper support for linkable JDK runtimes - Test clean-up. class-file API module. - ... and 127 more: https://git.openjdk.org/jdk/compare/5cad0b4d...04cd98f8 - Changes: https://git.openjdk.org/jdk/pull/14787/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=32 Stats: 3959 lines in 42 files changed: 3762 ins; 117 del; 80 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v6]
On Tue, 25 Jun 2024 13:39:07 GMT, Jan Kratochvil wrote: > Currently this patch conflicts a lot with #19085 > (jerboaa:jdk-8331560-cgroup-controller-delegation). Yes, I'll resolve it one way or another depending on which one goes in first. - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2189001364
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v7]
> Please review this enhancement to the container detection code which allows > it to figure out whether the JVM is actually running inside a container > (`podman`, `docker`, `crio`), or with some other means that enforces > memory/cpu limits by means of the cgroup filesystem. If neither of those > conditions hold, the JVM runs in not containerized mode, addressing the issue > described in the JBS tracker. For example, on my Linux system > `is_containerized() == false" is being indicated with the following trace log > line: > > > [0.001s][debug][os,container] OSContainer::init: is_containerized() = false > because no cpu or memory limit is present > > > This state is being exposed by the Java `Metrics` API class using the new > (still JDK internal) `isContainerized()` method. Example: > > > java -XshowSettings:system --version > Operating System Metrics: > Provider: cgroupv1 > System not containerized. > openjdk 23-internal 2024-09-17 > OpenJDK Runtime Environment (fastdebug build > 23-internal-adhoc.sgehwolf.jdk-jdk) > OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk, > mixed mode, sharing) > > > The basic property this is being built on is the observation that the cgroup > controllers typically get mounted read only into containers. Note that the > current container tests assert that `OSContainer::is_containerized() == true` > in various tests. Therefore, using the heuristic of "is any memory or cpu > limit present" isn't sufficient. I had considered that in an earlier > iteration, but many container tests failed. > > Overall, I think, with this patch we improve the current situation of > claiming a containerized system being present when it's actually just a > regular Linux system. > > Testing: > > - [x] GHA (risc-v failure seems infra related) > - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 (including > gtests) > - [x] Some manual testing using cri-o > > Thoughts? Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 17 commits: - Refactor mount info matching to helper function - Merge branch 'master' into jdk-8261242-is-containerized-fix - Remove problem listing of PlainRead which is reworked here - Merge branch 'master' into jdk-8261242-is-containerized-fix - Merge branch 'master' into jdk-8261242-is-containerized-fix - Add doc for mountinfo scanning. - Unify naming of variables - Merge branch 'master' into jdk-8261242-is-containerized-fix - Merge branch 'master' into jdk-8261242-is-containerized-fix - jcheck fixes - ... and 7 more: https://git.openjdk.org/jdk/compare/baafa662...532ea33b - Changes: https://git.openjdk.org/jdk/pull/18201/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18201&range=06 Stats: 411 lines in 20 files changed: 305 ins; 79 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/18201.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18201/head:pull/18201 PR: https://git.openjdk.org/jdk/pull/18201
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v6]
On Thu, 20 Jun 2024 17:37:05 GMT, Thomas Stuefe wrote:
>> Severin Gehwolf has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove problem listing of PlainRead which is reworked here
>
> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 422:
>
>> 420: * (12) super options: matched with '%s' and captured in
>> 'tmpcgroups'
>> 421: */
>> 422: if (sscanf(p, "%*d %*d %*d:%*d %s %s %s%*[^-]- %s %*s %s", tmproot,
>> tmpmount, mount_opts, tmp_fs_type, tmpcgroups) == 5) {
>
> The only difference to v1 is that we parse the super options (12), right?
> Could we factor out the parsing into a helper function? Or, alternatively, at
> least `#define` the scanf format somewhere up top, including the nice
> comment, and reuse that format string?
That's correct. I've moved it into a local helper function. Thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1652863523
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v33]
On Mon, 24 Jun 2024 14:33:51 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 137 commits: > > - JLinkDedupTestBatchSizeOne.java test fix > >Run only the packaged modules version if we have a linkable JDK runtime >with packaged modules available as well in the JDK install. > - Enable IncludeLocalesPluginTest > - Enable GenerateJLIClassesPluginTest.java test > - Enable JLinkReproducibleTest.java for linkable JDK images > - Remove restriction on directory based modules > >Enable the following tests: >- JLink100Modules.java >- JLinkDedupTestBatchSizeOne.java > - Comment fix in JRTArchive. Long line in JlinkTask > - Comment fixes in ImageFileCreator > - Comments and clean-up in JlinkTask > - Helper support for linkable JDK runtimes > - Test clean-up. class-file API module. > - ... and 127 more: https://git.openjdk.org/jdk/compare/5cad0b4d...04cd98f8 I've pushed some clean-up fixes to this PR so as to fix the overly long lines and added comments to relevant methods. The latest GHA failure on MacOSX x86_64 is infra related. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2189072464
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v7]
On Tue, 25 Jun 2024 13:54:46 GMT, Severin Gehwolf wrote: >> Please review this enhancement to the container detection code which allows >> it to figure out whether the JVM is actually running inside a container >> (`podman`, `docker`, `crio`), or with some other means that enforces >> memory/cpu limits by means of the cgroup filesystem. If neither of those >> conditions hold, the JVM runs in not containerized mode, addressing the >> issue described in the JBS tracker. For example, on my Linux system >> `is_containerized() == false" is being indicated with the following trace >> log line: >> >> >> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false >> because no cpu or memory limit is present >> >> >> This state is being exposed by the Java `Metrics` API class using the new >> (still JDK internal) `isContainerized()` method. Example: >> >> >> java -XshowSettings:system --version >> Operating System Metrics: >> Provider: cgroupv1 >> System not containerized. >> openjdk 23-internal 2024-09-17 >> OpenJDK Runtime Environment (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing) >> >> >> The basic property this is being built on is the observation that the cgroup >> controllers typically get mounted read only into containers. Note that the >> current container tests assert that `OSContainer::is_containerized() == >> true` in various tests. Therefore, using the heuristic of "is any memory or >> cpu limit present" isn't sufficient. I had considered that in an earlier >> iteration, but many container tests failed. >> >> Overall, I think, with this patch we improve the current situation of >> claiming a containerized system being present when it's actually just a >> regular Linux system. >> >> Testing: >> >> - [x] GHA (risc-v failure seems infra related) >> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 >> (including gtests) >> - [x] Some manual testing using cri-o >> >> Thoughts? > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 17 commits: > > - Refactor mount info matching to helper function > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Remove problem listing of PlainRead which is reworked here > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Add doc for mountinfo scanning. > - Unify naming of variables > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - jcheck fixes > - ... and 7 more: https://git.openjdk.org/jdk/compare/baafa662...532ea33b Could I get a second review on this please? @larry-cable maybe? Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-219133
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v7]
On Tue, 25 Jun 2024 13:54:46 GMT, Severin Gehwolf wrote: >> Please review this enhancement to the container detection code which allows >> it to figure out whether the JVM is actually running inside a container >> (`podman`, `docker`, `crio`), or with some other means that enforces >> memory/cpu limits by means of the cgroup filesystem. If neither of those >> conditions hold, the JVM runs in not containerized mode, addressing the >> issue described in the JBS tracker. For example, on my Linux system >> `is_containerized() == false" is being indicated with the following trace >> log line: >> >> >> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false >> because no cpu or memory limit is present >> >> >> This state is being exposed by the Java `Metrics` API class using the new >> (still JDK internal) `isContainerized()` method. Example: >> >> >> java -XshowSettings:system --version >> Operating System Metrics: >> Provider: cgroupv1 >> System not containerized. >> openjdk 23-internal 2024-09-17 >> OpenJDK Runtime Environment (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing) >> >> >> The basic property this is being built on is the observation that the cgroup >> controllers typically get mounted read only into containers. Note that the >> current container tests assert that `OSContainer::is_containerized() == >> true` in various tests. Therefore, using the heuristic of "is any memory or >> cpu limit present" isn't sufficient. I had considered that in an earlier >> iteration, but many container tests failed. >> >> Overall, I think, with this patch we improve the current situation of >> claiming a containerized system being present when it's actually just a >> regular Linux system. >> >> Testing: >> >> - [x] GHA (risc-v failure seems infra related) >> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 >> (including gtests) >> - [x] Some manual testing using cri-o >> >> Thoughts? > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 17 commits: > > - Refactor mount info matching to helper function > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Remove problem listing of PlainRead which is reworked here > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Add doc for mountinfo scanning. > - Unify naming of variables > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - jcheck fixes > - ... and 7 more: https://git.openjdk.org/jdk/compare/baafa662...532ea33b Thanks for the review! - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2196421487
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v7]
On Thu, 27 Jun 2024 18:40:09 GMT, Larry Cable wrote:
>> Severin Gehwolf has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 17 commits:
>>
>> - Refactor mount info matching to helper function
>> - Merge branch 'master' into jdk-8261242-is-containerized-fix
>> - Remove problem listing of PlainRead which is reworked here
>> - Merge branch 'master' into jdk-8261242-is-containerized-fix
>> - Merge branch 'master' into jdk-8261242-is-containerized-fix
>> - Add doc for mountinfo scanning.
>> - Unify naming of variables
>> - Merge branch 'master' into jdk-8261242-is-containerized-fix
>> - Merge branch 'master' into jdk-8261242-is-containerized-fix
>> - jcheck fixes
>> - ... and 7 more: https://git.openjdk.org/jdk/compare/baafa662...532ea33b
>
> src/hotspot/share/prims/jvm.cpp line 504:
>
>> 502: JVM_LEAF(jboolean, JVM_IsContainerized(void))
>> 503: #ifdef LINUX
>> 504: if (OSContainer::is_containerized()) {
>
> // nit: personal preference...
>
> return OSContainer::isContainerized() ? JNI_TRUE : JNI_FALSE;
I've kept this as is, since the suggestion generates this code after
preprocessing on Linux:
return OSContainer::is_containerized() ? JNI_TRUE : JNI_FALSE;
return JNI_FALSE;
over the existing version:
if (OSContainer::is_containerized()) {
return JNI_TRUE;
}
return JNI_FALSE;
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1658938198
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v8]
> Please review this enhancement to the container detection code which allows > it to figure out whether the JVM is actually running inside a container > (`podman`, `docker`, `crio`), or with some other means that enforces > memory/cpu limits by means of the cgroup filesystem. If neither of those > conditions hold, the JVM runs in not containerized mode, addressing the issue > described in the JBS tracker. For example, on my Linux system > `is_containerized() == false" is being indicated with the following trace log > line: > > > [0.001s][debug][os,container] OSContainer::init: is_containerized() = false > because no cpu or memory limit is present > > > This state is being exposed by the Java `Metrics` API class using the new > (still JDK internal) `isContainerized()` method. Example: > > > java -XshowSettings:system --version > Operating System Metrics: > Provider: cgroupv1 > System not containerized. > openjdk 23-internal 2024-09-17 > OpenJDK Runtime Environment (fastdebug build > 23-internal-adhoc.sgehwolf.jdk-jdk) > OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk, > mixed mode, sharing) > > > The basic property this is being built on is the observation that the cgroup > controllers typically get mounted read only into containers. Note that the > current container tests assert that `OSContainer::is_containerized() == true` > in various tests. Therefore, using the heuristic of "is any memory or cpu > limit present" isn't sufficient. I had considered that in an earlier > iteration, but many container tests failed. > > Overall, I think, with this patch we improve the current situation of > claiming a containerized system being present when it's actually just a > regular Linux system. > > Testing: > > - [x] GHA (risc-v failure seems infra related) > - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 (including > gtests) > - [x] Some manual testing using cri-o > > Thoughts? Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 18 commits: - Merge branch 'master' into jdk-8261242-is-containerized-fix - Refactor mount info matching to helper function - Merge branch 'master' into jdk-8261242-is-containerized-fix - Remove problem listing of PlainRead which is reworked here - Merge branch 'master' into jdk-8261242-is-containerized-fix - Merge branch 'master' into jdk-8261242-is-containerized-fix - Add doc for mountinfo scanning. - Unify naming of variables - Merge branch 'master' into jdk-8261242-is-containerized-fix - Merge branch 'master' into jdk-8261242-is-containerized-fix - ... and 8 more: https://git.openjdk.org/jdk/compare/486aa11e...1017da35 - Changes: https://git.openjdk.org/jdk/pull/18201/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18201&range=07 Stats: 411 lines in 20 files changed: 305 ins; 79 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/18201.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18201/head:pull/18201 PR: https://git.openjdk.org/jdk/pull/18201
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v8]
On Fri, 28 Jun 2024 15:41:48 GMT, Severin Gehwolf wrote: >> Please review this enhancement to the container detection code which allows >> it to figure out whether the JVM is actually running inside a container >> (`podman`, `docker`, `crio`), or with some other means that enforces >> memory/cpu limits by means of the cgroup filesystem. If neither of those >> conditions hold, the JVM runs in not containerized mode, addressing the >> issue described in the JBS tracker. For example, on my Linux system >> `is_containerized() == false" is being indicated with the following trace >> log line: >> >> >> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false >> because no cpu or memory limit is present >> >> >> This state is being exposed by the Java `Metrics` API class using the new >> (still JDK internal) `isContainerized()` method. Example: >> >> >> java -XshowSettings:system --version >> Operating System Metrics: >> Provider: cgroupv1 >> System not containerized. >> openjdk 23-internal 2024-09-17 >> OpenJDK Runtime Environment (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing) >> >> >> The basic property this is being built on is the observation that the cgroup >> controllers typically get mounted read only into containers. Note that the >> current container tests assert that `OSContainer::is_containerized() == >> true` in various tests. Therefore, using the heuristic of "is any memory or >> cpu limit present" isn't sufficient. I had considered that in an earlier >> iteration, but many container tests failed. >> >> Overall, I think, with this patch we improve the current situation of >> claiming a containerized system being present when it's actually just a >> regular Linux system. >> >> Testing: >> >> - [x] GHA (risc-v failure seems infra related) >> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 >> (including gtests) >> - [x] Some manual testing using cri-o >> >> Thoughts? > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 18 commits: > > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Refactor mount info matching to helper function > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Remove problem listing of PlainRead which is reworked here > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Add doc for mountinfo scanning. > - Unify naming of variables > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - ... and 8 more: https://git.openjdk.org/jdk/compare/486aa11e...1017da35 @adinn @iklam Could one of you please help with a second review, please? Not sure if @larry-cable review gets recorded with him having no OpenJDK project role :-/ Thanks in advance! - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2197212014
Integrated: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container
On Mon, 11 Mar 2024 16:55:36 GMT, Severin Gehwolf wrote: > Please review this enhancement to the container detection code which allows > it to figure out whether the JVM is actually running inside a container > (`podman`, `docker`, `crio`), or with some other means that enforces > memory/cpu limits by means of the cgroup filesystem. If neither of those > conditions hold, the JVM runs in not containerized mode, addressing the issue > described in the JBS tracker. For example, on my Linux system > `is_containerized() == false" is being indicated with the following trace log > line: > > > [0.001s][debug][os,container] OSContainer::init: is_containerized() = false > because no cpu or memory limit is present > > > This state is being exposed by the Java `Metrics` API class using the new > (still JDK internal) `isContainerized()` method. Example: > > > java -XshowSettings:system --version > Operating System Metrics: > Provider: cgroupv1 > System not containerized. > openjdk 23-internal 2024-09-17 > OpenJDK Runtime Environment (fastdebug build > 23-internal-adhoc.sgehwolf.jdk-jdk) > OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk, > mixed mode, sharing) > > > The basic property this is being built on is the observation that the cgroup > controllers typically get mounted read only into containers. Note that the > current container tests assert that `OSContainer::is_containerized() == true` > in various tests. Therefore, using the heuristic of "is any memory or cpu > limit present" isn't sufficient. I had considered that in an earlier > iteration, but many container tests failed. > > Overall, I think, with this patch we improve the current situation of > claiming a containerized system being present when it's actually just a > regular Linux system. > > Testing: > > - [x] GHA (risc-v failure seems infra related) > - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 (including > gtests) > - [x] Some manual testing using cri-o > > Thoughts? This pull request has now been integrated. Changeset: 0a6ffa57 Author:Severin Gehwolf URL: https://git.openjdk.org/jdk/commit/0a6ffa57954ddf4f92205205a5a1bada813d127a Stats: 411 lines in 20 files changed: 305 ins; 79 del; 27 mod 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container Reviewed-by: stuefe, iklam - PR: https://git.openjdk.org/jdk/pull/18201
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v8]
On Fri, 28 Jun 2024 15:41:48 GMT, Severin Gehwolf wrote: >> Please review this enhancement to the container detection code which allows >> it to figure out whether the JVM is actually running inside a container >> (`podman`, `docker`, `crio`), or with some other means that enforces >> memory/cpu limits by means of the cgroup filesystem. If neither of those >> conditions hold, the JVM runs in not containerized mode, addressing the >> issue described in the JBS tracker. For example, on my Linux system >> `is_containerized() == false" is being indicated with the following trace >> log line: >> >> >> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false >> because no cpu or memory limit is present >> >> >> This state is being exposed by the Java `Metrics` API class using the new >> (still JDK internal) `isContainerized()` method. Example: >> >> >> java -XshowSettings:system --version >> Operating System Metrics: >> Provider: cgroupv1 >> System not containerized. >> openjdk 23-internal 2024-09-17 >> OpenJDK Runtime Environment (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing) >> >> >> The basic property this is being built on is the observation that the cgroup >> controllers typically get mounted read only into containers. Note that the >> current container tests assert that `OSContainer::is_containerized() == >> true` in various tests. Therefore, using the heuristic of "is any memory or >> cpu limit present" isn't sufficient. I had considered that in an earlier >> iteration, but many container tests failed. >> >> Overall, I think, with this patch we improve the current situation of >> claiming a containerized system being present when it's actually just a >> regular Linux system. >> >> Testing: >> >> - [x] GHA (risc-v failure seems infra related) >> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 >> (including gtests) >> - [x] Some manual testing using cri-o >> >> Thoughts? > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 18 commits: > > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Refactor mount info matching to helper function > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Remove problem listing of PlainRead which is reworked here > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Add doc for mountinfo scanning. > - Unify naming of variables > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - ... and 8 more: https://git.openjdk.org/jdk/compare/486aa11e...1017da35 Thank you for the review! - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2199581201
Re: RFR: 8333446: Add tests for hierarchical container support [v3]
> Please review this PR which adds test support for systemd slices so that bugs > like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be > verified. The added test, `SystemdMemoryAwarenessTest` currently passes on > cgroups v1 and fails on cgroups v2 due to the way how > [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented > when JDK 13 was a thing. Therefore immediately problem-listed. It should get > unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) > merges. > > I'm adding those tests in order to not regress another time. > > Testing: > - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. > - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to JDK-8322420) > - [x] GHA Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Fix comments - 8333446: Add tests for hierarchical container support - Changes: - all: https://git.openjdk.org/jdk/pull/19530/files - new: https://git.openjdk.org/jdk/pull/19530/files/00b528ae..22141a48 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=01-02 Stats: 26334 lines in 522 files changed: 18610 ins; 5830 del; 1894 mod Patch: https://git.openjdk.org/jdk/pull/19530.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19530/head:pull/19530 PR: https://git.openjdk.org/jdk/pull/19530
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v9]
On Wed, 8 May 2024 03:00:50 GMT, Jan Kratochvil wrote: >> Jan Kratochvil has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 49 commits: >> >> - centos7 compat >> - 64a5feb6: >> - fixes >> - e514824f: >> - ebb459e9: >> - Merge branch 'jerboaarefactor-merge' into jerboaarefactor-merge-cgroup >> - Merge branch 'jerboaarefactor' into jerboaarefactor-merge >> - Merge remote-tracking branch 'origin/master' into jerboaarefactor >> - Merge remote-tracking branch 'origin/master' into jerboaarefactor-merge >> - Merge branch 'master-cgroup' into jerboaarefactor-merge-cgroup >> - ... and 39 more: https://git.openjdk.org/jdk/compare/9347bb7d...3da3a9e5 > > Your patch > https://github.com/jerboaa/jdk/commit/92aaa6fd7e3ff8b64de064fecfcd725a157cb5bb#diff-1c49a6b40a810aef82b7da9bfea8f03e07a43062977ba65f75df63c4b7c5b149R89 > contains a tab. @jankratochvil Please merge master and try to use the new code from https://bugs.openjdk.org/browse/JDK-8331560 to query the limits. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2203599274
Re: RFR: 8333446: Add tests for hierarchical container support [v3]
On Mon, 1 Jul 2024 14:43:58 GMT, Severin Gehwolf wrote: >> Please review this PR which adds test support for systemd slices so that >> bugs like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be >> verified. The added test, `SystemdMemoryAwarenessTest` currently passes on >> cgroups v1 and fails on cgroups v2 due to the way how >> [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented >> when JDK 13 was a thing. Therefore immediately problem-listed. It should get >> unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) >> merges. >> >> I'm adding those tests in order to not regress another time. >> >> Testing: >> - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. >> - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to >> JDK-8322420) >> - [x] GHA > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Fix comments > - 8333446: Add tests for hierarchical container support Gentle ping. - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2209259086
RFR: 8335882: platform/cgroup/TestSystemSettings.java fails on Alpine Linux
Please review this simple test fix to exclude the test from being run on an Alpine Linux system. Apparently default Alpine Linux systems don't have cgroups set up by default the way other distros do. More info on the bug. I propose to not run the test on musl systems. - Commit messages: - 8335882: platform/cgroup/TestSystemSettings.java fails on Alpine Linux Changes: https://git.openjdk.org/jdk/pull/20076/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20076&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335882 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20076.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20076/head:pull/20076 PR: https://git.openjdk.org/jdk/pull/20076
Re: RFR: 8335882: platform/cgroup/TestSystemSettings.java fails on Alpine Linux
On Mon, 8 Jul 2024 14:19:21 GMT, Severin Gehwolf wrote: > Please review this simple test fix to exclude the test from being run on an > Alpine Linux system. Apparently default Alpine Linux systems don't have > cgroups set up by default the way other distros do. More info on the bug. I > propose to not run the test on musl systems. @MBaesken Since you've noticed the failing test in your CI could you please verify the issue is gone with this? I don't have an Alpine Linux setup to easily test this exclusion. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/20076#issuecomment-2214223234
Re: RFR: 8335882: platform/cgroup/TestSystemSettings.java fails on Alpine Linux
On Mon, 8 Jul 2024 14:26:29 GMT, Matthias Baesken wrote: > Hi Severin, sure ! I put it into our build/test setup . Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/20076#issuecomment-2214368557
Re: RFR: 8335882: platform/cgroup/TestSystemSettings.java fails on Alpine Linux
On Mon, 8 Jul 2024 14:19:21 GMT, Severin Gehwolf wrote: > Please review this simple test fix to exclude the test from being run on an > Alpine Linux system. Apparently default Alpine Linux systems don't have > cgroups set up by default the way other distros do. More info on the bug. I > propose to not run the test on musl systems. Thanks for the reviews! The docs build failure in GHA is infra related: Run # If runner architecture is x64 set JAVA_HOME_17_X64 otherwise set to JAVA_HOME_17_arm64 [build.sh][INFO] CYGWIN_OR_MSYS=0 [build.sh][INFO] JAVA_HOME: /usr/lib/jvm/temurin-17-jdk-amd64 [build.sh][INFO] Downloading https://archive.apache.org/dist/ant/binaries/apache-ant-1.10.8-bin.zip to /home/runner/work/jdk/jdk/jtreg/src/make/../build/deps/apache-ant-1.10.8-bin.zip Error: sh][ERROR] wget exited with exit code 4 Error: Process completed with exit code 1. - PR Comment: https://git.openjdk.org/jdk/pull/20076#issuecomment-2217257118
Integrated: 8335882: platform/cgroup/TestSystemSettings.java fails on Alpine Linux
On Mon, 8 Jul 2024 14:19:21 GMT, Severin Gehwolf wrote: > Please review this simple test fix to exclude the test from being run on an > Alpine Linux system. Apparently default Alpine Linux systems don't have > cgroups set up by default the way other distros do. More info on the bug. I > propose to not run the test on musl systems. This pull request has now been integrated. Changeset: f3ff4f74 Author: Severin Gehwolf URL: https://git.openjdk.org/jdk/commit/f3ff4f7427c3c3f5cb2a115a61462bb9d28de1cd Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8335882: platform/cgroup/TestSystemSettings.java fails on Alpine Linux Reviewed-by: stuefe, mbaesken - PR: https://git.openjdk.org/jdk/pull/20076
Re: RFR: 8333446: Add tests for hierarchical container support [v3]
On Thu, 11 Jul 2024 03:39:37 GMT, Jan Kratochvil wrote: >> Severin Gehwolf has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - Merge branch 'master' into jdk-8333446-systemd-slice-tests >> - Merge branch 'master' into jdk-8333446-systemd-slice-tests >> - Fix comments >> - 8333446: Add tests for hierarchical container support > > test/hotspot/jtreg/ProblemList.txt line 119: > >> 117: containers/docker/TestMemoryAwareness.java 8303470 linux-all >> 118: containers/docker/TestJFREvents.java 8327723 linux-x64 >> 119: containers/systemd/SystemdMemoryAwarenessTest.java 8322420 linux-all > > This line should be removed as long as it gets applied after > [17198](https://github.com/openjdk/jdk/pull/17198). Sure. We need to see which one goes in first and I'll adjust accordingly. - PR Review Comment: https://git.openjdk.org/jdk/pull/19530#discussion_r1673683205
Re: RFR: 8333446: Add tests for hierarchical container support [v3]
On Thu, 11 Jul 2024 03:42:27 GMT, Jan Kratochvil
wrote:
> [test.patch.txt](https://github.com/user-attachments/files/16171122/test.patch.txt)
>
> * `CPUQuota` (changed it to `AllowedCPUs`) does not work for me - it
> properly distributes the load but JDK still sees all available CPU cores (4
> of my VM).
Could you elaborate on that? What does not work? It's relying on the JVM
properly detecting the set limit. `CPUQuota` sets the values in `cpu.max` on
unified hierarchy for the `cpu` controller. See the [systemd
doc](https://www.freedesktop.org/software/systemd/man/latest/systemd.resource-control.html).
It's available since systemd 213. RHEL 7 has 219 which should be good enough.
`AllowedCPUs` on the other hand uses the `cpuset` controller, which is a
different thing. For the purpose of this test, we should use `CPUQuota`.
> * the change 2 -> 1 cores: // We could check 2 cores ("0-1") but then it
> would fail on single-core nodes / virtual machines.
Yeah, we have a chicken/egg problem there. It seemed assuming 2 cores is
reasonable. We could query the number of not restricted CPUs (of the physical
system) using the WB API and take the minimum of the two. Let me work on that.
-
PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-448285
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v12]
On Thu, 11 Jul 2024 06:54:27 GMT, Jan Kratochvil
wrote:
>> The testcase requires root permissions.
>>
>> Designed by Severin Gehwolf, implemented by Jan Kratochvil.
>
> Jan Kratochvil has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 103 commits:
>
> - Fix the gtest
> - fix compilation warning
> - fix the gtest
> - less refactorizations
> - remove not a real backward compat.
> - whitespace
> - less refactorizations
> - reduce refactorizations
> - Fix caching
> - Merge branch 'master' into master-cgroup
> - ... and 93 more: https://git.openjdk.org/jdk/compare/537d20af...060e7688
Conceptually, I think it would be cleaner if we did the "trim" action at
construction time of `Subsystem` on a per controller basis. The path is a
property of the controller. It would be best do do it before caching is set up.
A couple of other comments:
- We should fix Hotspot first (this bug) and do the Metrics (java) change in a
separate patch
- As soon as we have found a lower limit we can stop. Hierarchies which have a
higher limit than the parent are ill-formed, and we can ignore it.
- We ought to also trim the path for the CPU controller. This patch only fixes
the memory controller.
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 836:
> 834:
> 835: void CgroupController::set_path(const char *cgroup_path) {
> 836: __attribute__((unused)) bool _cgroup_path; // Do not use the member
> variable.
This seems an unusual pattern for Hotspot.
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 910:
> 908: memory_swap_limit_min = memory_swap_limit;
> 909: best_level = dir_count;
> 910: }
There is no point in doing memory and memory and swap. Both are controlled by
the same controller. So there is no chance that the paths would differ.
src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 237:
> 235: _metrics_cache = new CachedMetric();
> 236: return controller()->trim_path(dir_count);
> 237: }
We should get away with only doing it for the actual underlying controllers.
I.e. we should do it before caching is set up.
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 78:
> 76: return (jlong)hier_memlimit;
> 77: }
> 78: log_trace(os, container)("Hierarchical Memory Limit is: Unlimited");
It is the hope to no longer needing to do this hierarchical look-up since we
know where in the hierarchy we ought to look for the memory limit.
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 116:
> 114: log_trace(os, container)("Hierarchical Memory and Swap Limit is:
> Unlimited");
> 115: } else {
> 116: return (jlong)hier_memswlimit;
Same here. Hierarchical look-ups shouldn't be needed if we do this correctly.
src/hotspot/os/linux/cgroupV1Subsystem_linux.hpp line 36:
> 34: class CgroupV1Controller: public CgroupController {
> 35: public:
> 36: using CgroupController::CgroupController;
Inheriting constructors are not permitted as per the Hotspot style-guide:
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md#inheriting-constructors
test/hotspot/gtest/runtime/test_cgroupSubsystem_linux.cpp line 1:
> 1: /*
Why are those test changes needed?
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 1:
> 1: /*
I think a more generic solution would be to use
https://bugs.openjdk.org/browse/JDK-8333446 for testing (requires only systemd
vs. this requiring libcg). A hierarchy setup of two limits should be possible
with it too, though I'm doubtful that's needed.
-
PR Review: https://git.openjdk.org/jdk/pull/17198#pullrequestreview-2171534036
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673955179
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673958522
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673952238
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673797261
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673798080
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673795471
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673800271
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673806004
Re: RFR: 8333446: Add tests for hierarchical container support [v3]
On Thu, 11 Jul 2024 14:26:23 GMT, Jan Kratochvil wrote: > > > ``` > > > * `CPUQuota` (changed it to `AllowedCPUs`) does not work for me - it > > > properly distributes the load but JDK still sees all available CPU cores > > > (4 of my VM). > > > ``` > > > > > > Could you elaborate on that? What does not work? > > In the log there is (`/proc/cpuinfo` has 4 entries on this system): > > ``` > [0.139s][trace][os,container] OSContainer::active_processor_count: 4 > ``` > > and therefore it fails with: > > ``` > java.lang.RuntimeException: 'OSContainer::active_processor_count: 2' missing > from stdout/stderr > at > jdk.test.lib.process.OutputAnalyzer.shouldContain(OutputAnalyzer.java:252) > at > SystemdMemoryAwarenessTest.testHelloSystemd(SystemdMemoryAwarenessTest.java:58) > at SystemdMemoryAwarenessTest.main(SystemdMemoryAwarenessTest.java:43) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) > at java.base/java.lang.reflect.Method.invoke(Method.java:580) > at > com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333) > at java.base/java.lang.Thread.run(Thread.java:1575) > ``` > > It is on Fedora 40 x86_64 (`systemd-255.8-1.fc40.x86_64`). Well yes, because the limit isn't properly detected (needs a JVM change that does that; imo https://github.com/openjdk/jdk/pull/17198). - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2223109654
Re: RFR: 8333446: Add tests for hierarchical container support [v4]
> Please review this PR which adds test support for systemd slices so that bugs > like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be > verified. The added test, `SystemdMemoryAwarenessTest` currently passes on > cgroups v1 and fails on cgroups v2 due to the way how > [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented > when JDK 13 was a thing. Therefore immediately problem-listed. It should get > unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) > merges. > > I'm adding those tests in order to not regress another time. > > Testing: > - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. > - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to JDK-8322420) > - [x] GHA Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Add Whitebox check for host cpu - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Fix comments - 8333446: Add tests for hierarchical container support - Changes: - all: https://git.openjdk.org/jdk/pull/19530/files - new: https://git.openjdk.org/jdk/pull/19530/files/22141a48..139a9069 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=02-03 Stats: 13132 lines in 454 files changed: 8669 ins; 2561 del; 1902 mod Patch: https://git.openjdk.org/jdk/pull/19530.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19530/head:pull/19530 PR: https://git.openjdk.org/jdk/pull/19530
Re: RFR: 8333446: Add tests for hierarchical container support [v4]
On Thu, 11 Jul 2024 16:46:13 GMT, Severin Gehwolf wrote: >> Please review this PR which adds test support for systemd slices so that >> bugs like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be >> verified. The added test, `SystemdMemoryAwarenessTest` currently passes on >> cgroups v1 and fails on cgroups v2 due to the way how >> [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented >> when JDK 13 was a thing. Therefore immediately problem-listed. It should get >> unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) >> merges. >> >> I'm adding those tests in order to not regress another time. >> >> Testing: >> - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. >> - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to >> JDK-8322420) >> - [x] GHA > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - Add Whitebox check for host cpu > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Fix comments > - 8333446: Add tests for hierarchical container support Example test run on cgv1 with a fixed JVM: https://cr.openjdk.org/~sgehwolf/webrevs/jdk-8333446-systemd-slice-tests/cgv1/SystemdMemoryAwarenessTest.jtr Example test run on cgv2 with a fixed JVM: https://cr.openjdk.org/~sgehwolf/webrevs/jdk-8333446-systemd-slice-tests/cgv2/SystemdMemoryAwarenessTest.jtr - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2223432957
Re: RFR: 8333446: Add tests for hierarchical container support [v4]
On Fri, 12 Jul 2024 12:28:16 GMT, Jan Kratochvil wrote: > With #17198 and this updated patch I still get the a FAIL due to: > > ``` > [0.333s][trace][os,container] OSContainer::active_processor_count: 4 > ``` > > But let's resolve it after #17198 gets final/approved. Because the #17198 is incomplete. As mentioned in the review: > We ought to also trim the path for the CPU controller. This patch only fixes > the memory controller. That's exactly why the test is failing. - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2225501718
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v12]
On Thu, 11 Jul 2024 06:54:27 GMT, Jan Kratochvil wrote: >> The testcase requires root permissions. >> >> Designed by Severin Gehwolf, implemented by Jan Kratochvil. > > Jan Kratochvil has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 103 commits: > > - Fix the gtest > - fix compilation warning > - fix the gtest > - less refactorizations > - remove not a real backward compat. > - whitespace > - less refactorizations > - reduce refactorizations > - Fix caching > - Merge branch 'master' into master-cgroup > - ... and 93 more: https://git.openjdk.org/jdk/compare/537d20af...060e7688 Feel free to take this alternative fix as inspiration: https://github.com/openjdk/jdk/compare/master...jerboaa:jdk:jdk-8322420_cgroup_hierarchy_walk_init There ought to be a way to remove the duplication between cgv1 and cgv2's impl of `adjust_controller()` but I haven't spent time on that. - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2225563522
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v14]
On Mon, 15 Jul 2024 07:02:11 GMT, Jan Kratochvil wrote: >> The testcase requires root permissions. >> >> Fix by Severin Gehwolf. >> Testcase by Jan Kratochvil. > > Jan Kratochvil has updated the pull request incrementally with one additional > commit since the last revision: > > Fix a needless whitespace change Sorry I cannot review my own patch. You'd need somebody else to help review it. - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2236600764
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v15]
On Thu, 18 Jul 2024 14:48:02 GMT, Jan Kratochvil wrote: >> The testcase requires root permissions. >> >> Fix by Severin Gehwolf. >> Testcase by Jan Kratochvil. > > Jan Kratochvil has updated the pull request incrementally with one additional > commit since the last revision: > > Unify 4 copies of adjust_controller() This patch seems OK (though, I'm biased). Please clean up the test. src/hotspot/os/linux/cgroupUtil_linux.cpp line 64: > 62: return cpu->adjust_controller(cpu_total); > 63: } > 64: return cpu; I guess an alternative - and maybe more readable solution - would be to inline `cpu->adjust_controller()` and `mem->adjust_controller()` code here. We have cg version agnostic api to query the limits. We'd just need accessors for `cgroup_path()` and a setter, `set_cgroup_path()` in `CgroupCpuController/CgroupMemoryController` impls. test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 28: > 26: * @key cgroups > 27: * @requires os.family == "linux" > 28: * @requires vm.flagless If you really want to keep that test, then we should add support for the `libcg` dependency in `jtreg-ext` lib so that we can write `requires os.family == "linux" & dep.libcgroup` or some such. - PR Review: https://git.openjdk.org/jdk/pull/17198#pullrequestreview-2191041991 PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1686225373 PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1686230300
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v15]
On Thu, 18 Jul 2024 14:48:02 GMT, Jan Kratochvil
wrote:
>> The testcase requires root permissions.
>>
>> Fix by Severin Gehwolf.
>> Testcase by Jan Kratochvil.
>
> Jan Kratochvil has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Unify 4 copies of adjust_controller()
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 161:
> 159: System.err.println(LINE_DELIM + " " + (isCgroup2 ? "cgroup2"
> : "cgroup1") + " mount point: " + sysFsCgroup);
> 160: memory_max_filename = isCgroup2 ? "memory.max" :
> "memory.limit_in_bytes";
> 161: Files.writeString(Path.of(sysFsCgroup + "/" + CGROUP_OUTER +
> "/" + memory_max_filename), "" + MEMORY_MAX_OUTER);
This logic doesn't seem to detect `cgv1` and `cgv2` correctly. When I run this
on a cgv1 system (hybrid) then I get a failure that looks like this:
--System.err:(33/2506)--
command: cgdelete -r -g memory:jdktest150899
stdout
stderr
cgdelete: cannot remove group 'jdktest150899': No such file or directory
command: cgcreate -g memory:jdktest150899/inner
stdout
stderr
isCgroup2 = true
cgroup2 mount point: /sys/fs/cgroup/unified
java.nio.file.NoSuchFileException:
/sys/fs/cgroup/unified/jdktest150899/memory.max
at
java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
at
java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:106)
at
java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
at
java.base/sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:262)
at
java.base/java.nio.file.spi.FileSystemProvider.newOutputStream(FileSystemProvider.java:482)
at java.base/java.nio.file.Files.newOutputStream(Files.java:228)
at java.base/java.nio.file.Files.write(Files.java:3516)
at java.base/java.nio.file.Files.writeString(Files.java:3738)
at java.base/java.nio.file.Files.writeString(Files.java:3678)
at NestedCgroup$Test.(NestedCgroup.java:161)
at NestedCgroup$TestTwoLimits.(NestedCgroup.java:190)
at NestedCgroup.main(NestedCgroup.java:221)
at
java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
at java.base/java.lang.reflect.Method.invoke(Method.java:588)
at
com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
at java.base/java.lang.Thread.run(Thread.java:1575)
JavaTest Message: Test threw exception: java.nio.file.NoSuchFileException:
/sys/fs/cgroup/unified/jdktest150899/memory.max
JavaTest Message: shutting down test
I suggest to use code similar to other tests which use the metrics API to
figure out which cgroup version is in use. For example `TestMemoryWithCgroupV1`
has this code snippet which should work just fine here as well:
Metrics metrics = Metrics.systemMetrics();
if (metrics == null) {
System.out.println("TEST PASSED!!!");
return;
}
if ("cgroupv1".equals(metrics.getProvider())) {
// cg v1 branch
} else {
// cg v2 branch
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1686587070
RFR: 8336881: [Linux] Support for hierarchical limits for Metrics
Please review this fix for cgroups-based metrics reporting in the `jdk.internal.platform` package. This fix is supposed to address wrong reporting of certain limits if the limits aren't set at the leaf nodes. For example, on cg v2, the memory limit interface file is `memory.max`. Consider a cgroup path of `/a/b/c/d`. The current code only reports the limits (via Metrics) correctly if it's set at `/a/b/c/d/memory.max`. However, some systems - like a systemd slice - sets those limits further up the hierarchy. For example at `/a/b/c/memory.max`. `/a/b/c/d/memory.max` might be set to the value `max` (for unlimited), yet `/a/b/c/memory.max` would report the actual limit value (e.g. `1048576000`). This patch addresses this issue by: 1. Refactoring the interface lookup code to relevant controllers for cpu/memory. The CgroupSubsystem classes then delegate to those for the lookup. This facilitates having an API for the lookup of an updated limit in step 2. 2. Walking the full hierarchy of the cgroup path (if any), looking for a lower limit than at the leaf. Note that it's not possible to raise the limit set at a path closer to the root via the interface file at a further-to-the-leaf-level. The odd case out seems to be `max` values on some systems (which seems to be the default value). As an optimization this hierarchy walk is skipped on containerized systems (like K8S), where the limits are set in interface files at the leaf nodes of the hierarchy. Therefore there should be no change on those systems. This patch depends on the Hotspot change implementing the same for the JVM so that `Metrics.isContainerized()` works correctly on affected systems where `-XshowSettings:system` currently reports `System not containerized` due to the missing JVM fix. A test framework for such hierarchical systems has been proposed in [JDK-8333446](https://bugs.openjdk.org/browse/JDK-8333446). This patch adds a test using that framework among some simpler unit tests. Thoughts? Testing: - [ ] GHA - [x] Container tests on Linux x86_64 on cg v1 and cg v2 systems - [x] Some manual testing using systemd slices - Depends on: https://git.openjdk.org/jdk/pull/17198 Commit messages: - 8336881: [Linux] Support for hierarchical limits for Metrics Changes: https://git.openjdk.org/jdk/pull/20280/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20280&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8336881 Stats: 1511 lines in 24 files changed: 1260 ins; 152 del; 99 mod Patch: https://git.openjdk.org/jdk/pull/20280.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20280/head:pull/20280 PR: https://git.openjdk.org/jdk/pull/20280
Re: RFR: 8336881: [Linux] Support for hierarchical limits for Metrics
On Mon, 22 Jul 2024 16:56:00 GMT, Severin Gehwolf wrote: > Please review this fix for cgroups-based metrics reporting in the > `jdk.internal.platform` package. This fix is supposed to address wrong > reporting of certain limits if the limits aren't set at the leaf nodes. > > For example, on cg v2, the memory limit interface file is `memory.max`. > Consider a cgroup path of `/a/b/c/d`. The current code only reports the > limits (via Metrics) correctly if it's set at `/a/b/c/d/memory.max`. However, > some systems - like a systemd slice - sets those limits further up the > hierarchy. For example at `/a/b/c/memory.max`. `/a/b/c/d/memory.max` might be > set to the value `max` (for unlimited), yet `/a/b/c/memory.max` would report > the actual limit value (e.g. `1048576000`). > > This patch addresses this issue by: > > 1. Refactoring the interface lookup code to relevant controllers for > cpu/memory. The CgroupSubsystem classes then delegate to those for the > lookup. This facilitates having an API for the lookup of an updated limit in > step 2. > 2. Walking the full hierarchy of the cgroup path (if any), looking for a > lower limit than at the leaf. Note that it's not possible to raise the limit > set at a path closer to the root via the interface file at a > further-to-the-leaf-level. The odd case out seems to be `max` values on some > systems (which seems to be the default value). > > As an optimization this hierarchy walk is skipped on containerized systems > (like K8S), where the limits are set in interface files at the leaf nodes of > the hierarchy. Therefore there should be no change on those systems. > > This patch depends on the Hotspot change implementing the same for the JVM so > that `Metrics.isContainerized()` works correctly on affected systems where > `-XshowSettings:system` currently reports `System not containerized` due to > the missing JVM fix. A test framework for such hierarchical systems has been > proposed in [JDK-8333446](https://bugs.openjdk.org/browse/JDK-8333446). This > patch adds a test using that framework among some simpler unit tests. > > Thoughts? > > Testing: > > - [ ] GHA > - [x] Container tests on Linux x86_64 on cg v1 and cg v2 systems > - [x] Some manual testing using systemd slices Note for reviewers. I'll be away until early August, so my responses might be delayed. - PR Comment: https://git.openjdk.org/jdk/pull/20280#issuecomment-2243438170
Re: RFR: 8336881: [Linux] Support for hierarchical limits for Metrics [v2]
> Please review this fix for cgroups-based metrics reporting in the > `jdk.internal.platform` package. This fix is supposed to address wrong > reporting of certain limits if the limits aren't set at the leaf nodes. > > For example, on cg v2, the memory limit interface file is `memory.max`. > Consider a cgroup path of `/a/b/c/d`. The current code only reports the > limits (via Metrics) correctly if it's set at `/a/b/c/d/memory.max`. However, > some systems - like a systemd slice - sets those limits further up the > hierarchy. For example at `/a/b/c/memory.max`. `/a/b/c/d/memory.max` might be > set to the value `max` (for unlimited), yet `/a/b/c/memory.max` would report > the actual limit value (e.g. `1048576000`). > > This patch addresses this issue by: > > 1. Refactoring the interface lookup code to relevant controllers for > cpu/memory. The CgroupSubsystem classes then delegate to those for the > lookup. This facilitates having an API for the lookup of an updated limit in > step 2. > 2. Walking the full hierarchy of the cgroup path (if any), looking for a > lower limit than at the leaf. Note that it's not possible to raise the limit > set at a path closer to the root via the interface file at a > further-to-the-leaf-level. The odd case out seems to be `max` values on some > systems (which seems to be the default value). > > As an optimization this hierarchy walk is skipped on containerized systems > (like K8S), where the limits are set in interface files at the leaf nodes of > the hierarchy. Therefore there should be no change on those systems. > > This patch depends on the Hotspot change implementing the same for the JVM so > that `Metrics.isContainerized()` works correctly on affected systems where > `-XshowSettings:system` currently reports `System not containerized` due to > the missing JVM fix. A test framework for such hierarchical systems has been > proposed in [JDK-8333446](https://bugs.openjdk.org/browse/JDK-8333446). This > patch adds a test using that framework among some simpler unit tests. > > Thoughts? > > Testing: > > - [x] GHA > - [x] Container tests on Linux x86_64 on cg v1 and cg v2 systems > - [x] Some manual testing using systemd slices Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. - Changes: - all: https://git.openjdk.org/jdk/pull/20280/files - new: https://git.openjdk.org/jdk/pull/20280/files/179791a1..179791a1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20280&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20280&range=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20280.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20280/head:pull/20280 PR: https://git.openjdk.org/jdk/pull/20280
Re: RFR: 8333446: Add tests for hierarchical container support [v4]
On Thu, 11 Jul 2024 16:46:13 GMT, Severin Gehwolf wrote: >> Please review this PR which adds test support for systemd slices so that >> bugs like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be >> verified. The added test, `SystemdMemoryAwarenessTest` currently passes on >> cgroups v1 and fails on cgroups v2 due to the way how >> [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented >> when JDK 13 was a thing. Therefore immediately problem-listed. It should get >> unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) >> merges. >> >> I'm adding those tests in order to not regress another time. >> >> Testing: >> - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. >> - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to >> JDK-8322420) >> - [x] GHA > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - Add Whitebox check for host cpu > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Fix comments > - 8333446: Add tests for hierarchical container support Please keep it open, bot. - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2284030728
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v33]
On Mon, 24 Jun 2024 14:33:51 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 137 commits: > > - JLinkDedupTestBatchSizeOne.java test fix > >Run only the packaged modules version if we have a linkable JDK runtime >with packaged modules available as well in the JDK install. > - Enable IncludeLocalesPluginTest > - Enable GenerateJLIClassesPluginTest.java test > - Enable JLinkReproducibleTest.java for linkable JDK images > - Remove restriction on directory based modules > >Enable the following tests: >- JLink100Modules.java >- JLinkDedupTestBatchSizeOne.java > - Comment fix in JRTArchive. Long line in JlinkTask > - Comment fixes in ImageFileCreator > - Comments and clean-up in JlinkTask > - Helper support for linkable JDK runtimes > - Test clean-up. class-file API module. > - ... and 127 more: https://git.openjdk.org/jdk/compare/5cad0b4d...04cd98f8 Please keep it open bot. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2284102882
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v19]
On Tue, 30 Jul 2024 07:47:55 GMT, Jan Kratochvil
wrote:
>> The testcase requires root permissions.
>>
>> Fix by Severin Gehwolf.
>> Testcase by Jan Kratochvil.
>
> Jan Kratochvil has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Inline adjust_controller() twice
> - Revert "Unify 4 copies of adjust_controller()"
>
>This reverts commit 77a81d07d74c8ae9bf34bfd8df9bcaca451ede9a.
Changes seem OK. The test needs some cleanup, though.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 103:
> 101: Asserts.assertEQ(0, exitValue, "Process returned unexpected
> exit code: " + exitValue);
> 102: return output;
> 103: }
This could be simplified to just `ProcessTools.executeProcess(new
ProcessBuilder(args));`
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 120:
> 118: args.add(jdkTool);
> 119: args.add("-cp");
> 120: args.add(System.getProperty("java.class.path"));
Should probably be `test.classes` instead of `java.class.path`.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 145:
> 143: }
> 144: throw e;
> 145: }
This should no longer be needed since we have the `@requires` line.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 193:
> 191: System.err.println(LINE_DELIM + " " + (isCgroup2 ? "cgroup2"
> : "cgroup1") + " mount point: " + sysFsCgroup);
> 192: memory_max_filename = isCgroup2 ? "memory.max" :
> "memory.limit_in_bytes";
> 193: Files.writeString(Path.of(sysFsCgroup + "/" + CGROUP_OUTER +
> "/" + memory_max_filename), "" + MEMORY_MAX_OUTER);
This still doesn't work on cgv1 (hybrid). The reg-ex pattern matching is wrong.
I'd suggest to simplify this by using `cgget` and `cgset` with forked
processes. Something like this (depending on the version use `memory.max` or
`memory.limit_in_bytes`):
$ cgcreate -g memory:/jdktest128331
$ cgcreate -g memory:/jdktest128331/inner
$ cgset -r memory.limit_in_bytes=512000 /jdktest128331/inner
$ cgget -n -v -r memory.limit_in_bytes /jdktest128331/inner
512000
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 210:
> 208: public Limits hook(List cgexec) throws IOException {
> 209: // CgroupV1Subsystem::read_memory_limit_in_bytes considered
> hierarchical_memory_limit only when inner memory.limit_in_bytes is unlimited.
> 210: Files.writeString(Path.of(sysFsCgroup + "/" + CGROUP_OUTER +
> "/" + CGROUP_INNER + "/" + memory_max_filename), "" + MEMORY_MAX_INNER);
This should be done using `cgset`.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 217:
> 215:
> 216: // KFAIL - verify the
> CgroupSubsystem::initialize_hierarchy() and
> jdk.internal.platform.CgroupSubsystem.initializeHierarchy() bug
> 217: // TestTwoLimits does not see the lower MEMORY_MAX_OUTER
> limit.
Remove this obsolete(?) comment.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 247:
> 245: self_verbose.add("-version");
> 246: pSystem(self_verbose);
> 247: }
Instead of disabling the memory controller that way, consider creating only the
cpu controller (using `cgcreate`) and test for memory limits?
test/jtreg-ext/requires/VMProps.java line 141:
> 139: map.put("vm.flagless", this::isFlagless);
> 140: map.put("jdk.foreign.linker", this::jdkForeignLinker);
> 141: map.put("vm.cgroup.tools", this::cgroupTools);
Why the `vm` prefix? Could be just `cgroup.tools` no? This isn't a JVM property.
-
PR Review: https://git.openjdk.org/jdk/pull/17198#pullrequestreview-2233709616
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714213497
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714218948
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714219641
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714181908
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714221143
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714221815
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714226775
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714228598
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v20]
On Wed, 14 Aug 2024 02:59:33 GMT, Jan Kratochvil
wrote:
>> The testcase requires root permissions.
>>
>> Fix by Severin Gehwolf.
>> Testcase by Jan Kratochvil.
>
> Jan Kratochvil has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Testcase update upon review by Severin Gehwolf
I've got my quibbles with the NestedCgroup test. In a nutshell, `TestTwoLimits`
and `TestNoController` are the same. Only for the latter the `cpu` controller
is enabled, we set the same `memory` limits for that tree and then assert the
same as for `TestTwoLimits`. I have to ask: why is `TestNoController` useful?
It might have been in some version of the patch, but not in its current form.
In OpenJDK we just assume that certain controllers (`cpu` and `memory` are
enabled). If they aren't then we return unlimited. We don't look at
`cgroup.subtree_control` files.
That leaves `TestTwoLimits` which essentially tests for a system that's badly
configured and we haven't seen in the wild. For what purpose? So that we encode
in a test what we know we don't yet support, because "you ain't gonna need it".
If you really insist to keep it, then please clean it up. I've spent way too
many cycles understanding it and would like to spare somebody else needing to
do the same.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 91:
> 89:
> 90: public Test(String controller_) throws Exception {
> 91: controller = controller_;
You are re-assigning a static class value's value in a constructor. This is
bound to break.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 109:
> 107:
> 108: // Alpine Linux 3.20.1 needs cgcreate1 otherwise:
> 109: // cgcreate: can't create cgroup [...]: No such file or
> directory
Suggestion:
// Create the parent cgroup hierarchy
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 118:
> 116: || output.contains("cgcreate: can't create cgroup " +
> CGROUP_OUTER + ": Cgroup, requested group parameter does not exist")) {
> 117: throw new SkippedException("Missing root permission: " +
> output.getStderr());
> 118: }
That we are the UID 0 ought to be checked in `NestedCgroup.main` before any
test is being run. Not here somewhere down the line. Use `Platform.isRoot()`.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 139:
> 137: } else {
> 138: throw new IllegalArgumentException();
> 139: }
This can be done once in `NestedCgroup.main` and then passed in on
instantiation of `Test*` classes.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 146:
> 144:
> 145: HookResult hookResult = hook();
> 146: // C++ CgroupController
Please remove this comment (or rephrase). It's not useful as it is.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 160:
> 158: HookResult hookResult = new HookResult();
> 159:
> 160: // CgroupV1Subsystem::read_memory_limit_in_bytes considered
> hierarchical_memory_limit only when inner memory.limit_in_bytes is unlimited.
There is no `hierarchical_memory_limit` any more. Please remove the comment. It
might get stale soon. If you really want a comment add a more high level one.
-
PR Review: https://git.openjdk.org/jdk/pull/17198#pullrequestreview-2237962572
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1717191803
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1717095471
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1716837525
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1717193195
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1717096997
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1716803597
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v19]
On Wed, 14 Aug 2024 03:26:02 GMT, Jan Kratochvil wrote: >> test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 217: >> >>> 215: >>> 216: // KFAIL - verify the >>> CgroupSubsystem::initialize_hierarchy() and >>> jdk.internal.platform.CgroupSubsystem.initializeHierarchy() bug >>> 217: // TestTwoLimits does not see the lower MEMORY_MAX_OUTER >>> limit. >> >> Remove this obsolete(?) comment. > > This comment is documenting what the `TestTwoLimits` testcase does. Which I > find useful. > It is KFAIL - Known Failure - the current OpenJDK code does not properly > simulate what Linux kernel does. If you do: > > cgset -r memory.max=$[1024*1024*1024] a/b > cgset -r memory.max=$[512*1024] a > cgexec -g memory:a/b java... > > Then OpenJDK thinks it is `1024*1024*1024 KB` but Linux kernel will limit > OpenJDK to `512*1024 KB`. So this problem is documented and tested. I see. Please update the comment. `KFAIL` isn't something I was familiar with as an acronym. Code/method references in tests don't seem appropriate since as soon as the code gets changed the comment is out of date/misleading. Refrain from having references to code. More general remarks: - No well-behaved system should set the limit at a deeper nesting level to anything non-`max`. That's why the code currently doesn't handle it. It's an optimization if you will and I don't see the benefit of walking the full hierarchy every time for such bad systems. - Please use a more high level comment for this test. Something like `"Since only non-well-behaved systems have a higher limit (other than unlimited) at a lower hierarchy level in the tree we stop iterating once we've encountered a limit that is not unlimited (-1). This test simulates this by asserting that the first non-unlimited value is detected by HotSpot, not the actual enforced limit of the kernel higher up the hierarchy."` - PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1716819006
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v21]
On Mon, 19 Aug 2024 08:12:25 GMT, Jan Kratochvil wrote: > As we cannot find an agreement even on the comment in the testcase and this > pull request will have soon an anniversary, proposing: > > * check-in the fix from a separate pull request as it is whole your fix > anyway > * I will just file this testcase into JBS, keep it downstream and close > this pull request > * or you can also check it in together with this testcase in any way, > shape or form as long as there isn't stated my name That sounds fair. Sorry about the trouble, but I believe the result will be better and more maintainable. Please log the test case as a separate JBS and pursue this separately (I won't use it). If you close this PR I'll create a new one and re-assign JDK-8322420 to me. Hopefully we can make some progress then. IMO, testing for the issue in form of https://bugs.openjdk.org/browse/JDK-8333446 should be sufficient. - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2296145898
Re: RFR: 8333446: Add tests for hierarchical container support [v5]
> Please review this PR which adds test support for systemd slices so that bugs > like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be > verified. The added test, `SystemdMemoryAwarenessTest` currently passes on > cgroups v1 and fails on cgroups v2 due to the way how > [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented > when JDK 13 was a thing. Therefore immediately problem-listed. It should get > unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) > merges. > > I'm adding those tests in order to not regress another time. > > Testing: > - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. > - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to JDK-8322420) > - [x] GHA Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Add Whitebox check for host cpu - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Fix comments - 8333446: Add tests for hierarchical container support - Changes: - all: https://git.openjdk.org/jdk/pull/19530/files - new: https://git.openjdk.org/jdk/pull/19530/files/139a9069..eda249b4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=03-04 Stats: 54052 lines in 1621 files changed: 30101 ins; 16055 del; 7896 mod Patch: https://git.openjdk.org/jdk/pull/19530.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19530/head:pull/19530 PR: https://git.openjdk.org/jdk/pull/19530
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v34]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 139 commits: - Merge branch 'master' into jdk-8311302-jmodless-link - Merge branch 'master' into jdk-8311302-jmodless-link - JLinkDedupTestBatchSizeOne.java test fix Run only the packaged modules version if we have a linkable JDK runtime with packaged modules available as well in the JDK install. - Enable IncludeLocalesPluginTest - Enable GenerateJLIClassesPluginTest.java test - Enable JLinkReproducibleTest.java for linkable JDK images - Remove restriction on directory based modules Enable the following tests: - JLink100Modules.java - JLinkDedupTestBatchSizeOne.java - Comment fix in JRTArchive. Long line in JlinkTask - Comment fixes in ImageFileCreator - Comments and clean-up in JlinkTask - ... and 129 more: https://git.openjdk.org/jdk/compare/686eb233...7e2bc4e1 - Changes: https://git.openjdk.org/jdk/pull/14787/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=33 Stats: 3959 lines in 42 files changed: 3762 ins; 117 del; 80 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8333446: Add tests for hierarchical container support [v5]
On Tue, 20 Aug 2024 17:34:46 GMT, Severin Gehwolf wrote: >> Please review this PR which adds test support for systemd slices so that >> bugs like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be >> verified. The added test, `SystemdMemoryAwarenessTest` currently passes on >> cgroups v1 and fails on cgroups v2 due to the way how >> [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented >> when JDK 13 was a thing. Therefore immediately problem-listed. It should get >> unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) >> merges. >> >> I'm adding those tests in order to not regress another time. >> >> Testing: >> - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. >> - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to >> JDK-8322420) >> - [x] GHA > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Add Whitebox check for host cpu > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Fix comments > - 8333446: Add tests for hierarchical container support GHA failure on maxos-aarch64 is unrelated: [runtime/cds/DeterministicDump](https://github.com/jerboaa/jdk/actions/runs/10476366525#user-content-runtime_cds_deterministicdump) - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2301519742
Re: RFR: 8336881: [Linux] Support for hierarchical limits for Metrics [v3]
> Please review this fix for cgroups-based metrics reporting in the > `jdk.internal.platform` package. This fix is supposed to address wrong > reporting of certain limits if the limits aren't set at the leaf nodes. > > For example, on cg v2, the memory limit interface file is `memory.max`. > Consider a cgroup path of `/a/b/c/d`. The current code only reports the > limits (via Metrics) correctly if it's set at `/a/b/c/d/memory.max`. However, > some systems - like a systemd slice - sets those limits further up the > hierarchy. For example at `/a/b/c/memory.max`. `/a/b/c/d/memory.max` might be > set to the value `max` (for unlimited), yet `/a/b/c/memory.max` would report > the actual limit value (e.g. `1048576000`). > > This patch addresses this issue by: > > 1. Refactoring the interface lookup code to relevant controllers for > cpu/memory. The CgroupSubsystem classes then delegate to those for the > lookup. This facilitates having an API for the lookup of an updated limit in > step 2. > 2. Walking the full hierarchy of the cgroup path (if any), looking for a > lower limit than at the leaf. Note that it's not possible to raise the limit > set at a path closer to the root via the interface file at a > further-to-the-leaf-level. The odd case out seems to be `max` values on some > systems (which seems to be the default value). > > As an optimization this hierarchy walk is skipped on containerized systems > (like K8S), where the limits are set in interface files at the leaf nodes of > the hierarchy. Therefore there should be no change on those systems. > > This patch depends on the Hotspot change implementing the same for the JVM so > that `Metrics.isContainerized()` works correctly on affected systems where > `-XshowSettings:system` currently reports `System not containerized` due to > the missing JVM fix. A test framework for such hierarchical systems has been > proposed in [JDK-8333446](https://bugs.openjdk.org/browse/JDK-8333446). This > patch adds a test using that framework among some simpler unit tests. > > Thoughts? > > Testing: > > - [x] GHA > - [x] Container tests on Linux x86_64 on cg v1 and cg v2 systems > - [x] Some manual testing using systemd slices Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit: 8336881: [Linux] Support for hierarchical limits for Metrics - Changes: https://git.openjdk.org/jdk/pull/20280/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20280&range=02 Stats: 1511 lines in 24 files changed: 1260 ins; 152 del; 99 mod Patch: https://git.openjdk.org/jdk/pull/20280.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20280/head:pull/20280 PR: https://git.openjdk.org/jdk/pull/20280
Re: RFR: 8336881: [Linux] Support for hierarchical limits for Metrics [v2]
On Mon, 29 Jul 2024 14:18:24 GMT, Severin Gehwolf wrote: >> Please review this fix for cgroups-based metrics reporting in the >> `jdk.internal.platform` package. This fix is supposed to address wrong >> reporting of certain limits if the limits aren't set at the leaf nodes. >> >> For example, on cg v2, the memory limit interface file is `memory.max`. >> Consider a cgroup path of `/a/b/c/d`. The current code only reports the >> limits (via Metrics) correctly if it's set at `/a/b/c/d/memory.max`. >> However, some systems - like a systemd slice - sets those limits further up >> the hierarchy. For example at `/a/b/c/memory.max`. `/a/b/c/d/memory.max` >> might be set to the value `max` (for unlimited), yet `/a/b/c/memory.max` >> would report the actual limit value (e.g. `1048576000`). >> >> This patch addresses this issue by: >> >> 1. Refactoring the interface lookup code to relevant controllers for >> cpu/memory. The CgroupSubsystem classes then delegate to those for the >> lookup. This facilitates having an API for the lookup of an updated limit in >> step 2. >> 2. Walking the full hierarchy of the cgroup path (if any), looking for a >> lower limit than at the leaf. Note that it's not possible to raise the limit >> set at a path closer to the root via the interface file at a >> further-to-the-leaf-level. The odd case out seems to be `max` values on some >> systems (which seems to be the default value). >> >> As an optimization this hierarchy walk is skipped on containerized systems >> (like K8S), where the limits are set in interface files at the leaf nodes of >> the hierarchy. Therefore there should be no change on those systems. >> >> This patch depends on the Hotspot change implementing the same for the JVM >> so that `Metrics.isContainerized()` works correctly on affected systems >> where `-XshowSettings:system` currently reports `System not containerized` >> due to the missing JVM fix. A test framework for such hierarchical systems >> has been proposed in >> [JDK-8333446](https://bugs.openjdk.org/browse/JDK-8333446). This patch adds >> a test using that framework among some simpler unit tests. >> >> Thoughts? >> >> Testing: >> >> - [x] GHA >> - [x] Container tests on Linux x86_64 on cg v1 and cg v2 systems >> - [x] Some manual testing using systemd slices > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains three commits: > > - 8336881: [Linux] Support for hierarchical limits for Metrics > - Unify 4 copies of adjust_controller() > - Fix a needless whitespace change Moving to draft. Needs the hotspot fix which gets a reboot. - PR Comment: https://git.openjdk.org/jdk/pull/20280#issuecomment-2296180545
Re: RFR: 8333446: Add tests for hierarchical container support [v5]
On Tue, 20 Aug 2024 17:34:46 GMT, Severin Gehwolf wrote: >> Please review this PR which adds test support for systemd slices so that >> bugs like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be >> verified. The added test, `SystemdMemoryAwarenessTest` currently passes on >> cgroups v1 and fails on cgroups v2 due to the way how >> [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented >> when JDK 13 was a thing. Therefore immediately problem-listed. It should get >> unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) >> merges. >> >> I'm adding those tests in order to not regress another time. >> >> Testing: >> - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. >> - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to >> JDK-8322420) >> - [x] GHA > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Add Whitebox check for host cpu > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Fix comments > - 8333446: Add tests for hierarchical container support @dholmes-ora @iklam @MBaesken Could somebody of you help review this, please? Increases test coverage in the container detection area. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2302519288
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v35]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 140 commits: - Merge branch 'master' into jdk-8311302-jmodless-link - Merge branch 'master' into jdk-8311302-jmodless-link - Merge branch 'master' into jdk-8311302-jmodless-link - JLinkDedupTestBatchSizeOne.java test fix Run only the packaged modules version if we have a linkable JDK runtime with packaged modules available as well in the JDK install. - Enable IncludeLocalesPluginTest - Enable GenerateJLIClassesPluginTest.java test - Enable JLinkReproducibleTest.java for linkable JDK images - Remove restriction on directory based modules Enable the following tests: - JLink100Modules.java - JLinkDedupTestBatchSizeOne.java - Comment fix in JRTArchive. Long line in JlinkTask - Comment fixes in ImageFileCreator - ... and 130 more: https://git.openjdk.org/jdk/compare/aefdbdc7...59997873 - Changes: https://git.openjdk.org/jdk/pull/14787/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=34 Stats: 3959 lines in 42 files changed: 3762 ins; 117 del; 80 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8333446: Add tests for hierarchical container support [v5]
On Tue, 27 Aug 2024 14:12:40 GMT, Zdenek Zambersky wrote: > If I am not mistaken, new test requires, that testsuite is ran as superuser > (root). (Because it writes `/etc/systemd/system`, runs certain systemd > commands). Should test be skipped for non-root? Thanks! I can add that. FWIW, container tests are in a similar situation (applying cpu/memory limits is not allowed in rootless on cg v1) and they don't check for it. - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2312812395
Re: RFR: 8333446: Add tests for hierarchical container support [v5]
On Tue, 27 Aug 2024 14:24:18 GMT, Matthias Baesken wrote: > I added the PR to our internal build/test queue . Thanks, Matthias! - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2312813813
Re: RFR: 8336881: [Linux] Support for hierarchical limits for Metrics [v4]
> Please review this fix for cgroups-based metrics reporting in the > `jdk.internal.platform` package. This fix is supposed to address wrong > reporting of certain limits if the limits aren't set at the leaf nodes. > > For example, on cg v2, the memory limit interface file is `memory.max`. > Consider a cgroup path of `/a/b/c/d`. The current code only reports the > limits (via Metrics) correctly if it's set at `/a/b/c/d/memory.max`. However, > some systems - like a systemd slice - sets those limits further up the > hierarchy. For example at `/a/b/c/memory.max`. `/a/b/c/d/memory.max` might be > set to the value `max` (for unlimited), yet `/a/b/c/memory.max` would report > the actual limit value (e.g. `1048576000`). > > This patch addresses this issue by: > > 1. Refactoring the interface lookup code to relevant controllers for > cpu/memory. The CgroupSubsystem classes then delegate to those for the > lookup. This facilitates having an API for the lookup of an updated limit in > step 2. > 2. Walking the full hierarchy of the cgroup path (if any), looking for a > lower limit than at the leaf. Note that it's not possible to raise the limit > set at a path closer to the root via the interface file at a > further-to-the-leaf-level. The odd case out seems to be `max` values on some > systems (which seems to be the default value). > > As an optimization this hierarchy walk is skipped on containerized systems > (like K8S), where the limits are set in interface files at the leaf nodes of > the hierarchy. Therefore there should be no change on those systems. > > This patch depends on the Hotspot change implementing the same for the JVM so > that `Metrics.isContainerized()` works correctly on affected systems where > `-XshowSettings:system` currently reports `System not containerized` due to > the missing JVM fix. A test framework for such hierarchical systems has been > proposed in [JDK-8333446](https://bugs.openjdk.org/browse/JDK-8333446). This > patch adds a test using that framework among some simpler unit tests. > > Thoughts? > > Testing: > > - [x] GHA > - [x] Container tests on Linux x86_64 on cg v1 and cg v2 systems > - [x] Some manual testing using systemd slices Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'jdk-8322420_cgroup_hierarchy_walk_init' into jdk-8336881-metrics-systemd-slice - 8336881: [Linux] Support for hierarchical limits for Metrics - Changes: - all: https://git.openjdk.org/jdk/pull/20280/files - new: https://git.openjdk.org/jdk/pull/20280/files/f0c775b2..2d918ca4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20280&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20280&range=02-03 Stats: 6199 lines in 321 files changed: 4177 ins; 896 del; 1126 mod Patch: https://git.openjdk.org/jdk/pull/20280.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20280/head:pull/20280 PR: https://git.openjdk.org/jdk/pull/20280
Re: RFR: 8333446: Add tests for hierarchical container support [v6]
> Please review this PR which adds test support for systemd slices so that bugs > like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be > verified. The added test, `SystemdMemoryAwarenessTest` currently passes on > cgroups v1 and fails on cgroups v2 due to the way how > [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented > when JDK 13 was a thing. Therefore immediately problem-listed. It should get > unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) > merges. > > I'm adding those tests in order to not regress another time. > > Testing: > - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. > - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to JDK-8322420) > - [x] GHA Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision: - Add root check for SystemdMemoryAwarenessTest.java - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Add Whitebox check for host cpu - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Fix comments - 8333446: Add tests for hierarchical container support - Changes: - all: https://git.openjdk.org/jdk/pull/19530/files - new: https://git.openjdk.org/jdk/pull/19530/files/eda249b4..7e8d9ed4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=04-05 Stats: 6081 lines in 315 files changed: 4125 ins; 864 del; 1092 mod Patch: https://git.openjdk.org/jdk/pull/19530.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19530/head:pull/19530 PR: https://git.openjdk.org/jdk/pull/19530
Re: RFR: 8333446: Add tests for hierarchical container support [v5]
On Wed, 28 Aug 2024 14:21:09 GMT, Zdenek Zambersky wrote: > > > If I am not mistaken, new test requires, that testsuite is ran as > > > superuser (root). (Because it writes `/etc/systemd/system`, runs certain > > > systemd commands). Should test be skipped for non-root? > > > > > > Thanks! I can add that. FWIW, container tests are in a similar situation > > (applying cpu/memory limits is not allowed in rootless on cg v1) and they > > don't check for it. > > I think, it would be nice to skip on non-root, as tests are often ran as > non-root. Thanks. Done in https://github.com/openjdk/jdk/pull/19530/commits/7e8d9ed46815096ae8c4502f3320ebf5208438d5 - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2315757991
Re: RFR: 8333446: Add tests for hierarchical container support [v6]
On Thu, 29 Aug 2024 15:22:02 GMT, Matthias Baesken wrote:
>> Severin Gehwolf has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 10 additional
>> commits since the last revision:
>>
>> - Add root check for SystemdMemoryAwarenessTest.java
>> - Merge branch 'master' into jdk-8333446-systemd-slice-tests
>> - Merge branch 'master' into jdk-8333446-systemd-slice-tests
>> - Merge branch 'master' into jdk-8333446-systemd-slice-tests
>> - Add Whitebox check for host cpu
>> - Merge branch 'master' into jdk-8333446-systemd-slice-tests
>> - Merge branch 'master' into jdk-8333446-systemd-slice-tests
>> - Merge branch 'master' into jdk-8333446-systemd-slice-tests
>> - Fix comments
>> - 8333446: Add tests for hierarchical container support
>
> test/hotspot/jtreg/containers/systemd/SystemdMemoryAwarenessTest.java line 58:
>
>> 56: SystemdRunOptions opts =
>> SystemdTestUtils.newOpts("HelloSystemd");
>> 57: // 1 GB memory
>> 58: opts.memoryLimit("1000M");
>
> Just wondering - is 1G here possible (the comment states 1 GB / 1024M) ?
I probably shall fix the comment or change it to `1024M`. Either way it has to
match the assertion where we look for `1048576000` bytes in the output.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19530#discussion_r1736549823
Re: RFR: 8333446: Add tests for hierarchical container support [v6]
On Wed, 28 Aug 2024 16:13:07 GMT, Severin Gehwolf wrote: >> Please review this PR which adds test support for systemd slices so that >> bugs like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be >> verified. The added test, `SystemdMemoryAwarenessTest` currently passes on >> cgroups v1 and fails on cgroups v2 due to the way how >> [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented >> when JDK 13 was a thing. Therefore immediately problem-listed. It should get >> unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) >> merges. >> >> I'm adding those tests in order to not regress another time. >> >> Testing: >> - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. >> - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to >> JDK-8322420) >> - [x] GHA > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 10 additional > commits since the last revision: > > - Add root check for SystemdMemoryAwarenessTest.java > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Add Whitebox check for host cpu > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Merge branch 'master' into jdk-8333446-systemd-slice-tests > - Fix comments > - 8333446: Add tests for hierarchical container support Noting here that I'll amend those tests to also cover nested hierarchical limits where the lower limit is higher up the hierarchy. - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2318291396
Re: RFR: 8333446: Add tests for hierarchical container support [v6]
On Fri, 30 Aug 2024 11:02:52 GMT, Matthias Baesken wrote: >> Severin Gehwolf has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 10 additional >> commits since the last revision: >> >> - Add root check for SystemdMemoryAwarenessTest.java >> - Merge branch 'master' into jdk-8333446-systemd-slice-tests >> - Merge branch 'master' into jdk-8333446-systemd-slice-tests >> - Merge branch 'master' into jdk-8333446-systemd-slice-tests >> - Add Whitebox check for host cpu >> - Merge branch 'master' into jdk-8333446-systemd-slice-tests >> - Merge branch 'master' into jdk-8333446-systemd-slice-tests >> - Merge branch 'master' into jdk-8333446-systemd-slice-tests >> - Fix comments >> - 8333446: Add tests for hierarchical container support > > src/hotspot/share/prims/whitebox.cpp line 2507: > >> 2505: WB_END >> 2506: >> 2507: // Physical cpus of the host machine (including containers), Linux >> only. > > Isn't the comment a bit misleading ? From what I see , ` > os::Linux::active_processor_count()` can use various mechanisms to get number > of processor info, if it uses https://linux.die.net/man/2/sched_getaffinity > it gives the 'set of CPUs on which it is eligible to run.' That might be > different from what the host has. Yes. See #20768 for an attempt to unify it. I'll change the comment with the update that I have for nested hierarchies. Thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/19530#discussion_r1738475745
Re: RFR: 8333446: Add tests for hierarchical container support [v6]
On Fri, 30 Aug 2024 11:05:24 GMT, Matthias Baesken wrote: > Not saying that this is a very bad thing, maybe it is just the way it is, > that 'root' is needed ? I'll do some more research whether or not that is a hard requirement. Thanks for the comments so far. - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2320970507
Re: RFR: 8333446: Add tests for hierarchical container support [v6]
On Fri, 30 Aug 2024 11:46:51 GMT, Severin Gehwolf wrote: > > Not saying that this is a very bad thing, maybe it is just the way it is, > > that 'root' is needed ? > > I'll do some more research whether or not that is a hard requirement. Thanks > for the comments so far. It turns out it works on cgroups v2 as non-root. I shall amend the test so that it at least runs OK on non-root and cgroups v2. - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2321150231
Re: RFR: 8333446: Add tests for hierarchical container support [v7]
> Please review this PR which adds test support for systemd slices so that bugs > like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be > verified. The added test, `SystemdMemoryAwarenessTest` currently passes on > cgroups v1 and fails on cgroups v2 due to the way how > [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented > when JDK 13 was a thing. Therefore immediately problem-listed. It should get > unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) > merges. > > I'm adding those tests in order to not regress another time. > > Testing: > - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. > - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to JDK-8322420) > - [x] GHA Severin Gehwolf has updated the pull request incrementally with four additional commits since the last revision: - Fix comment of WB::host_cpus() - Handle non-root + CGv2 - Add nested hierarchy to test framework - Revert "Add root check for SystemdMemoryAwarenessTest.java" This reverts commit 7e8d9ed46815096ae8c4502f3320ebf5208438d5. - Changes: - all: https://git.openjdk.org/jdk/pull/19530/files - new: https://git.openjdk.org/jdk/pull/19530/files/7e8d9ed4..a98fd7d6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=05-06 Stats: 232 lines in 4 files changed: 167 ins; 35 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/19530.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19530/head:pull/19530 PR: https://git.openjdk.org/jdk/pull/19530
Re: RFR: 8333446: Add tests for hierarchical container support [v7]
On Fri, 30 Aug 2024 14:14:05 GMT, Severin Gehwolf wrote: >> Please review this PR which adds test support for systemd slices so that >> bugs like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be >> verified. The added test, `SystemdMemoryAwarenessTest` currently passes on >> cgroups v1 and fails on cgroups v2 due to the way how >> [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented >> when JDK 13 was a thing. Therefore immediately problem-listed. It should get >> unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) >> merges. >> >> I'm adding those tests in order to not regress another time. >> >> Testing: >> - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. >> - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to >> JDK-8322420) >> - [x] GHA > > Severin Gehwolf has updated the pull request incrementally with four > additional commits since the last revision: > > - Fix comment of WB::host_cpus() > - Handle non-root + CGv2 > - Add nested hierarchy to test framework > - Revert "Add root check for SystemdMemoryAwarenessTest.java" > >This reverts commit 7e8d9ed46815096ae8c4502f3320ebf5208438d5. I've updated the patch to: 1. Run tests on cg v2 when non-root (uses `~/.config/systemd/user`) directory and `systemctl --user` as well as `systemd-run --user` in that case. 2. The framework now also sets it up so that there is a lower limit further down the hierarchy: `jdk_internal.slice.d` directory has the lowest limit. The slice files itself have a higher one. The test asserts that the lower limit is being detected correctly. 3. The framework now skips the test in the main entry point (i.e. on cg v1 and non-root). 4. Addressed review comments so far. Tested on cg v1 and cg v2 as root and non-root each (with the patch of #20646 applied as well). Please let me know what you think. - PR Comment: https://git.openjdk.org/jdk/pull/19530#issuecomment-2321413352
Re: RFR: 8333446: Add tests for hierarchical container support [v6]
On Fri, 30 Aug 2024 11:40:45 GMT, Severin Gehwolf wrote:
>> src/hotspot/share/prims/whitebox.cpp line 2507:
>>
>>> 2505: WB_END
>>> 2506:
>>> 2507: // Physical cpus of the host machine (including containers), Linux
>>> only.
>>
>> Isn't the comment a bit misleading ? From what I see , `
>> os::Linux::active_processor_count()` can use various mechanisms to get
>> number of processor info, if it uses
>> https://linux.die.net/man/2/sched_getaffinity it gives the 'set of CPUs on
>> which it is eligible to run.' That might be different from what the host
>> has.
>
> Yes. See #20768 for an attempt to unify it. I'll change the comment with the
> update that I have for nested hierarchies. Thanks!
I've changed the comment.
>> test/hotspot/jtreg/containers/systemd/SystemdMemoryAwarenessTest.java line
>> 58:
>>
>>> 56: SystemdRunOptions opts =
>>> SystemdTestUtils.newOpts("HelloSystemd");
>>> 57: // 1 GB memory
>>> 58: opts.memoryLimit("1000M");
>>
>> Just wondering - is 1G here possible (the comment states 1 GB / 1024M) ?
>
> I probably shall fix the comment or change it to `1024M`. Either way it has
> to match the assertion where we look for `1048576000` bytes in the output.
This should be fixed now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19530#discussion_r1738770335
PR Review Comment: https://git.openjdk.org/jdk/pull/19530#discussion_r1738770983
Re: RFR: 8336881: [Linux] Support for hierarchical limits for Metrics [v5]
> Please review this fix for cgroups-based metrics reporting in the > `jdk.internal.platform` package. This fix is supposed to address wrong > reporting of certain limits if the limits aren't set at the leaf nodes. > > For example, on cg v2, the memory limit interface file is `memory.max`. > Consider a cgroup path of `/a/b/c/d`. The current code only reports the > limits (via Metrics) correctly if it's set at `/a/b/c/d/memory.max`. However, > some systems - like a systemd slice - sets those limits further up the > hierarchy. For example at `/a/b/c/memory.max`. `/a/b/c/d/memory.max` might be > set to the value `max` (for unlimited), yet `/a/b/c/memory.max` would report > the actual limit value (e.g. `1048576000`). > > This patch addresses this issue by: > > 1. Refactoring the interface lookup code to relevant controllers for > cpu/memory. The CgroupSubsystem classes then delegate to those for the > lookup. This facilitates having an API for the lookup of an updated limit in > step 2. > 2. Walking the full hierarchy of the cgroup path (if any), looking for a > lower limit than at the leaf. Note that it's not possible to raise the limit > set at a path closer to the root via the interface file at a > further-to-the-leaf-level. The odd case out seems to be `max` values on some > systems (which seems to be the default value). > > As an optimization this hierarchy walk is skipped on containerized systems > (like K8S), where the limits are set in interface files at the leaf nodes of > the hierarchy. Therefore there should be no change on those systems. > > This patch depends on the Hotspot change implementing the same for the JVM so > that `Metrics.isContainerized()` works correctly on affected systems where > `-XshowSettings:system` currently reports `System not containerized` due to > the missing JVM fix. A test framework for such hierarchical systems has been > proposed in [JDK-8333446](https://bugs.openjdk.org/browse/JDK-8333446). This > patch adds a test using that framework among some simpler unit tests. > > Thoughts? > > Testing: > > - [x] GHA > - [x] Container tests on Linux x86_64 on cg v1 and cg v2 systems > - [x] Some manual testing using systemd slices Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 89 additional commits since the last revision: - Merge branch 'jdk-8322420_cgroup_hierarchy_walk_init' into jdk-8336881-metrics-systemd-slice - Merge branch 'master' into jdk-8322420_cgroup_hierarchy_walk_init - 8339419: [s390x] Problemlist compiler/c2/irTests/TestIfMinMax.java Reviewed-by: thartmann - 8338916: Build warnings about overriding recipe for jvm-ldflags.txt Reviewed-by: jwaters, erikj - 8339336: Fix build system whitespace to adhere to coding conventions Reviewed-by: erikj - 8339214: Remove misleading CodeBuilder.loadConstant(Opcode, ConstantDesc) Reviewed-by: asotona - 8338740: java/net/httpclient/HttpsTunnelAuthTest.java fails with java.io.IOException: HTTP/1.1 header parser received no bytes Reviewed-by: djelinski, jpai - 8325397: sun/java2d/Disposer/TestDisposerRace.java fails in linux-aarch64 Reviewed-by: alanb - 8339401: Optimize ClassFile load and store instructions Reviewed-by: liach, redestad - 8339364: AIX build fails: various unused variable and function warnings Reviewed-by: mdoerr, clanger, jwaters - ... and 79 more: https://git.openjdk.org/jdk/compare/e4f2f439...7beccf23 - Changes: - all: https://git.openjdk.org/jdk/pull/20280/files - new: https://git.openjdk.org/jdk/pull/20280/files/2d918ca4..7beccf23 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20280&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20280&range=03-04 Stats: 10214 lines in 362 files changed: 6051 ins; 1713 del; 2450 mod Patch: https://git.openjdk.org/jdk/pull/20280.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20280/head:pull/20280 PR: https://git.openjdk.org/jdk/pull/20280
Re: RFR: 8333446: Add tests for hierarchical container support [v8]
> Please review this PR which adds test support for systemd slices so that bugs > like [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) can be > verified. The added test, `SystemdMemoryAwarenessTest` currently passes on > cgroups v1 and fails on cgroups v2 due to the way how > [JDK-8217338](https://bugs.openjdk.org/browse/JDK-8217338) was implemented > when JDK 13 was a thing. Therefore immediately problem-listed. It should get > unlisted once [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420) > merges. > > I'm adding those tests in order to not regress another time. > > Testing: > - [x] Container tests on Linux x86_64 cgroups v2 and Linux x86_64 cgroups v1. > - [x] New systemd test on cg v1 (passes). Fails on cg v2 (due to JDK-8322420) > - [x] GHA Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 15 additional commits since the last revision: - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Fix comment of WB::host_cpus() - Handle non-root + CGv2 - Add nested hierarchy to test framework - Revert "Add root check for SystemdMemoryAwarenessTest.java" This reverts commit 7e8d9ed46815096ae8c4502f3320ebf5208438d5. - Add root check for SystemdMemoryAwarenessTest.java - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Merge branch 'master' into jdk-8333446-systemd-slice-tests - Add Whitebox check for host cpu - ... and 5 more: https://git.openjdk.org/jdk/compare/fc4604c0...cf49a96e - Changes: - all: https://git.openjdk.org/jdk/pull/19530/files - new: https://git.openjdk.org/jdk/pull/19530/files/a98fd7d6..cf49a96e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19530&range=06-07 Stats: 10055 lines in 354 files changed: 6004 ins; 1681 del; 2370 mod Patch: https://git.openjdk.org/jdk/pull/19530.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19530/head:pull/19530 PR: https://git.openjdk.org/jdk/pull/19530
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v36]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 141 commits: - Merge branch 'master' into jdk-8311302-jmodless-link - Merge branch 'master' into jdk-8311302-jmodless-link - Merge branch 'master' into jdk-8311302-jmodless-link - Merge branch 'master' into jdk-8311302-jmodless-link - JLinkDedupTestBatchSizeOne.java test fix Run only the packaged modules version if we have a linkable JDK runtime with packaged modules available as well in the JDK install. - Enable IncludeLocalesPluginTest - Enable GenerateJLIClassesPluginTest.java test - Enable JLinkReproducibleTest.java for linkable JDK images - Remove restriction on directory based modules Enable the following tests: - JLink100Modules.java - JLinkDedupTestBatchSizeOne.java - Comment fix in JRTArchive. Long line in JlinkTask - ... and 131 more: https://git.openjdk.org/jdk/compare/0d593cd1...ec68b0a8 - Changes: https://git.openjdk.org/jdk/pull/14787/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=35 Stats: 3959 lines in 42 files changed: 3762 ins; 117 del; 80 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8293540: [Metrics] Incorrectly detected resource limits with additional cgroup fs mounts [v3]
On Wed, 28 Sep 2022 05:45:14 GMT, Ioi Lam wrote:
>> Severin Gehwolf has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains one commit:
>>
>> 8293540: [Metrics] Potentially incorrectly detected resource limits with
>> additional cgroup fs mounts
>
> The JDK change looks good to me. Some nits for the test cases.
Thanks for the review @iklam!
> test/jdk/jdk/internal/platform/docker/TestDockerBasic.java line 26:
>
>> 24: /*
>> 25: * @test
>> 26: * @summary Verify that -XshowSettings:system works
>
> Add `@bug 8293540`
Thanks, added.
> test/jdk/jdk/internal/platform/docker/TestDockerBasic.java line 64:
>
>> 62: opts.addDockerOpts("--memory", "300m");
>> 63: if (addCgroupMounts) {
>> 64: opts.addDockerOpts("--volume",
>> "/sys/fs/cgroup:/cgroup-in:ro");
>
> Add comments that the extra mount should be ignored by the cgroup set-up
> code. (Same for other test cases).
OK. Added in the updated version.
-
PR: https://git.openjdk.org/jdk/pull/10248
