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=20280=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: 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
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 [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 [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: 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: 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]
> 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=19530=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19530=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 [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: 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 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: 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
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: 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
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. @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
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=20076=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: 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
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]
> 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=19530=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19530=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: 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
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 @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
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=18201=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 [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 [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 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: 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 [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: 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=18201=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 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: 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=14787=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 [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: 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=18201=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18201=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 [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=18201=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: 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: 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=19530=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19530=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: 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: 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: 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: 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: 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=18201=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: 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: 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 [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]
> 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=14787=31 - incr: https://webrevs.openjdk.org/?repo=jdk=14787=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 [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 [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=14787=30 - incr: https://webrevs.openjdk.org/?repo=jdk=14787=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 [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 [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: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 [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 [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=14787=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 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: 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
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=19530=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: 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
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 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: 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: 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: 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: 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 [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 [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=14787=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 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 [v28]
> 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 108 commits: - 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 - Fix coment - Fix comment - ... and 98 more: https://git.openjdk.org/jdk/compare/957eb611...8cca277e - Changes: https://git.openjdk.org/jdk/pull/14787/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14787=27 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 [v27]
> 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 two additional commits since the last revision: - Simplify JLINK_JDK_EXTRA_OPTS var handling - Only add runtime track files for linkable runtimes - Changes: - all: https://git.openjdk.org/jdk/pull/14787/files - new: https://git.openjdk.org/jdk/pull/14787/files/67aea4ca..be30a182 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14787=26 - incr: https://webrevs.openjdk.org/?repo=jdk=14787=25-26 Stats: 11 lines in 2 files changed: 5 ins; 5 del; 1 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 [v26]
On Wed, 8 May 2024 22:36:51 GMT, Magnus Ihse Bursie 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 105 commits: >> >> - 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 >> - Fix coment >> - Fix comment >> - Fix typo >> - Revert some now unneded build changes >> - Follow build tools naming convention >> - ... and 95 more: https://git.openjdk.org/jdk/compare/23a72a1f...67aea4ca > > make/Images.gmk line 100: > >> 98: >> 99: ifeq ($(JLINK_PRODUCE_RUNTIME_LINK_JDK), true) >> 100: JLINK_JDK_EXTRA_OPTS := $(JLINK_JDK_EXTRA_OPTS) >> --generate-linkable-runtime > > I just noticed this slight improvement: > > Suggestion: > > JLINK_JDK_EXTRA_OPTS += --generate-linkable-runtime Thanks, fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1601322116
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]
On Thu, 4 Apr 2024 20:56:55 GMT, Magnus Ihse Bursie wrote: >> Severin Gehwolf has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Use shorter name for the build tool >> - Review feedback from Erik J. >> - Remove dependency on CDS which isn't needed anymore > > Overall, I think the build system changes in the current revision of the > patch look good, but please let me know for a deeper review when things have > stabilized. Thanks for the review, @magicus! - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2101006001
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v26]
On Tue, 7 May 2024 16:52:12 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 105 commits: > > - 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 > - Fix coment > - Fix comment > - Fix typo > - Revert some now unneded build changes > - Follow build tools naming convention > - ... and 95 more: https://git.openjdk.org/jdk/compare/23a72a1f...67aea4ca GHA test failures [gc/shenandoah/oom/TestThreadFailure](https://github.com/jerboaa/jdk/actions/runs/8989206765#user-content-gc_shenandoah_oom_testthreadfailure) on Mac x86_64 and [java/util/zip/ZipFile/ZipSourceCache](https://github.com/jerboaa/jdk/actions/runs/8989206765#user-content-java_util_zip_zipfile_zipsourcecache) on Windows x86_64 seem unrelated to this patch. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2100159861
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Tue, 16 Apr 2024 13:54:53 GMT, Alan Bateman wrote: >>> > @mlchung @AlanBateman Any thoughts on this latest version? Is this going >>> > into the direction you had envisioned? Any more blockers? The CSR should >>> > be up-to-date and is open for review as well. If no more blockers I'll go >>> > over this patch once more and clean it up to prepare for integration. >>> > Thanks! >>> >>> Thanks for all the efforts on this. >> >> Thanks for looking at this, Alan! >> >>> I've looked through the latest version. I'm a little bit comfortable with >>> LinkDeltaProducer. One reason it's build tool that is tied tied to code in >>> the jdk.jlink module, and secondly is that it copies >>> runtime-image-link.delta into the run-time image. Our long standing >>> position is that the run-time image is created by jlink rather than a >>> combination of jlink and extra stuff copied in by the build. >>> >>> The part that I like with the current proposal is >>> lib/runtime-image-link.delta as it's less complicated that previous >>> iterations that added a resource into the jimage container. >>> >>> What would you (and @mlchung) think of having jlink generate >>> lib/runtime-image-link.delta as a step between the pipeline and image >>> generation? >> >> 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? > >> 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... - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2098895722
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v26]
> 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 105 commits: - 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 - Fix coment - Fix comment - Fix typo - Revert some now unneded build changes - Follow build tools naming convention - ... and 95 more: https://git.openjdk.org/jdk/compare/23a72a1f...67aea4ca - Changes: https://git.openjdk.org/jdk/pull/14787/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14787=25 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: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v8]
On Tue, 7 May 2024 09:36:10 GMT, Jan Kratochvil wrote: > Should JDK still support `memory.use_hierarchy == 0`? IMO, no. Just get rid of it and assume hierarchical everywhere. We'd be walking the hierarchy for other (lower limits), which should cover this case on those legacy systems. - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2097892763
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Fri, 3 May 2024 15:58:11 GMT, Severin Gehwolf wrote:
>> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 375:
>>
>>> 373: if (!c.isContainerized()) {
>>> 374: ostream.println(INDENT + "System not containerized.");
>>> 375: return;
>>
>> Why return here? Would this not cut the output short in the
>> non-containerized case?
>>
>> And if this not intended, the not-containerized-`-XshowSettings:system` test
>> below should test and catch this (e.g. scan for CPU set)
>
>> Why return here?
>
> Because it's not useful to see containerized settings (other than the cg
> version in use) after this patch. The JVM won't use them (uses the physical
> settings instead). Why would you want to show the settings?
To clarify. `showSettings:system` output on a host system:
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)
... and in a container (with memory limit 500m):
Operating System Metrics:
Provider: cgroupv1
Effective CPU Count: 12
CPU Period: 10us
CPU Quota: -1
CPU Shares: -1
List of Processors, 12 total:
0 1 2 3 4 5 6 7 8 9 10 11
List of Effective Processors, 12 total:
0 1 2 3 4 5 6 7 8 9 10 11
List of Memory Nodes, 1 total:
0
List of Available Memory Nodes, 1 total:
0
Memory Limit: 500.00M
Memory Soft Limit: Unlimited
Memory & Swap Limit: 500.00M
Maximum Processes Limit: 2048
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)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1589407238
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container
On Mon, 22 Apr 2024 13:56:23 GMT, Jan Kratochvil wrote: > Anyway in this patch one could unify naming across variables/parameters, the > same value is called `_is_ro`, `is_read_only`, `ro_opt`, `read_only`, `ro`. I've tried to unify the naming a bit. Thanks for the review! - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2093300919
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v3]
> 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/06fa7bd3...434430ca - Changes: - all: https://git.openjdk.org/jdk/pull/18201/files - new: https://git.openjdk.org/jdk/pull/18201/files/0df26ebd..434430ca Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18201=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18201=01-02 Stats: 82529 lines in 2377 files changed: 37138 ins; 34932 del; 10459 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:21:29 GMT, Thomas Stuefe wrote: > Why return here? Because it's not useful to see containerized settings (other than the cg version in use) after this patch. The JVM won't use them (uses the physical settings instead). Why would you want to show the settings? - PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1589396352
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Tue, 16 Apr 2024 18:10:08 GMT, Thomas Stuefe wrote: >> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 351: >> >>> 349: // >>> 350: // We collect the read only mount option in the cgroup infos so as >>> to have that >>> 351: // info ready when determining is_containerized(). >> >> Here, and in other places: a comment indicating the line format we scan >> would be appreciated, possibly with argument numbers. Saves the casual code >> reader from looking into proc man page. Even just pasting the example line >> for proc manpage would be fine >> (https://man7.org/linux/man-pages/man5/proc.5.html) (but with order adapted >> to your scanf call, they count major:minor as one) > > Trying to parse the `%s%*[^-]-` > > So, %s parses the mount options, until we encounter whitespace. Then %*[^-]- > parses everything that is not a dash, until we encounter the dash? Then we > eat the dash? This is to skip the optionals? Correct. Note that `%s %*[^-]` doesn't work for files without optionals. Since `%*[^-]` requires a non-empty match and the optionals are, well, optional :-) I've added more verbose comments to clarify this. - PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1589390841
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Tue, 16 Apr 2024 18:16:33 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 > > src/hotspot/os/linux/osContainer_linux.cpp line 78: > >> 76: const char *reason; >> 77: bool any_mem_cpu_limit_present = false; >> 78: bool ctrl_ro = cgroup_subsystem->is_containerized(); > > nit: naming? what does ctrl mean in this case? Maybe use > "cgroup_is_containerized"? `ctrl` was short for `controller`. I've changed the naming. - PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1589391426
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v8]
On Sun, 10 Mar 2024 14:40:09 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 35 commits: > > - Fix whitespace > - Merge branch 'master' into master-cgroup > >Conflicts: > test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp > - Fix gtest > - Update the Java part > - Fix cgroup1 backward compatibility message > - Merge remote-tracking branch 'centos79/master-cgroup' into master-cgroup > - Disable cgroup.subtree_control testcase on cgroup1 > - Fix gtest > - Merge branch 'master' into master-cgroup > - Merge remote-tracking branch 'f38crac/master-cgroup' into master-cgroup > - ... and 25 more: https://git.openjdk.org/jdk/compare/243cb098...39c90162 > Should I rebase it on top of the commit > [jerboaa@92aaa6f](https://github.com/jerboaa/jdk/commit/92aaa6fd7e3ff8b64de064fecfcd725a157cb5bb) > or wait until you finish it? Up to you. I'll keep you posted once the PRs are out. - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2085319806
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v8]
On Tue, 30 Apr 2024 07:01:16 GMT, Jan Kratochvil wrote: > * Will the patch be accepted only for memory or it has to support also CPU? It should be fine for memory only for a start, but we should allow for on-boarding of other controllers as well. > * Should I code it on top of OpenJDK trunk or on top of > [jerboaa@92aaa6f](https://github.com/jerboaa/jdk/commit/92aaa6fd7e3ff8b64de064fecfcd725a157cb5bb) > ? Since this patch solves a similar problem than https://bugs.openjdk.java.net/browse/JDK-8217338 did at the time when only cg v1 was supported I'd feel more comfortable in paying the technical debt and recode it so that cg v1 and cg v2 behaves the same or at least similar. That should make the code cleaner, easier to test and maintain in the long run. So I'm thinking on top of the refactoring. I'll try to prioritize those work items accordingly. Does that sound OK? - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2085186173
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v8]
On Sun, 10 Mar 2024 14:40:09 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 35 commits: > > - Fix whitespace > - Merge branch 'master' into master-cgroup > >Conflicts: > test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp > - Fix gtest > - Update the Java part > - Fix cgroup1 backward compatibility message > - Merge remote-tracking branch 'centos79/master-cgroup' into master-cgroup > - Disable cgroup.subtree_control testcase on cgroup1 > - Fix gtest > - Merge branch 'master' into master-cgroup > - Merge remote-tracking branch 'f38crac/master-cgroup' into master-cgroup > - ... and 25 more: https://git.openjdk.org/jdk/compare/243cb098...39c90162 Thanks for the updates. I like that we have consistency between cgv1 and cgv2 in the latest version in terms of hierarchical limit. What would be even better is to get consistency between CPU and memory lookup (if the restriction is enforced higher up the hierarchy). That is, it would be ideal to make `initialize_hierarchy()` controller specific. Meanwhile I've been working on [some refactoring](https://github.com/jerboaa/jdk/commit/92aaa6fd7e3ff8b64de064fecfcd725a157cb5bb) which builds on top of [JDK-8302744](https://bugs.openjdk.org/browse/JDK-8302744) so as to make the code a bit nicer once this integrates. Then, the idea would be to use scratch controllers (`CgroupCpuController` and `CgroupMemoryController`) to determine whether or not there is a limit and figure out the actual path on a per-controller specific way - (use `CgroupMemoryController->read_memory_limit_in_bytes(phys_mem)` and `CgroupUtil::processor_count(CgroupCpuController* cpu_ctrl, int host_cpus)` in the process). Does that make sense? A few other observations: - The common case is when the JVM runs in a container. Then, the cgroup path is `/` on cgv2 and the and `root_mount == cgroup_path` on cgv1. We don't need to do the extra processing on those systems as the limit will be at the leaf. - The (fairly) uncommon case is the host case where the cgroup limit is applied elsewhere (e.g. systemd slice). This is where we'd need the hierarchy walk. - When we need to walk the hierarchy, we start at the longest path and only traverse if there is _NO_ limit. A system which sets a higher, limit (that isn't `max`), seems ill-defined and I've not come across one. As soon as we've found a lower value than unlimited (`-1`), we stop. Since cg2 is hierarchical, the lowest limit will affect the entire tree (corollary: higher values further down from that point won't have an effect): ``` /a/b --> memory.max 300 `- /c --> memory.max max (this wouldn't have any effect, therefore can be ignored). ``` - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2077263573
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Thu, 11 Apr 2024 12:08:02 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 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 Thanks for your input Larry! - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2066136268
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Thu, 18 Apr 2024 13:27:38 GMT, Jan Kratochvil wrote: > Could not we rename `is_containerized()` to `use_container_limit()` ? As that > is the current only purpose of `is_containerized()`. I'm not sure. There is value to have `is_containerized()` like it would behave after this patch. Specifically the first table row difference in [your comment](https://github.com/openjdk/jdk/pull/18201#issuecomment-2063868908) concerns me. JVMs running in a container without limit wouldn't be detected as "containerized". That seems a large share of deployments to miss. - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2064487567
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Wed, 17 Apr 2024 01:07:04 GMT, Jan Kratochvil wrote: >>> IMHO `is_containerized()` is OK to return `false` even when running in a >>> container but with no limitations set. >> >> The idea here is to use this property to tune OpenJDK for in-container, >> specifically k8s, use. In such a setup it's custom to run a single process >> within set resource constraints. In order to do this, we need a reliable way >> to distinguish that vs. non-containerized setup. If somebody really wants to >> run OpenJDK in a container expecting it to run like a physical OpenJDK >> deployment, that's when `-XX:-UseContainerSupport` should be used. > >> The idea here is to use this property to tune OpenJDK for in-container, >> specifically k8s, use. In such a setup it's custom to run a single process >> within set resource constraints. > > The in-container tuning means to use all the available resources. Containers > in the real world have some memory limits set which is where my modified > patch still correctly identifies it as a container to use all the available > resources of the node which is the whole goal of the container detection code. > >> In order to do this, we need a reliable way to distinguish that vs. >> non-containerized setup. > > I expect it should have been written "We need a reliable way to distinguish > real world in-container vs. non-containerized setup. We do not mind behavior > for artificial containers on OpenJDK development machines.". Which is what my > patch does in an easier and less error-prone way. > >> If somebody really wants to run OpenJDK in a container expecting it to run >> like a physical OpenJDK deployment, that's when `-XX:-UseContainerSupport` >> should be used. > > That behaves still the same with my patch. > > Could you give a countercase where my patch behaves wrongly? @jankratochvil I believe this boils down to what we actually want. Should `OSContainer::is_containerized()` return `false` when run *inside* a container? If so, when is it OK to do that? Should `OSContainer::is_containerized()` return `true` on a physical Linux deployment? IMO, the read-only property of the mount points was something that fit naturally since, we already scan those anyway for (cgv1 vs cgv2 detection). Therefore it was something to consider to make heuristics more accurate. The truth table of the patch in this PR looks like this: | `OSContainer::is_containerized()` value | Actual deployment scenario | | - | - | | `true` | OpenJDK runs in an unprivileged container **without** a cpu/memory limit | | `true` | OpenJDK runs in an unprivileged container **with** a cpu/memory limit | | `true` | OpenJDK runs in a privileged container **with** a cpu/memory limit | | `false` | OpenJDK runs in a privileged container **without** a cpu/memory limit | | `false` | OpenJDK runs in a systemd slice **without** a cpu/memory limit | | `true` | OpenJDK runs in a systemd slice **with** a cpu/memory limit | | `false` | OpenJDK runs on a physical Linux system (VM or bare metal) | As you can see, the case of "OpenJDK runs in a privileged container *without* a cpu/memory limit" gives the wrong result. However, I consider this a fairly uncommon setup and there isn't really anything we can do to detect this kind of setup. Even if we did manage to detect it, why would we care? It's a question of probability. > Could you give a countercase where my patch behaves wrongly? Again, it's not a case of right or wrong IMO. Since we are in the land of heuristics, they will be wrong in some cases. We should make them so that we cover the common cases and, perhaps, are able to use those in serviceability tools to help guide diagnosis and/or further tuning. So far the existing capabilities were OK, but prevent further out-of-the-box tuning and/or accurate data collection. Your proposed patch (it's one I had in a previous iteration too, fwiw) would also return `false` for the case of "OpenJDK runs in an unprivileged container **without** a cpu/memory limit", which seems counterintuitive if OpenJDK actually runs in a container! What's more, it seems a fairly common case. Also, there is a chance of the OpenJDK heuristics to fail memory/cpu limit detection because of bugs and new developments. It seems the safer option to know that OpenJDK is containerized (using other heuristics) in that case. Maybe that's just me. Let's have that discussion more broadly and see if we can reach consensus! - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2063477204
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Tue, 16 Apr 2024 13:54:53 GMT, Alan Bateman wrote: > 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. OK. If a hidden jlink option is good enough, then this works for me. I hope to have time to update the patch in the coming days. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2060691122
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Tue, 16 Apr 2024 14:40:46 GMT, Jan Kratochvil wrote: > IMHO `is_containerized()` is OK to return `false` even when running in a > container but with no limitations set. The idea here is to use this property to tune OpenJDK for in-container, specifically k8s, use. In such a setup it's custom to run a single process within set resource constraints. In order to do this, we need a reliable way to distinguish that vs. non-containerized setup. If somebody really wants to run OpenJDK in a container expecting it to run like a physical OpenJDK deployment, that's when `-XX:-UseContainerSupport` should be used. - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2059344194
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Wed, 3 Apr 2024 22:31:39 GMT, Mandy Chung wrote: >> Severin Gehwolf has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Move CreateLinkableRuntimePlugin to build folder >> >> Keep runtime link supporting classes in package >> jdk.tools.jlink.internal.runtimelink > > Thanks for the investigation w.r.t. extending jimage. It does not seem worth > the effort in pursuing the support of adding resources to an existing jimage > file. To me, putting the diff data under `lib` directory as a private file > is a simpler and acceptable solution. It allows the second jlink invocation > to avoid the plugin pipeline if the per-module metadata is generated in > normal jlink invocation. > > (An observation: The current implementation requires the diffs to be > generated as a plugin. For this approach, it may be possible to implement > with a separate main class that calls JLink API and generates the diffs.) > > @mlchung @AlanBateman Any thoughts on this latest version? Is this going > > into the direction you had envisioned? Any more blockers? The CSR should be > > up-to-date and is open for review as well. If no more blockers I'll go over > > this patch once more and clean it up to prepare for integration. Thanks! > > Thanks for all the efforts on this. Thanks for looking at this, Alan! > I've looked through the latest version. I'm a little bit comfortable with > LinkDeltaProducer. One reason it's build tool that is tied tied to code in > the jdk.jlink module, and secondly is that it copies runtime-image-link.delta > into the run-time image. Our long standing position is that the run-time > image is created by jlink rather than a combination of jlink and extra stuff > copied in by the build. > > The part that I like with the current proposal is > lib/runtime-image-link.delta as it's less complicated that previous > iterations that added a resource into the jimage container. > > What would you (and @mlchung) think of having jlink generate > lib/runtime-image-link.delta as a step between the pipeline and image > generation? 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? - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2058919838
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
> 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 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 - Changes: - all: https://git.openjdk.org/jdk/pull/18201/files - new: https://git.openjdk.org/jdk/pull/18201/files/98325f18..0df26ebd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18201=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18201=00-01 Stats: 407791 lines in 3887 files changed: 43423 ins; 33650 del; 330718 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
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? Gentle ping. - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2049297883
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Wed, 3 Apr 2024 22:31:39 GMT, Mandy Chung wrote: >> Severin Gehwolf has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Move CreateLinkableRuntimePlugin to build folder >> >> Keep runtime link supporting classes in package >> jdk.tools.jlink.internal.runtimelink > > Thanks for the investigation w.r.t. extending jimage. It does not seem worth > the effort in pursuing the support of adding resources to an existing jimage > file. To me, putting the diff data under `lib` directory as a private file > is a simpler and acceptable solution. It allows the second jlink invocation > to avoid the plugin pipeline if the per-module metadata is generated in > normal jlink invocation. > > (An observation: The current implementation requires the diffs to be > generated as a plugin. For this approach, it may be possible to implement > with a separate main class that calls JLink API and generates the diffs.) @mlchung @AlanBateman Any thoughts on this latest version? Is this going into the direction you had envisioned? Any more blockers? The CSR should be up-to-date and is open for review as well. If no more blockers I'll go over this patch once more and clean it up to prepare for integration. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2044593102
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v24]
On Fri, 5 Apr 2024 09:25:49 GMT, Magnus Ihse Bursie wrote: >> Thanks. Yes, the long name was my doing. Sorry. > > Kind of aligning with the "Donaudampfschiffahrtsgesellschaftskapitän" > prejudice of German. ;-) > > > > (In Sweden, we have "flaggstångsknoppsförgyllare" so you are not alone) Hah! My kids just recently informed me about "Donaudampfschiffahrtsgesellschaftskapitänsmützenproductionsstätte"... or whatever else you can come up with :) - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1553260930
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v24]
On Thu, 4 Apr 2024 20:56:02 GMT, Magnus Ihse Bursie wrote: >> I was not aware of such a convention and I can't say I agree with it. It >> just seems redundant and unnecessary, but I suppose we should wait for >> Magnus to respond. > > Just to clarify: I did not say the name needed to be long, just that many (if > not all) tools has used the convention of using the package name > `build.tools.` and the class name `.java`. > > I think the new name sounds . Thanks. Yes, the long name was my doing. Sorry. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1553173105
Re: RFR: 8328366: Thread.setContextClassloader from thread in FJP commonPool task no longer works after JDK-8327501
On Tue, 19 Mar 2024 12:16:29 GMT, Viktor Klang wrote: > This is a draft PR with a potential solution to 8328366 without regressing > 8327501. > > @DougLea To avoid regressions in the future, where would regression tests for > this ideally be located? FWIW, I've tested this with the quarkus reproducer and a reproducer for the class load leak. They seem both to work with this patch. Please be sure to mention the bug for the ThreadLocal follow-up. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/18374#issuecomment-2037767866
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]
> 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 three additional commits since the last revision: - Use shorter name for the build tool - Review feedback from Erik J. - Remove dependency on CDS which isn't needed anymore - Changes: - all: https://git.openjdk.org/jdk/pull/14787/files - new: https://git.openjdk.org/jdk/pull/14787/files/ce04f42a..84d4feff Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14787=24 - incr: https://webrevs.openjdk.org/?repo=jdk=14787=23-24 Stats: 32 lines in 3 files changed: 3 ins; 16 del; 13 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 [v24]
On Thu, 4 Apr 2024 12:56:41 GMT, Erik Joelsson 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 100 commits: >> >> - Fix coment >> - Fix comment >> - Fix typo >> - Revert some now unneded build changes >> - Follow build tools naming convention >> - Update to new build-time approach with delta in lib >> - Make generation of fs_$module_files unconditional >> - Merge branch 'master' into jdk-8311302-jmodless-link >> - Fix copyright year >> - Move CreateLinkableRuntimePlugin to build folder >> >>Keep runtime link supporting classes in package >>jdk.tools.jlink.internal.runtimelink >> - ... and 90 more: https://git.openjdk.org/jdk/compare/6ae1cf12...ce04f42a > > make/CompileToolsJdk.gmk line 50: > >> 48: EXCLUDES := \ >> 49: build/tools/classlist \ >> 50: build/tools/runtimeimagelinkdeltaproducer \ > > This directory name is comically long. I guess that's not really a problem, > but perhaps "linkdelta" would be descriptive enough given that the class has > the full name anyway? FYI: This was trying to address a comment from @magicus https://github.com/openjdk/jdk/pull/14787#discussion_r1514894494 Happy to not follow that convention and/or rename the tool. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1551909878
