Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v5]
On Wed, 24 Feb 2021 17:37:13 GMT, Severin Gehwolf wrote: >> Marked as reviewed by andrew (Reviewer). > > @gnu-andrew Thanks for the review! I'll retest and then integrate. Tests look good on my end. Also this check passed all Linux tests (it's a no-op everywhere else): https://github.com/jerboaa/jdk/actions/runs/596859548 - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v5]
On Wed, 24 Feb 2021 16:24:32 GMT, Andrew John Hughes wrote: >> Severin Gehwolf has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add comment to parsing logic of /proc/self/cgroup > > Marked as reviewed by andrew (Reviewer). @gnu-andrew Thanks for the review! I'll retest and then integrate. - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v6]
> This is an enhancement which solves two issues: > > 1. Multiple reads of relevant cgroup interface files. Now interface files are > only read once per file (just like Hotspot). > 2. Proxies creation of the impl specific subsystem via `determineType()` as > before, but now reads all relevant interface files: `/proc/cgroups`, > `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the > parsed information to the impl specific subsystem classes for instantiation. > This allows for more flexibility of testing as interface files can be mocked > and, thus, more cases can be tested that way without having access to these > specific systems. For example, proper regression tests for JDK-8217766 and > JDK-8253435 have been added now with this in place. > > * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests > pass. Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision: - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics - Add comment to parsing logic of /proc/self/cgroup - Fix jcheck - Add documentation and reduce code running in the critical section - Add some documentation - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics - 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection - Changes: - all: https://git.openjdk.java.net/jdk/pull/1393/files - new: https://git.openjdk.java.net/jdk/pull/1393/files/078e16db..5d5275e7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1393&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1393&range=04-05 Stats: 29136 lines in 793 files changed: 18085 ins; 6354 del; 4697 mod Patch: https://git.openjdk.java.net/jdk/pull/1393.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1393/head:pull/1393 PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v5]
On Fri, 12 Feb 2021 12:49:01 GMT, Severin Gehwolf wrote: >> This is an enhancement which solves two issues: >> >> 1. Multiple reads of relevant cgroup interface files. Now interface files >> are only read once per file (just like Hotspot). >> 2. Proxies creation of the impl specific subsystem via `determineType()` as >> before, but now reads all relevant interface files: `/proc/cgroups`, >> `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the >> parsed information to the impl specific subsystem classes for instantiation. >> This allows for more flexibility of testing as interface files can be mocked >> and, thus, more cases can be tested that way without having access to these >> specific systems. For example, proper regression tests for JDK-8217766 and >> JDK-8253435 have been added now with this in place. >> >> * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests >> pass. > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Add comment to parsing logic of /proc/self/cgroup Marked as reviewed by andrew (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v4]
On Fri, 12 Feb 2021 12:49:19 GMT, Severin Gehwolf wrote: >> Setting reviewers to 2 since I want @gnu-andrew to be OK with it too. > > Hi Harold, > >> Thanks for doing this! Sorry for taking so long to review this change. The >> change looks good. > > Thanks for the review! > >> Before pushing it, could you add a comment explaining what the code in lines >> 185-194 of CgroupSubsystemFactory.java is doing? > > Done. > >> Also, please don't overwrite the fix for JDK-8257746. > > AFAIK this patch doesn't touch this code. So this should not happen > (overwrite of the fix). FWIW, once this is in I intend to propose a patch > which adds a regression test for JDK-8257746. It would depend on this change. > > Thanks, > Severin Thanks, this version looks better. Sorry for the delay. I didn't realise you'd replied until I received your e-mail. Not sure where notifications for these git PRs end up. - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v4]
On Fri, 12 Feb 2021 10:19:03 GMT, Severin Gehwolf wrote: >> Hi Severin, >> Thanks for doing this! Sorry for taking so long to review this change. The >> change looks good. Before pushing it, could you add a comment explaining >> what the code in lines 185-194 of CgroupSubsystemFactory.java is doing? >> Also, please don't overwrite the fix for JDK-8257746. >> Thanks again! Harold > > Setting reviewers to 2 since I want @gnu-andrew to be OK with it too. Hi Harold, > Thanks for doing this! Sorry for taking so long to review this change. The > change looks good. Thanks for the review! > Before pushing it, could you add a comment explaining what the code in lines > 185-194 of CgroupSubsystemFactory.java is doing? Done. > Also, please don't overwrite the fix for JDK-8257746. AFAIK this patch doesn't touch this code. So this should not happen (overwrite of the fix). FWIW, once this is in I intend to propose a patch which adds a regression test for JDK-8257746. It would depend on this change. Thanks, Severin - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v5]
> This is an enhancement which solves two issues: > > 1. Multiple reads of relevant cgroup interface files. Now interface files are > only read once per file (just like Hotspot). > 2. Proxies creation of the impl specific subsystem via `determineType()` as > before, but now reads all relevant interface files: `/proc/cgroups`, > `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the > parsed information to the impl specific subsystem classes for instantiation. > This allows for more flexibility of testing as interface files can be mocked > and, thus, more cases can be tested that way without having access to these > specific systems. For example, proper regression tests for JDK-8217766 and > JDK-8253435 have been added now with this in place. > > * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests > pass. Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision: Add comment to parsing logic of /proc/self/cgroup - Changes: - all: https://git.openjdk.java.net/jdk/pull/1393/files - new: https://git.openjdk.java.net/jdk/pull/1393/files/9a2d6a20..078e16db Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1393&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1393&range=03-04 Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1393.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1393/head:pull/1393 PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v4]
On Thu, 11 Feb 2021 19:51:43 GMT, Harold Seigel 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 seven additional >> commits since the last revision: >> >> - Fix jcheck >> - Add documentation and reduce code running in the critical section >> - Add some documentation >> - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics >> - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics >> - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics >> - 8254001: [Metrics] Enhance parsing of cgroup interface files for version >> detection > > Hi Severin, > Thanks for doing this! Sorry for taking so long to review this change. The > change looks good. Before pushing it, could you add a comment explaining > what the code in lines 185-194 of CgroupSubsystemFactory.java is doing? > Also, please don't overwrite the fix for JDK-8257746. > Thanks again! Harold Setting reviewers to 2 since I want @gnu-andrew to be OK with it too. - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v4]
On Tue, 9 Feb 2021 13:31:25 GMT, Severin Gehwolf wrote: >> This is an enhancement which solves two issues: >> >> 1. Multiple reads of relevant cgroup interface files. Now interface files >> are only read once per file (just like Hotspot). >> 2. Proxies creation of the impl specific subsystem via `determineType()` as >> before, but now reads all relevant interface files: `/proc/cgroups`, >> `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the >> parsed information to the impl specific subsystem classes for instantiation. >> This allows for more flexibility of testing as interface files can be mocked >> and, thus, more cases can be tested that way without having access to these >> specific systems. For example, proper regression tests for JDK-8217766 and >> JDK-8253435 have been added now with this in place. >> >> * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests >> pass. > > 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 seven additional > commits since the last revision: > > - Fix jcheck > - Add documentation and reduce code running in the critical section > - Add some documentation > - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics > - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics > - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics > - 8254001: [Metrics] Enhance parsing of cgroup interface files for version > detection Hi Severin, Thanks for doing this! Sorry for taking so long to review this change. The change looks good. Before pushing it, could you add a comment explaining what the code in lines 185-194 of CgroupSubsystemFactory.java is doing? Also, please don't overwrite the fix for JDK-8257746. Thanks again! Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 1 Feb 2021 20:26:54 GMT, Andrew John Hughes wrote:
>> Anybody willing to review this?
>
>> Anybody willing to review this?
>
> I can have a go.
>
> I have two main concerns:
>
> 1. There seems to be little documentation on the new additions. I'm
> particularly concerned about things like CgroupV1Subsystem.java where a big
> chunk of documentation on the formats being read is removed and I don't see
> it being moved elsewhere. Documentation on the new getInstance methods would
> be helpful too.
> 2. With the new volatile fields, I think it would be better if the lock being
> held to initialise them was only held when necessary to perform the
> assignment. Holding it while performing an extensive process of parsing
> multiple files that could potentially fail seems a little dangerous. i.e.
> something like:
> if (INSTANCE == null) { CGroupV1Subsystem tmp = initSubSystem(infos);
> synchronized (CgroupV1Subsystem.class) { if (INSTANCE == null) { INSTANCE =
> tmp; } } }
@gnu-andrew Let me know what you think of the updated patch! Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 1 Feb 2021 20:26:54 GMT, Andrew John Hughes wrote:
> > Anybody willing to review this?
>
> I can have a go.
>
> I have two main concerns:
>
> 1. There seems to be little documentation on the new additions. I'm
> particularly concerned about things like CgroupV1Subsystem.java where a big
> chunk of documentation on the formats being read is removed and I don't see
> it being moved elsewhere. Documentation on the new getInstance methods would
> be helpful too.
The updated patch now includes some more documentation. The reason why I've
removed some of it was because the logic changed and it was outdated.
> 2. With the new volatile fields, I think it would be better if the lock
> being held to initialise them was only held when necessary to perform the
> assignment. Holding it while performing an extensive process of parsing
> multiple files that could potentially fail seems a little dangerous. i.e.
> something like:
>if (INSTANCE == null) { CGroupV1Subsystem tmp = initSubSystem(infos);
> synchronized (CgroupV1Subsystem.class) { if (INSTANCE == null) { INSTANCE =
> tmp; } } }
I've changed this as suggested, but keep in mind no parsing of multiple files
happens after this patch. It happens in CgroupSubsystemFactory.
Thanks,
Severin
-
PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v4]
> This is an enhancement which solves two issues: > > 1. Multiple reads of relevant cgroup interface files. Now interface files are > only read once per file (just like Hotspot). > 2. Proxies creation of the impl specific subsystem via `determineType()` as > before, but now reads all relevant interface files: `/proc/cgroups`, > `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the > parsed information to the impl specific subsystem classes for instantiation. > This allows for more flexibility of testing as interface files can be mocked > and, thus, more cases can be tested that way without having access to these > specific systems. For example, proper regression tests for JDK-8217766 and > JDK-8253435 have been added now with this in place. > > * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests > pass. 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 seven additional commits since the last revision: - Fix jcheck - Add documentation and reduce code running in the critical section - Add some documentation - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics - 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection - Changes: - all: https://git.openjdk.java.net/jdk/pull/1393/files - new: https://git.openjdk.java.net/jdk/pull/1393/files/0ece5f22..9a2d6a20 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1393&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1393&range=02-03 Stats: 100580 lines in 2281 files changed: 44215 ins; 36687 del; 19678 mod Patch: https://git.openjdk.java.net/jdk/pull/1393.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1393/head:pull/1393 PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Tue, 12 Jan 2021 14:29:28 GMT, Severin Gehwolf wrote:
> Anybody willing to review this?
I can have a go.
I have two main concerns:
1. There seems to be little documentation on the new additions. I'm
particularly concerned about things like CgroupV1Subsystem.java where a big
chunk of documentation on the formats being read is removed and I don't see it
being moved elsewhere. Documentation on the new getInstance methods would be
helpful too.
2. With the new volatile fields, I think it would be better if the lock being
held to initialise them was only held when necessary to perform the assignment.
Holding it while performing an extensive process of parsing multiple files that
could potentially fail seems a little dangerous. i.e. something like:
if (INSTANCE == null) { CGroupV1Subsystem tmp = initSubSystem(infos);
synchronized (CgroupV1Subsystem.class) { if (INSTANCE == null) { INSTANCE =
tmp; } } }
-
PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Tue, 12 Jan 2021 14:29:28 GMT, Severin Gehwolf wrote: >> Ping? Anyone? > > Anybody willing to review this? PING? - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v3]
> This is an enhancement which solves two issues: > > 1. Multiple reads of relevant cgroup interface files. Now interface files are > only read once per file (just like Hotspot). > 2. Proxies creation of the impl specific subsystem via `determineType()` as > before, but now reads all relevant interface files: `/proc/cgroups`, > `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the > parsed information to the impl specific subsystem classes for instantiation. > This allows for more flexibility of testing as interface files can be mocked > and, thus, more cases can be tested that way without having access to these > specific systems. For example, proper regression tests for JDK-8217766 and > JDK-8253435 have been added now with this in place. > > * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests > pass. 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-8254001-enhance-file-parsing-java-metrics - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics - 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection - Changes: - all: https://git.openjdk.java.net/jdk/pull/1393/files - new: https://git.openjdk.java.net/jdk/pull/1393/files/fd55ffd7..0ece5f22 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1393&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1393&range=01-02 Stats: 53075 lines in 1681 files changed: 24628 ins; 14727 del; 13720 mod Patch: https://git.openjdk.java.net/jdk/pull/1393.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1393/head:pull/1393 PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
Hi David, On Wed, 2021-01-13 at 11:33 +1000, David Holmes wrote: Hi Severin, On 13/01/2021 12:31 am, Severin Gehwolf wrote: > On Tue, 15 Dec 2020 12:57:12 GMT, Severin Gehwolf > wrote: > > > > @bobvandette Please review when you've got some cycles to spare. > > > Much appreciated! > > > > Ping? Anyone? > > Anybody willing to review this? FYI Bob retired at the end of last year. I don't know who currently has working knowledge of Cgroups on the core-libs and serviceability mailing lists. Yes, thanks. That seems to be my problem, actually. I can't review it myself :-/ Cheers, Severin
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
Hi Severin, On 13/01/2021 12:31 am, Severin Gehwolf wrote: On Tue, 15 Dec 2020 12:57:12 GMT, Severin Gehwolf wrote: @bobvandette Please review when you've got some cycles to spare. Much appreciated! Ping? Anyone? Anybody willing to review this? FYI Bob retired at the end of last year. I don't know who currently has working knowledge of Cgroups on the core-libs and serviceability mailing lists. David - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Tue, 15 Dec 2020 12:57:12 GMT, Severin Gehwolf wrote: >> @bobvandette Please review when you've got some cycles to spare. Much >> appreciated! > > Ping? Anyone? Anybody willing to review this? - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 23 Nov 2020 19:58:20 GMT, Severin Gehwolf wrote: >>> With respect to JDK-8255908, the changes look good to me. >> >> Thanks! > > @bobvandette Please review when you've got some cycles to spare. Much > appreciated! Ping? Anyone? - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v2]
> This is an enhancement which solves two issues: > > 1. Multiple reads of relevant cgroup interface files. Now interface files are > only read once per file (just like Hotspot). > 2. Proxies creation of the impl specific subsystem via `determineType()` as > before, but now reads all relevant interface files: `/proc/cgroups`, > `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the > parsed information to the impl specific subsystem classes for instantiation. > This allows for more flexibility of testing as interface files can be mocked > and, thus, more cases can be tested that way without having access to these > specific systems. For example, proper regression tests for JDK-8217766 and > JDK-8253435 have been added now with this in place. > > * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests > pass. Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics - 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection - Changes: - all: https://git.openjdk.java.net/jdk/pull/1393/files - new: https://git.openjdk.java.net/jdk/pull/1393/files/1832b70f..fd55ffd7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1393&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1393&range=00-01 Stats: 142480 lines in 1880 files changed: 116851 ins; 13097 del; 12532 mod Patch: https://git.openjdk.java.net/jdk/pull/1393.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1393/head:pull/1393 PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 23 Nov 2020 19:39:18 GMT, Poonam Bajaj wrote: > With respect to JDK-8255908, the changes look good to me. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 23 Nov 2020 19:56:56 GMT, Severin Gehwolf wrote: >> With respect to JDK-8255908, the changes look good to me. > >> With respect to JDK-8255908, the changes look good to me. > > Thanks! @bobvandette Please review when you've got some cycles to spare. Much appreciated! - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 23 Nov 2020 15:50:18 GMT, Severin Gehwolf wrote: >> This is an enhancement which solves two issues: >> >> 1. Multiple reads of relevant cgroup interface files. Now interface files >> are only read once per file (just like Hotspot). >> 2. Proxies creation of the impl specific subsystem via `determineType()` as >> before, but now reads all relevant interface files: `/proc/cgroups`, >> `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the >> parsed information to the impl specific subsystem classes for instantiation. >> This allows for more flexibility of testing as interface files can be mocked >> and, thus, more cases can be tested that way without having access to these >> specific systems. For example, proper regression tests for JDK-8217766 and >> JDK-8253435 have been added now with this in place. >> >> * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests >> pass. > > @poonamparhar Perhaps you want to take a look at this in the context of not > regressing JDK-8255908? Thanks! With respect to JDK-8255908, the changes look good to me. - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 23 Nov 2020 15:46:56 GMT, Severin Gehwolf wrote:
> This is an enhancement which solves two issues:
>
> 1. Multiple reads of relevant cgroup interface files. Now interface files are
> only read once per file (just like Hotspot).
> 2. Proxies creation of the impl specific subsystem via `determineType()` as
> before, but now reads all relevant interface files: `/proc/cgroups`,
> `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the
> parsed information to the impl specific subsystem classes for instantiation.
> This allows for more flexibility of testing as interface files can be mocked
> and, thus, more cases can be tested that way without having access to these
> specific systems. For example, proper regression tests for JDK-8217766 and
> JDK-8253435 have been added now with this in place.
>
> * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests
> pass.
test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java line 238:
> 236: assertFalse("Join controller combination expected as cgroups
> v1", res.isCgroupV2());
> 237: CgroupInfo memoryInfo = res.getInfos().get("memory");
> 238: assertEquals("/user.slice/user-1000.slice/session-3.scope",
> memoryInfo.getCgroupPath());
The gist of the Java equivalent of JDK-8253939 (which fixed this for Hotspot).
I.e. a regression test for JDK-8217766 and JDK-8254854.
test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java line 267:
> 265: assertFalse("Duplicate cpusets should not influence detection
> heuristic", res.isCgroupV2());
> 266: CgroupInfo cpuSetInfo = res.getInfos().get("cpuset");
> 267: assertEquals("/sys/fs/cgroup/cpuset",
> cpuSetInfo.getMountPoint());
We can now assert the proper mount point is being used for multiple cpuset
mounts.
-
PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 23 Nov 2020 15:46:56 GMT, Severin Gehwolf wrote: > This is an enhancement which solves two issues: > > 1. Multiple reads of relevant cgroup interface files. Now interface files are > only read once per file (just like Hotspot). > 2. Proxies creation of the impl specific subsystem via `determineType()` as > before, but now reads all relevant interface files: `/proc/cgroups`, > `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the > parsed information to the impl specific subsystem classes for instantiation. > This allows for more flexibility of testing as interface files can be mocked > and, thus, more cases can be tested that way without having access to these > specific systems. For example, proper regression tests for JDK-8217766 and > JDK-8253435 have been added now with this in place. > > * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests > pass. @poonamparhar Perhaps you want to take a look at this in the context of not regressing JDK-8255908? Thanks! src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java line 268: > 266: info.setMountPoint(mountPath); > 267: info.setMountRoot(mountRoot); > 268: } This is the actual fix of JDK-8253435 for the Java side. - PR: https://git.openjdk.java.net/jdk/pull/1393
RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
This is an enhancement which solves two issues: 1. Multiple reads of relevant cgroup interface files. Now interface files are only read once per file (just like Hotspot). 2. Proxies creation of the impl specific subsystem via `determineType()` as before, but now reads all relevant interface files: `/proc/cgroups`, `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the parsed information to the impl specific subsystem classes for instantiation. This allows for more flexibility of testing as interface files can be mocked and, thus, more cases can be tested that way without having access to these specific systems. For example, proper regression tests for JDK-8217766 and JDK-8253435 have been added now with this in place. * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests pass. - Commit messages: - 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection Changes: https://git.openjdk.java.net/jdk/pull/1393/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1393&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8254001 Stats: 537 lines in 5 files changed: 268 ins; 193 del; 76 mod Patch: https://git.openjdk.java.net/jdk/pull/1393.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1393/head:pull/1393 PR: https://git.openjdk.java.net/jdk/pull/1393
