Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller

[Mikhailo Seledtsov]

On Mon, 23 May 2022 22:11:47 GMT, Ioi Lam  wrote:

> This PR fixes a bug found on an Ubuntu host that's mostly running with 
> cgroupv2, but there's a controller (freezer) that is mounted in cgroupv1 mode.
> 
> The container support code in the VM and JDK checks if we have simultaneously 
> mounted v1 and v2 containers. If so, we revert to "hybrid" mode (which is 
> essentially v1).
> 
> However, the "hybrid checks" only considers the following controllers that 
> Java cares about:
> 
> - cpu
> - cpuacct
> - cpuset
> - blkio
> - memory
> - pids
> 
> If only "freezer" is mounted in v1 mode, Java will start in V2 mode. Later, 
> when `setCgroupV2Path()` sees the first line in /proc/self/cgroup, it runs 
> into the assert.
> 
> 
> $ cat /proc/self/cgroup
> 1:freezer:/
> 0::/user.slice/user-1001.slice/session-85.scope
> 
> 
> The fix is simple -- when we get to `setCgroupV2Path()`, we have already 
> decided that the system has not mounted any v1 controllers that we care 
> about, so we should just ignore all the v1 controllers (which has a non-empty 
> name).

Marked as reviewed by mseledtsov (Committer).

Change looks good to me. Thanks for the fix.

-

PR: https://git.openjdk.java.net/jdk/pull/8858

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v9]

[Mikhailo Seledtsov]

On Mon, 2 May 2022 06:24:21 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview).
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. We 
>> can add additional labels, if required, as the PR progresses.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 13 commits:
> 
>  - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec
>  - Merge with jdk-19+20
>  - Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e
>  - Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69
>  - Refresh cf561073f48fad58e931a5285b92629aa83c9bd1
>  - Merge with jdk-19+19
>  - Refresh
>  - Refresh
>  - Refresh
>  - Refresh
>  - ... and 3 more: 
> https://git.openjdk.java.net/jdk/compare/16a8ebbf...51bc652d

Thank you for addressing my review feedback. HotSpot/Runtime, test library, 
common test and JFR test changes look good to me.

-

Marked as reviewed by mseledtsov (Committer).

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Mikhailo Seledtsov]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview).
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

I have reviewed the following portions of this change: 
  test/common, test/gtest, test/hotspot/runtime, test/jdk/jfr, test library

Feedback:
  - see several minor comments inline
  - under runtime/cds/appcds/test-classes there is an empty "TEST.properties" 
file; was it added by mistake?

With only a few minor comments, I approve of the code reviewed by me provided 
that my comments will be addressed.

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Mikhailo Seledtsov]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview).
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

test/lib/jdk/test/lib/thread/VThreadRunner.java line 61:

> 59: /**
> 60:  * Run a task in a virtual thread and wait for it to terminate.
> 61:  * If the task completse with an exception then it is thrown by this 
> method.

typo: completse --> completes

test/lib/jdk/test/lib/thread/VThreadRunner.java line 121:

> 119: /**
> 120:  * Run a task in a virtual thread and wait for it to terminate.
> 121:  * If the task completse with an exception then it is thrown by this 
> method.

completse --> completes

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Mikhailo Seledtsov]

On Fri, 29 Apr 2022 18:23:35 GMT, Mikhailo Seledtsov  
wrote:

>> Alan Bateman has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e
>
> test/jdk/jdk/jfr/api/consumer/TestManyClasses.java line 57:
> 
>> 55: int classLoaderCount = Integer.parseInt(args[0]);
>> 56: int classCount = Integer.parseInt(args[1]);
>> 57: for (int i = 0; i  
> Minor: Style: please insert space between < and classCount

Also, should this be i < classLoaderCount ??

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Mikhailo Seledtsov]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview).
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

test/jdk/jdk/jfr/api/consumer/TestManyClasses.java line 57:

> 55: int classLoaderCount = Integer.parseInt(args[0]);
> 56: int classCount = Integer.parseInt(args[1]);
> 57: for (int i = 0; i  62: theClass.newInstance();
> 63: TestEvent event = new TestEvent();
> 64: event.theClass = event;

Did you mean event.theClass = theClass instead ?

test/jdk/jdk/jfr/api/consumer/TestManyRecordings.java line 57:

> 55: int classLoaderCount = Integer.parseInt(args[0]);
> 56: int classCount = Integer.parseInt(args[1]);
> 57: for (int i = 0; i https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Mikhailo Seledtsov]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview).
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

test/hotspot/jtreg/runtime/vthread/StackChunkClassLoaderTest.java line 2:

> 1: /*
> 2:  * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.

Please update copyright year.

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8277992: Add fast jdk_svc subtests to jdk:tier3

[Mikhailo Seledtsov]

On Wed, 1 Dec 2021 02:29:14 GMT, David Holmes  wrote:

>> OpenJDK tiered tests definitions have the catch-all `tier4` that runs all 
>> tests not defined in the lower tiers. `hotspot:tier4` has lots of them, 
>> mostly long-running vmTestbase tests, which take many hours even on a very 
>> parallel machines.
>> 
>> This, unfortunately, has a chilling effect on `jdk:tier4`, which is seldom 
>> run by contributors, because `hotspot:tier4` is in the way. But, there are 
>> plenty of fast and stable tests in `jdk:tier4` that can be run in 
>> `jdk:tier3`. `jdk_svc` is the example of such tests: management features 
>> (including but not limited to JFR) are important to keep from regressions, 
>> and significant subset of them runs pretty fast.
>> 
>> So, it makes sense to move some `jdk_svc` tests to `jdk:tier3` to expose it 
>> to more contributors. I think the only group we don't want to run is 
>> `svc_tools`, which includes lots of non-parallel tests that are rather slow.
>> 
>> Sample run before:
>> 
>> 
>> ==
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR 
>>   
>>jtreg:test/jdk:tier3174   174 0 0 
>>   
>> ==
>> TEST SUCCESS
>> 
>> Finished building target 'run-test' in configuration 
>> 'linux-x86_64-server-fastdebug'
>> 
>> real 2m38.242s
>> user 80m7.216s
>> sys  2m13.846s
>> 
>> 
>> ==
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR 
>>   
 jtreg:test/jdk:tier4   2904  2901 3 0 
 <<
>> ==
>> TEST FAILURE
>> 
>> real 18m13.933s
>> user 546m50.556s
>> sys  25m7.086s
>> 
>> 
>> Sample run after:
>> 
>> 
>> ==
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR 
>>   
>>jtreg:test/jdk:tier3   1296  1296 0 0 
>>   
>> ==
>> TEST SUCCESS
>> 
>> Finished building target 'run-test' in configuration 
>> 'linux-x86_64-server-fastdebug'
>> 
>> real 7m49.017s
>> user 287m30.943s
>> sys  13m20.060s
>> 
>> ==
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR 
>>   
 jtreg:test/jdk:tier4   1783  1780 3 0 
 <<
>> ==
>> TEST FAILURE
>> 
>> 
>> real 12m19.973s
>> user 351m48.561s
>> sys  14m51.566s
>
> Hi @shipilev ,
> We need to have someone look at the impact of this on our CI. I don't think 
> we run the tier4 group as defined in our tier 4 so we may not see those 
> execution time savings that offset the extra cost to tier3.

Thanks @dholmes-ora for making sure to examine the impact on the CI. I took a 
look, and this change should not have considerable adverse impact on Oracle CI. 

Aleksey, the change looks good to me.

-

PR: https://git.openjdk.java.net/jdk/pull/6619

Re: RFR: 8277992: Add fast jdk_svc subtests to jdk:tier3

[Mikhailo Seledtsov]

On Tue, 30 Nov 2021 18:48:15 GMT, Aleksey Shipilev  wrote:

> OpenJDK tiered tests definitions have the catch-all `tier4` that runs all 
> tests not defined in the lower tiers. `hotspot:tier4` has lots of them, 
> mostly long-running vmTestbase tests, which take many hours even on a very 
> parallel machines.
> 
> This, unfortunately, has a chilling effect on `jdk:tier4`, which is seldom 
> run by contributors, because `hotspot:tier4` is in the way. But, there are 
> plenty of fast and stable tests in `jdk:tier4` that can be run in 
> `jdk:tier3`. `jdk_svc` is the example of such tests: management features 
> (including but not limited to JFR) are important to keep from regressions, 
> and significant subset of them runs pretty fast.
> 
> So, it makes sense to move some `jdk_svc` tests to `jdk:tier3` to expose it 
> to more contributors. I think the only group we don't want to run is 
> `svc_tools`, which includes lots of non-parallel tests that are rather slow.
> 
> Sample run before:
> 
> 
> ==
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR  
>  
>jtreg:test/jdk:tier3174   174 0 0  
>  
> ==
> TEST SUCCESS
> 
> Finished building target 'run-test' in configuration 
> 'linux-x86_64-server-fastdebug'
> 
> real  2m38.242s
> user  80m7.216s
> sys   2m13.846s
> 
> 
> ==
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR  
>  
>>> jtreg:test/jdk:tier4   2904  2901 3 0 <<
> ==
> TEST FAILURE
> 
> real  18m13.933s
> user  546m50.556s
> sys   25m7.086s
> 
> 
> Sample run after:
> 
> 
> ==
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR  
>  
>jtreg:test/jdk:tier3   1296  1296 0 0  
>  
> ==
> TEST SUCCESS
> 
> Finished building target 'run-test' in configuration 
> 'linux-x86_64-server-fastdebug'
> 
> real  7m49.017s
> user  287m30.943s
> sys   13m20.060s
> 
> ==
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR  
>  
>>> jtreg:test/jdk:tier4   1783  1780 3 0 <<
> ==
> TEST FAILURE
> 
> 
> real  12m19.973s
> user  351m48.561s
> sys   14m51.566s

Marked as reviewed by mseledtsov (Committer).

-

PR: https://git.openjdk.java.net/jdk/pull/6619

Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage() [v3]

[Mikhailo Seledtsov]

On Tue, 17 Aug 2021 22:13:54 GMT, Mikhailo Seledtsov  
wrote:

>> Please review this change that updates the buildJdkDockerImage() test 
>> library API.
>> 
>> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l 
>> support for containers is not tested".
>> The initial intent was to extend the buildJdkDockerImage() API of 
>> DockerTestUtils to accept custom Dockerfile content.
>> As I analyzed the usage of buildJdkDockerImage() I realized that:
>>   - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest"
>>   its use has been obsolete for some time, in favor of Dockerfile 
>> generated by DockerTestUtils
>>   - 3rd argument "buildDirName" is also always the same: "jdk-docker"
>> 
>> Hence I thought it would be a good idea to simplify this API and make it 
>> up-to-date.
>> 
>> Also, since the method signature is being updated, I thought it would be a 
>> good idea to also change the name to use more generic container terminology:
>> buildJdkDockerImage() --> buildJdkContainerImage()
>
> Mikhailo Seledtsov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fixed a comment

Igor, Harold, thank you for review.

-

PR: https://git.openjdk.java.net/jdk/pull/5134

Integrated: 8272398: Update DockerTestUtils.buildJdkDockerImage()

[Mikhailo Seledtsov]

On Mon, 16 Aug 2021 23:31:41 GMT, Mikhailo Seledtsov  
wrote:

> Please review this change that updates the buildJdkDockerImage() test library 
> API.
> 
> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l 
> support for containers is not tested".
> The initial intent was to extend the buildJdkDockerImage() API of 
> DockerTestUtils to accept custom Dockerfile content.
> As I analyzed the usage of buildJdkDockerImage() I realized that:
>   - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest"
>   its use has been obsolete for some time, in favor of Dockerfile 
> generated by DockerTestUtils
>   - 3rd argument "buildDirName" is also always the same: "jdk-docker"
> 
> Hence I thought it would be a good idea to simplify this API and make it 
> up-to-date.
> 
> Also, since the method signature is being updated, I thought it would be a 
> good idea to also change the name to use more generic container terminology:
> buildJdkDockerImage() --> buildJdkContainerImage()

This pull request has now been integrated.

Changeset: ec63957f
Author:Mikhailo Seledtsov 
URL:   
https://git.openjdk.java.net/jdk/commit/ec63957f9d103e86d3b8e235e79cabb8992cb3ca
Stats: 68 lines in 17 files changed: 20 ins; 13 del; 35 mod

8272398: Update DockerTestUtils.buildJdkDockerImage()

Reviewed-by: iignatyev, hseigel

-

PR: https://git.openjdk.java.net/jdk/pull/5134

Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage() [v3]

[Mikhailo Seledtsov]

> Please review this change that updates the buildJdkDockerImage() test library 
> API.
> 
> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l 
> support for containers is not tested".
> The initial intent was to extend the buildJdkDockerImage() API of 
> DockerTestUtils to accept custom Dockerfile content.
> As I analyzed the usage of buildJdkDockerImage() I realized that:
>   - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest"
>   its use has been obsolete for some time, in favor of Dockerfile 
> generated by DockerTestUtils
>   - 3rd argument "buildDirName" is also always the same: "jdk-docker"
> 
> Hence I thought it would be a good idea to simplify this API and make it 
> up-to-date.
> 
> Also, since the method signature is being updated, I thought it would be a 
> good idea to also change the name to use more generic container terminology:
> buildJdkDockerImage() --> buildJdkContainerImage()

Mikhailo Seledtsov has updated the pull request incrementally with one 
additional commit since the last revision:

  Fixed a comment

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5134/files
  - new: https://git.openjdk.java.net/jdk/pull/5134/files/d211c220..5dc08781

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5134=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5134=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5134.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5134/head:pull/5134

PR: https://git.openjdk.java.net/jdk/pull/5134

Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage() [v2]

[Mikhailo Seledtsov]

On Tue, 17 Aug 2021 20:30:04 GMT, Harold Seigel  wrote:

>> Mikhailo Seledtsov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Addressing review feedback
>
> test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java line 175:
> 
>> 173: /**
>> 174:  * Build a container image based on given Dockerfile and image 
>> build directory.
>> 175:  *
> 
> This comment looks wrong because there is no longer a given Dockerfile.

Thanks. I will remove this comment.

-

PR: https://git.openjdk.java.net/jdk/pull/5134

Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage() [v2]

[Mikhailo Seledtsov]

> Please review this change that updates the buildJdkDockerImage() test library 
> API.
> 
> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l 
> support for containers is not tested".
> The initial intent was to extend the buildJdkDockerImage() API of 
> DockerTestUtils to accept custom Dockerfile content.
> As I analyzed the usage of buildJdkDockerImage() I realized that:
>   - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest"
>   its use has been obsolete for some time, in favor of Dockerfile 
> generated by DockerTestUtils
>   - 3rd argument "buildDirName" is also always the same: "jdk-docker"
> 
> Hence I thought it would be a good idea to simplify this API and make it 
> up-to-date.
> 
> Also, since the method signature is being updated, I thought it would be a 
> good idea to also change the name to use more generic container terminology:
> buildJdkDockerImage() --> buildJdkContainerImage()

Mikhailo Seledtsov has updated the pull request incrementally with one 
additional commit since the last revision:

  Addressing review feedback

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5134/files
  - new: https://git.openjdk.java.net/jdk/pull/5134/files/51a75339..d211c220

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5134=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5134=00-01

  Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5134.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5134/head:pull/5134

PR: https://git.openjdk.java.net/jdk/pull/5134

Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage()

[Mikhailo Seledtsov]

On Mon, 16 Aug 2021 23:48:46 GMT, Igor Ignatyev  wrote:

>> Please review this change that updates the buildJdkDockerImage() test 
>> library API.
>> 
>> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l 
>> support for containers is not tested".
>> The initial intent was to extend the buildJdkDockerImage() API of 
>> DockerTestUtils to accept custom Dockerfile content.
>> As I analyzed the usage of buildJdkDockerImage() I realized that:
>>   - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest"
>>   its use has been obsolete for some time, in favor of Dockerfile 
>> generated by DockerTestUtils
>>   - 3rd argument "buildDirName" is also always the same: "jdk-docker"
>> 
>> Hence I thought it would be a good idea to simplify this API and make it 
>> up-to-date.
>> 
>> Also, since the method signature is being updated, I thought it would be a 
>> good idea to also change the name to use more generic container terminology:
>> buildJdkDockerImage() --> buildJdkContainerImage()
>
> test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java line 179:
> 
>> 177:  * @throws Exception
>> 178:  */
>> 179: public static void buildImage(String imageName, Path buildDir) 
>> throws Exception {
> 
> it seems there are no users of this method, do we need to keep it public?

Sounds good. Will do.

-

PR: https://git.openjdk.java.net/jdk/pull/5134

Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when path contains colon

[Mikhailo Seledtsov]

On Mon, 16 Aug 2021 17:25:57 GMT, Harold Seigel  wrote:

> Please review this small fix for JDK-8272124.  The fix puts a limit of 3 when 
> splitting self cgroup lines by ':' so that Cgroup paths won't get truncated 
> if they contain embedded ':'s.  For example, an entry of 
> "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a 
> Cgroup path of "/user.sli:ce" instead of "/user.sli".
> 
> The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 
> and Linux aarch64.
> 
> Thanks, Harold

Changes look good to me.

-

Marked as reviewed by mseledtsov (Committer).

PR: https://git.openjdk.java.net/jdk/pull/5127

Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage()

[Mikhailo Seledtsov]

On Mon, 16 Aug 2021 23:31:41 GMT, Mikhailo Seledtsov  
wrote:

> Please review this change that updates the buildJdkDockerImage() test library 
> API.
> 
> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l 
> support for containers is not tested".
> The initial intent was to extend the buildJdkDockerImage() API of 
> DockerTestUtils to accept custom Dockerfile content.
> As I analyzed the usage of buildJdkDockerImage() I realized that:
>   - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest"
>   its use has been obsolete for some time, in favor of Dockerfile 
> generated by DockerTestUtils
>   - 3rd argument "buildDirName" is also always the same: "jdk-docker"
> 
> Hence I thought it would be a good idea to simplify this API and make it 
> up-to-date.
> 
> Also, since the method signature is being updated, I thought it would be a 
> good idea to also change the name to use more generic container terminology:
> buildJdkDockerImage() --> buildJdkContainerImage()

@hseigel Could you please review when you have a chance ?

Testing:
  - ran jdk/containers and hotspot/containers tests on
 - OL 7.9 with Docker
 - OL 8.3 with Podman
 - OL 8.3 aarch64 with Podman
All PASS

-

PR: https://git.openjdk.java.net/jdk/pull/5134

RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage()

[Mikhailo Seledtsov]

Please review this change that updates the buildJdkDockerImage() test library 
API.

This work originated while working on "8195809: [TESTBUG] jps and jcmd -l 
support for containers is not tested".
The initial intent was to extend the buildJdkDockerImage() API of 
DockerTestUtils to accept custom Dockerfile content.
As I analyzed the usage of buildJdkDockerImage() I realized that:
  - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest"
  its use has been obsolete for some time, in favor of Dockerfile generated 
by DockerTestUtils
  - 3rd argument "buildDirName" is also always the same: "jdk-docker"

Hence I thought it would be a good idea to simplify this API and make it 
up-to-date.

Also, since the method signature is being updated, I thought it would be a good 
idea to also change the name to use more generic container terminology:
buildJdkDockerImage() --> buildJdkContainerImage()

-

Commit messages:
 - Updated buildJdkDockerImage, changed signature and renamed to 
buildJdkContainerImage
 - Proposed docker build API updates

Changes: https://git.openjdk.java.net/jdk/pull/5134/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5134=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8272398
  Stats: 68 lines in 17 files changed: 20 ins; 15 del; 33 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5134.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5134/head:pull/5134

PR: https://git.openjdk.java.net/jdk/pull/5134

Re: [jdk17] RFR: 8269840: Update Platform.isDefaultCDSArchiveSupported() to return true for aarch64 platforms

[Mikhailo Seledtsov]

On Thu, 8 Jul 2021 01:59:25 GMT, Mikhailo Seledtsov  
wrote:

> Now that "JDK-8268212 Build linux-aarch64 natively" added support for default 
> CDS archive, time to update test configuration for this platform. This is a 
> very small one-line change.

Yumin, Ioi, thanks for the review. 

All relevant tests passed.

-

PR: https://git.openjdk.java.net/jdk17/pull/229

[jdk17] Integrated: 8269840: Update Platform.isDefaultCDSArchiveSupported() to return true for aarch64 platforms

[Mikhailo Seledtsov]

On Thu, 8 Jul 2021 01:59:25 GMT, Mikhailo Seledtsov  
wrote:

> Now that "JDK-8268212 Build linux-aarch64 natively" added support for default 
> CDS archive, time to update test configuration for this platform. This is a 
> very small one-line change.

This pull request has now been integrated.

Changeset: 46c610cb
Author:    Mikhailo Seledtsov 
URL:   
https://git.openjdk.java.net/jdk17/commit/46c610cbd84fc19c3f6591c9a6672768fb90c481
Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod

8269840: Update Platform.isDefaultCDSArchiveSupported() to return true for 
aarch64 platforms

Reviewed-by: minqi, iklam

-

PR: https://git.openjdk.java.net/jdk17/pull/229

Re: [jdk17] RFR: 8269840: Update Platform.isDefaultCDSArchiveSupported() to return true for aarch64 platforms

[Mikhailo Seledtsov]

On Thu, 8 Jul 2021 01:59:25 GMT, Mikhailo Seledtsov  
wrote:

> Now that "JDK-8268212 Build linux-aarch64 natively" added support for default 
> CDS archive, time to update test configuration for this platform. This is a 
> very small one-line change.

Thanks Ioi. I have updated the issue description and PR description to "Update 
Platform.isDefaultCDSArchiveSupported() to return true for aarch64 platforms". 
I can test on Linux-aarch64 and MacOsx-aarch64. I presume Windows-aarch64 is 
built natively by the OpenJDK community members (if it is built and tested), 
then this should be fine.

-

PR: https://git.openjdk.java.net/jdk17/pull/229

Re: [jdk17] RFR: 8269840: Update Platform.isDefaultCDSArchiveSupported() to return true for Linux-aarch64

[Mikhailo Seledtsov]

On Thu, 8 Jul 2021 01:59:25 GMT, Mikhailo Seledtsov  
wrote:

> Now that "JDK-8268212 Build linux-aarch64 natively" added support for default 
> CDS archive, time to update test configuration for this platform. This is a 
> very small one-line change.

Testing:
Grepped test sources for isDefaultCDSArchiveSupported. Found the following 
tests references:
test/hotspot/jtreg/runtime/cds/CheckDefaultArchiveFile.java,test/hotspot/jtreg/runtime/cds/CheckSharingWithDefaultArchive.java,test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java

Running these tests on Linux-aarch64.

@ioilam @calvinccheung @lmesnik Could you please review this one-liner when you 
have a chance?

-

PR: https://git.openjdk.java.net/jdk17/pull/229

[jdk17] RFR: 8269840: Update Platform.isDefaultCDSArchiveSupported() to return true for Linux-aarch64

[Mikhailo Seledtsov]

Now that "JDK-8268212 Build linux-aarch64 natively" added support for default 
CDS archive, time to update test configuration for this platform. This is a 
very small one-line change.

-

Commit messages:
 - 8269840: Update Platform.isDefaultCDSArchiveSupported() to return true for 
Linux-aarch64

Changes: https://git.openjdk.java.net/jdk17/pull/229/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=229=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8269840
  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/229.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/229/head:pull/229

PR: https://git.openjdk.java.net/jdk17/pull/229

Re: RFR: 8263412: ClassFileInstaller can't be used by classes outside of default package

[Mikhailo Seledtsov]

On Thu, 11 Mar 2021 05:47:00 GMT, Igor Ignatyev  wrote:

> Hi all,
> 
> could you please review the patch which moves `ClassFileInstaller` class to 
> `jdk.test.lib.helpers` package? 
> to reduce changes in the tests, `ClassFileInstaller` in the default package 
> is kept w/ just `main` method that calls  `jdk.test.lib.helpers. 
> ClassFileInstaller::main`. 
> 
> from JBS:
>> ClassFileInstaller is in the default package hence it's impossible to use it 
>> directly by classes in any other package. although ClassFileInstaller is 
>> mainly used directly from jtreg test description, there are cases when one 
>> needs to use it in another "driver" class (e.g. to reduce boilerplate), yet 
>> to do. that these driver classes have to either be in a default package or 
>> use reflection, both approaches have drawbacks. 
>> 
>> to solve that, we can move ClassFileInstaller implementation to a package, 
>> and to avoid unneeded churn, keep package-less ClassFileInstaller class 
>> which will call package-full impl.
>>
>> the need for this patch has raised as part of 
>> [JDK-8254129](https://bugs.openjdk.java.net/browse/JDK-8254129).
> 
> testing:
> - [x] `:jdk_core` against `{macos,windows,linux}-x64`
> - [x] `:jdk_svc` against `{macos,windows,linux}-x64`
> - [x] `test/hotspot/jtreg` w/o `applications` and `vmTestbase` directories 
> against `{macos,windows,linux}-x64`
> 
> Thanks,
> -- Igor

Marked as reviewed by mseledtsov (Committer).

-

PR: https://git.openjdk.java.net/jdk/pull/2932

Re: RFR 15 8242462: Residual Cleanup of rmic removal

[mikhailo . seledtsov]

Runtime test changes look good to me,

Misha

On 4/10/20 11:44 AM, Igor Ignatyev wrote:

Hi Roger,

removal of applications/ctw/modules/jdk_rmic.java and changes in doc (assuming 
.html was generated from .md) look good to me.

adding hotspot-runtime alias to bring attention of runtime team. I'm not sure 
who are the right people to review bin/unshuffle_list.txt though,.. build team 
(cc'ed them)?

Cheers,
-- Igor


On Apr 9, 2020, at 6:48 AM, Roger Riggs  wrote:

Please review a few cleanups I missed on the removal of rmic.
I didn't get a reply on the changes to the hotspot (cds) tests.
(I did get an ok on the javadoc changes)

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-remove-rmic-8225319-misc-1/

Issue:
   https://bugs.openjdk.java.net/browse/JDK-8242462

Thanks, Roger

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[mikhailo . seledtsov]

Sounds good,

Thank you,

Misha

On 7/17/19 6:45 PM, Igor Ignatev wrote:
We definitely should do it as a separate RFE, I meant to write it in 
my email, but was interrupted by a fire drill, and forgot about it 
when returned.


— Igor

On Jul 17, 2019, at 6:38 PM, mikhailo.seledt...@oracle.com 
 wrote:


However, I would recommend to do this work as part of a new RFE. If 
we agree, I will create an RFE, and we can continue discussion in the 
context of that RFE.

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[mikhailo . seledtsov]

On 7/17/19 11:37 AM, Igor Ignatyev wrote:


Hi Severin,

the updated webrev looks good to me, please see a couple comments below.

Cheers,
-- Igor

On Jul 17, 2019, at 10:34 AM, Severin Gehwolf > wrote:


Hi Misha,

On Wed, 2019-07-17 at 10:22 -0700,mikhailo.seledt...@oracle.com 
wrote:

Hi Severin,

On 7/17/19 5:44 AM, Severin Gehwolf wrote:

Hi Igor, Misha,

On Tue, 2019-07-16 at 11:49 -0700, Igor Ignatyev wrote:

Hi Severin,

I don't think that tests (or test libraries for that matter) should
be responsible for setting correct PATH value, it should be a part of
host configuration procedure (tests can/should check that all
required bins are available though). in other words, I'd prefer if
you remove 'env.put("PATH", ...)' lines from both DockerTestUtils and
TestJFREvents. the rest looks good to me.

Updated webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/02/webrev/

No more additions to PATH are being done.

I've discovered that VMProps.java which defines "docker.required", used
the "docker" binary even for podman test runs. This ended up not
running most of the tests even with -Djdk.test.docker.command=podman
specified.

Good catch.
should we rename docker.support and DOCKER_COMMAND to something more 
abstract?


Now that more container technologies are coming online we could consider 
more generic names for these properties/variables.


Here are some thoughts:

  - container.support (CONTAINER_COMMAND) - may be too generic

  - linux.container.support (LINUX_CONTAINER_COMMAND) - more narrow

  - even more narrow/specific: oci.container.support 
(OCI_CONTAINER_COMMAND)


 OCI in this case is " Open Container Initiative", ( Linux 
Foundation project to design open standards for Linux Container technology)


 I believe both Docker and Podman are OCI compliant.

However, I would recommend to do this work as part of a new RFE. If we 
agree, I will create an RFE, and we can continue discussion in the 
context of that RFE.



Thank you,

Misha




I've fixed that by moving DOCKER_COMMAND to Platform.java so
that it can be used in both places.


Sounds good to me.

(of course, the alternative would be to import
jdk.test.lib.containers.docker.DockerTestUtils into VMProps.java -- not
sure if there are any potential problems doing it this way)


I've tried that but for some reason this was a problem and VMProps
failed to compile. I don't know exactly how those jtreg extensions work
and went with the Platform approach. Hope that's OK.


all files needed for VMProps (or other @requires expression class) 
have to be listed in requires.extraPropDefns or 
requires.extraPropDefns.bootlibs property in TEST.ROOT file in all the 
test suites which use these extensions. we are trying to be very 
cautious in what is used by VMProps (directly and indirectly) so these 
lists won't grow and we won't require any modules other than 
java.base, given DockerTestUtils has dependencies on a number of other 
library classes, the Platform approach is much better from that point 
of view.





Testing: Container tests with docker daemon running on Linux x86_64,
container tests without docker daemon running (podman is daemon-less)
via the podman binary on Linux x86_64 (with -e:PATH). All pass.


Sounds good.


Overall looks good to me.


Thanks for the review!


One minor nit: DockerTestUtils.java does not need "import
java.util.Map;" (no need to post updated webrev for this change)


OK, good catch. Fixed locally.

Thanks,
Severin



Thank you,

Misha


More thoughts?

Thanks,
Severin


Thanks,
-- Igor

On Jul 16, 2019, at 5:36 AM, Severin Gehwolf > wrote:


Hi,

I believe I still need a *R*eviewer for this. Any takers?

Thanks,
Severin

On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com 
 wrote:

Hi Severin,

  The change looks good to me. Thank you for adding support for 
Podman

container technology.

Testing: I ran both HotSpot and JDK container tests with your patch;
tests executed on Oracle Linux 7.6 using default container 
engine (Docker):


test/hotspot/jtreg/containers/   AND
test/jdk/jdk/internal/platform/docker/

All PASS


Thanks,

Misha


On 7/12/19 11:08 AM, Severin Gehwolf wrote:

Hi,

There is an alternative container engine which is being used by 
Fedora
and RHEL 8, called podman[1]. It's mostly compatible with 
docker. It
looks like OpenJDK docker tests can be made podman compatible 
with a
few little tweaks. One "interesting" one is to not assert 
"Successfully
built" in the build output but only rely on the exit code, 
which seems

to be OK for my testing. Interestingly the test would be skipped in
that case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
webrev: 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/


Adjustments I've done:
 * Don't assert "Successfully built" in image build output[2].
 * Add 

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[mikhailo . seledtsov]

On 7/17/19 10:34 AM, Severin Gehwolf wrote:

Hi Misha,

On Wed, 2019-07-17 at 10:22 -0700, mikhailo.seledt...@oracle.com wrote:

Hi Severin,

On 7/17/19 5:44 AM, Severin Gehwolf wrote:

Hi Igor, Misha,

On Tue, 2019-07-16 at 11:49 -0700, Igor Ignatyev wrote:

Hi Severin,

I don't think that tests (or test libraries for that matter) should
be responsible for setting correct PATH value, it should be a part of
host configuration procedure (tests can/should check that all
required bins are available though). in other words, I'd prefer if
you remove 'env.put("PATH", ...)' lines from both DockerTestUtils and
TestJFREvents. the rest looks good to me.

Updated webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/02/webrev/

No more additions to PATH are being done.

I've discovered that VMProps.java which defines "docker.required", used
the "docker" binary even for podman test runs. This ended up not
running most of the tests even with -Djdk.test.docker.command=podman
specified.

Good catch.

I've fixed that by moving DOCKER_COMMAND to Platform.java so
that it can be used in both places.

Sounds good to me.

(of course, the alternative would be to import
jdk.test.lib.containers.docker.DockerTestUtils into VMProps.java -- not
sure if there are any potential problems doing it this way)

I've tried that but for some reason this was a problem and VMProps
failed to compile. I don't know exactly how those jtreg extensions work
and went with the Platform approach. Hope that's OK.


Thank you for the details. That's OK by me.


Thank you,

Misha




Testing: Container tests with docker daemon running on Linux x86_64,
container tests without docker daemon running (podman is daemon-less)
via the podman binary on Linux x86_64 (with -e:PATH). All pass.

Sounds good.


Overall looks good to me.

Thanks for the review!


One minor nit: DockerTestUtils.java does not need "import
java.util.Map;" (no need to post updated webrev for this change)

OK, good catch. Fixed locally.

Thanks,
Severin


Thank you,

Misha


More thoughts?

Thanks,
Severin


Thanks,
-- Igor


On Jul 16, 2019, at 5:36 AM, Severin Gehwolf  wrote:

Hi,

I believe I still need a *R*eviewer for this. Any takers?

Thanks,
Severin

On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com wrote:

Hi Severin,

The change looks good to me. Thank you for adding support for Podman
container technology.

Testing: I ran both HotSpot and JDK container tests with your patch;
tests executed on Oracle Linux 7.6 using default container engine (Docker):

  test/hotspot/jtreg/containers/   AND
test/jdk/jdk/internal/platform/docker/

All PASS


Thanks,

Misha


On 7/12/19 11:08 AM, Severin Gehwolf wrote:

Hi,

There is an alternative container engine which is being used by Fedora
and RHEL 8, called podman[1]. It's mostly compatible with docker. It
looks like OpenJDK docker tests can be made podman compatible with a
few little tweaks. One "interesting" one is to not assert "Successfully
built" in the build output but only rely on the exit code, which seems
to be OK for my testing. Interestingly the test would be skipped in
that case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/

Adjustments I've done:
   * Don't assert "Successfully built" in image build output[2].
   * Add /usr/sbin to PATH as the podman binary relies on iptables for it
 to work which is in /usr/sbin on Fedora
   * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
 to be equal to the previous value. I've found those counters to be
 slowly increasing, which made the tests unreliable.

Testing:

Running docker tests with docker as engine. Did the same with podman as
engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
passed (non-trivially).

Thoughts?

Thanks,
Severin

[1] https://podman.io/
[2] Image builds with podman look
  like ("COMMIT" over "Successfully built"):
STEP 1: FROM fedora:29
STEP 2: RUN dnf install -y java-11-openjdk-devel && dnf clean all
--> Using cache 96f8b1a0dfe7dba581a64fc67a27002ddf52e032af55f9ddc765182a690afd9d
STEP 3: COPY TestMetrics.class  TestMetrics.java /opt/
269042160f7a4e6a06789cd19640ea658a8f941bc53de0fd40a574dc3bdb49a8
STEP 4: CMD /usr/lib/jvm/java-11-openjdk/bin/java -cp /opt --add-modules 
java.base --add-exports java.base/jdk.internal.platform=ALL-UNNAMED TestMetrics
STEP 5: COMMIT fedora-metrics-11
d749088d6ce4510f212820ad4eca55a9b05e5c5c245f2372b6cfe91926e8cd7e

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[mikhailo . seledtsov]

Hi Severin,

On 7/17/19 5:44 AM, Severin Gehwolf wrote:

Hi Igor, Misha,

On Tue, 2019-07-16 at 11:49 -0700, Igor Ignatyev wrote:

Hi Severin,

I don't think that tests (or test libraries for that matter) should
be responsible for setting correct PATH value, it should be a part of
host configuration procedure (tests can/should check that all
required bins are available though). in other words, I'd prefer if
you remove 'env.put("PATH", ...)' lines from both DockerTestUtils and
TestJFREvents. the rest looks good to me.

Updated webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/02/webrev/

No more additions to PATH are being done.

I've discovered that VMProps.java which defines "docker.required", used
the "docker" binary even for podman test runs. This ended up not
running most of the tests even with -Djdk.test.docker.command=podman
specified.

Good catch.

I've fixed that by moving DOCKER_COMMAND to Platform.java so
that it can be used in both places.


Sounds good to me.

(of course, the alternative would be to import 
jdk.test.lib.containers.docker.DockerTestUtils into VMProps.java -- not 
sure if there are any potential problems doing it this way)



Testing: Container tests with docker daemon running on Linux x86_64,
container tests without docker daemon running (podman is daemon-less)
via the podman binary on Linux x86_64 (with -e:PATH). All pass.


Sounds good.


Overall looks good to me.

One minor nit: DockerTestUtils.java does not need "import 
java.util.Map;" (no need to post updated webrev for this change)



Thank you,

Misha



More thoughts?

Thanks,
Severin


Thanks,
-- Igor


On Jul 16, 2019, at 5:36 AM, Severin Gehwolf  wrote:

Hi,

I believe I still need a *R*eviewer for this. Any takers?

Thanks,
Severin

On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com wrote:

Hi Severin,

   The change looks good to me. Thank you for adding support for Podman
container technology.

Testing: I ran both HotSpot and JDK container tests with your patch;
tests executed on Oracle Linux 7.6 using default container engine (Docker):

 test/hotspot/jtreg/containers/   AND
test/jdk/jdk/internal/platform/docker/

All PASS


Thanks,

Misha


On 7/12/19 11:08 AM, Severin Gehwolf wrote:

Hi,

There is an alternative container engine which is being used by Fedora
and RHEL 8, called podman[1]. It's mostly compatible with docker. It
looks like OpenJDK docker tests can be made podman compatible with a
few little tweaks. One "interesting" one is to not assert "Successfully
built" in the build output but only rely on the exit code, which seems
to be OK for my testing. Interestingly the test would be skipped in
that case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/

Adjustments I've done:
  * Don't assert "Successfully built" in image build output[2].
  * Add /usr/sbin to PATH as the podman binary relies on iptables for it
to work which is in /usr/sbin on Fedora
  * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
to be equal to the previous value. I've found those counters to be
slowly increasing, which made the tests unreliable.

Testing:

Running docker tests with docker as engine. Did the same with podman as
engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
passed (non-trivially).

Thoughts?

Thanks,
Severin

[1] https://podman.io/
[2] Image builds with podman look
 like ("COMMIT" over "Successfully built"):
STEP 1: FROM fedora:29
STEP 2: RUN dnf install -y java-11-openjdk-devel && dnf clean all
--> Using cache 96f8b1a0dfe7dba581a64fc67a27002ddf52e032af55f9ddc765182a690afd9d
STEP 3: COPY TestMetrics.class  TestMetrics.java /opt/
269042160f7a4e6a06789cd19640ea658a8f941bc53de0fd40a574dc3bdb49a8
STEP 4: CMD /usr/lib/jvm/java-11-openjdk/bin/java -cp /opt --add-modules 
java.base --add-exports java.base/jdk.internal.platform=ALL-UNNAMED TestMetrics
STEP 5: COMMIT fedora-metrics-11
d749088d6ce4510f212820ad4eca55a9b05e5c5c245f2372b6cfe91926e8cd7e

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[mikhailo . seledtsov]

On 7/16/19 1:32 PM, Igor Ignatyev wrote:

Hi Misha,

I understand that it doesn't alter the host system. my concern is that we move problem of 
host-configuration into tests. let's say tomorrow a new container engine will require 
something from /usr/local/sbin, or /usr/local/Cellar/docker/bin on another OS, or, god 
forbid, C:\Program Files(x86)\podman\bin. it has nothing to do w/ tests, it's a question 
of configuring a host, as I said, should be handled at another level by 
"scripts" run (once) prior test execution.

OK, it makes sense now.

Thank you,

Misha



-- Igor


On Jul 16, 2019, at 1:23 PM, mikhailo.seledt...@oracle.com wrote:

Hi Igor,

In both cases the environment variable is set for the Docker/Podman 
container process, not the host system. This will not affect the host system in 
any way. The docker process has its own namespace for environment variables. 
Does this alleviate your concerns?


Thank you,

Misha

On 7/16/19 11:49 AM, Igor Ignatyev wrote:

Hi Severin,

I don't think that tests (or test libraries for that matter) should be responsible for 
setting correct PATH value, it should be a part of host configuration procedure (tests 
can/should check that all required bins are available though). in other words, I'd prefer 
if you remove 'env.put("PATH", ...)' lines from both DockerTestUtils and 
TestJFREvents. the rest looks good to me.

Thanks,
-- Igor


On Jul 16, 2019, at 5:36 AM, Severin Gehwolf  wrote:

Hi,

I believe I still need a *R*eviewer for this. Any takers?

Thanks,
Severin

On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com wrote:

Hi Severin,

   The change looks good to me. Thank you for adding support for Podman
container technology.

Testing: I ran both HotSpot and JDK container tests with your patch;
tests executed on Oracle Linux 7.6 using default container engine (Docker):

 test/hotspot/jtreg/containers/   AND
test/jdk/jdk/internal/platform/docker/

All PASS


Thanks,

Misha


On 7/12/19 11:08 AM, Severin Gehwolf wrote:

Hi,

There is an alternative container engine which is being used by Fedora
and RHEL 8, called podman[1]. It's mostly compatible with docker. It
looks like OpenJDK docker tests can be made podman compatible with a
few little tweaks. One "interesting" one is to not assert "Successfully
built" in the build output but only rely on the exit code, which seems
to be OK for my testing. Interestingly the test would be skipped in
that case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/

Adjustments I've done:
  * Don't assert "Successfully built" in image build output[2].
  * Add /usr/sbin to PATH as the podman binary relies on iptables for it
to work which is in /usr/sbin on Fedora
  * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
to be equal to the previous value. I've found those counters to be
slowly increasing, which made the tests unreliable.

Testing:

Running docker tests with docker as engine. Did the same with podman as
engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
passed (non-trivially).

Thoughts?

Thanks,
Severin

[1] https://podman.io/
[2] Image builds with podman look
 like ("COMMIT" over "Successfully built"):
STEP 1: FROM fedora:29
STEP 2: RUN dnf install -y java-11-openjdk-devel && dnf clean all
--> Using cache 96f8b1a0dfe7dba581a64fc67a27002ddf52e032af55f9ddc765182a690afd9d
STEP 3: COPY TestMetrics.class  TestMetrics.java /opt/
269042160f7a4e6a06789cd19640ea658a8f941bc53de0fd40a574dc3bdb49a8
STEP 4: CMD /usr/lib/jvm/java-11-openjdk/bin/java -cp /opt --add-modules 
java.base --add-exports java.base/jdk.internal.platform=ALL-UNNAMED TestMetrics
STEP 5: COMMIT fedora-metrics-11
d749088d6ce4510f212820ad4eca55a9b05e5c5c245f2372b6cfe91926e8cd7e

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[mikhailo . seledtsov]

Hi Igor,

   In both cases the environment variable is set for the Docker/Podman 
container process, not the host system. This will not affect the host 
system in any way. The docker process has its own namespace for 
environment variables. Does this alleviate your concerns?



Thank you,

Misha

On 7/16/19 11:49 AM, Igor Ignatyev wrote:

Hi Severin,

I don't think that tests (or test libraries for that matter) should be responsible for 
setting correct PATH value, it should be a part of host configuration procedure (tests 
can/should check that all required bins are available though). in other words, I'd prefer 
if you remove 'env.put("PATH", ...)' lines from both DockerTestUtils and 
TestJFREvents. the rest looks good to me.

Thanks,
-- Igor


On Jul 16, 2019, at 5:36 AM, Severin Gehwolf  wrote:

Hi,

I believe I still need a *R*eviewer for this. Any takers?

Thanks,
Severin

On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com wrote:

Hi Severin,

   The change looks good to me. Thank you for adding support for Podman
container technology.

Testing: I ran both HotSpot and JDK container tests with your patch;
tests executed on Oracle Linux 7.6 using default container engine (Docker):

 test/hotspot/jtreg/containers/   AND
test/jdk/jdk/internal/platform/docker/

All PASS


Thanks,

Misha


On 7/12/19 11:08 AM, Severin Gehwolf wrote:

Hi,

There is an alternative container engine which is being used by Fedora
and RHEL 8, called podman[1]. It's mostly compatible with docker. It
looks like OpenJDK docker tests can be made podman compatible with a
few little tweaks. One "interesting" one is to not assert "Successfully
built" in the build output but only rely on the exit code, which seems
to be OK for my testing. Interestingly the test would be skipped in
that case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/

Adjustments I've done:
  * Don't assert "Successfully built" in image build output[2].
  * Add /usr/sbin to PATH as the podman binary relies on iptables for it
to work which is in /usr/sbin on Fedora
  * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
to be equal to the previous value. I've found those counters to be
slowly increasing, which made the tests unreliable.

Testing:

Running docker tests with docker as engine. Did the same with podman as
engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
passed (non-trivially).

Thoughts?

Thanks,
Severin

[1] https://podman.io/
[2] Image builds with podman look
 like ("COMMIT" over "Successfully built"):
STEP 1: FROM fedora:29
STEP 2: RUN dnf install -y java-11-openjdk-devel && dnf clean all
--> Using cache 96f8b1a0dfe7dba581a64fc67a27002ddf52e032af55f9ddc765182a690afd9d
STEP 3: COPY TestMetrics.class  TestMetrics.java /opt/
269042160f7a4e6a06789cd19640ea658a8f941bc53de0fd40a574dc3bdb49a8
STEP 4: CMD /usr/lib/jvm/java-11-openjdk/bin/java -cp /opt --add-modules 
java.base --add-exports java.base/jdk.internal.platform=ALL-UNNAMED TestMetrics
STEP 5: COMMIT fedora-metrics-11
d749088d6ce4510f212820ad4eca55a9b05e5c5c245f2372b6cfe91926e8cd7e

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[mikhailo . seledtsov]

Hi Severin,

  The change looks good to me. Thank you for adding support for Podman 
container technology.


Testing: I ran both HotSpot and JDK container tests with your patch; 
tests executed on Oracle Linux 7.6 using default container engine (Docker):


    test/hotspot/jtreg/containers/   AND 
test/jdk/jdk/internal/platform/docker/


All PASS


Thanks,

Misha


On 7/12/19 11:08 AM, Severin Gehwolf wrote:

Hi,

There is an alternative container engine which is being used by Fedora
and RHEL 8, called podman[1]. It's mostly compatible with docker. It
looks like OpenJDK docker tests can be made podman compatible with a
few little tweaks. One "interesting" one is to not assert "Successfully
built" in the build output but only rely on the exit code, which seems
to be OK for my testing. Interestingly the test would be skipped in
that case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/

Adjustments I've done:
  * Don't assert "Successfully built" in image build output[2].
  * Add /usr/sbin to PATH as the podman binary relies on iptables for it
to work which is in /usr/sbin on Fedora
  * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
to be equal to the previous value. I've found those counters to be
slowly increasing, which made the tests unreliable.

Testing:

Running docker tests with docker as engine. Did the same with podman as
engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
passed (non-trivially).

Thoughts?

Thanks,
Severin

[1] https://podman.io/
[2] Image builds with podman look
 like ("COMMIT" over "Successfully built"):
STEP 1: FROM fedora:29
STEP 2: RUN dnf install -y java-11-openjdk-devel && dnf clean all
--> Using cache 96f8b1a0dfe7dba581a64fc67a27002ddf52e032af55f9ddc765182a690afd9d
STEP 3: COPY TestMetrics.class  TestMetrics.java /opt/
269042160f7a4e6a06789cd19640ea658a8f941bc53de0fd40a574dc3bdb49a8
STEP 4: CMD /usr/lib/jvm/java-11-openjdk/bin/java -cp /opt --add-modules 
java.base --add-exports java.base/jdk.internal.platform=ALL-UNNAMED TestMetrics
STEP 5: COMMIT fedora-metrics-11
d749088d6ce4510f212820ad4eca55a9b05e5c5c245f2372b6cfe91926e8cd7e

Re: RFR: 8224502 & 8224506 Container testbugs

[Mikhailo Seledtsov]

Changes look good to me,

Thank you for fixing,
Misha

On 6/20/19, 6:31 AM, Bob Vandette wrote:

Please review these two Docker container test bugs that I’d like to get fixed 
in JDK13.
Misha has already reviewed and approved these fixes.

I’ve tested these changes by running all Docker container tests locally with a 
fastdebug build
and also under mach5.

BUG:
https://bugs.openjdk.java.net/browse/JDK-8224502
[TESTBUG] JDK docker test TestSystemMetrics.java fails with access issues and 
OOM

WEBREV:
http://cr.openjdk.java.net/~bobv/8224502/webrev.01

This problem is caused by the test consuming too much memory.  The author
expected to allocate a 64MB array but instead allocated 512MB.  I also
added a container memory limit of 256MB in order to ensure that if the host 
running
the test does not have sufficient memory, we’ll get a better handle on the
cause of the failure.

--

BUG:
https://bugs.openjdk.java.net/browse/JDK-8224506
[TESTBUG] TestDockerMemoryMetrics.java fails with exitValue = 137

WEBREV:
http://cr.openjdk.java.net/~bobv/8224506/webrev.01

This problem occurs when the test is run with a fastdebug build.  The container
that is being created has a memory limit of 20MB which is too small.


NOTE: I will commit each fix individually with incremental changes to 
ProblemList.txt.


Bob.

Re: RFR: 8221340 - [TESTBUG] TestCgroupMetrics.java fails after fix for JDK-8219562

[mikhailo . seledtsov]

Changes look good to me,

Misha


On 5/7/19 5:56 AM, Bob Vandette wrote:

Please review this change to the Container Metrics implementation.

Change were made in JDK-8219562 [1] to osContainer_linux.cpp to correct
the parsing of of the /proc/self/mountinfo file but corresponding changes to the
Metrics API and Container tests shou,d have been done to match this change.

BUG:
https://bugs.openjdk.java.net/browse/JDK-8221340

WEBREV:
http://cr.openjdk.java.net/~bobv/8221340/webrev.00/

TESTING:
These changes were verified by running Mach5  tier1 and 2 tests and the jtreg 
container
tests on a Linux x64 system.

[1] https://bugs.openjdk.java.net/browse/JDK-8219562

Bob.

Re: RFR: 8222533 - jtreg test jdk/internal/platform/cgroup/TestCgroupMetrics.java fails on SLES12.3 linux ppc64le machine

[mikhailo . seledtsov]

Please make sure to update copyright year on the SubSystem.java before 
integration.


Thank you,

Misha


On 5/7/19 1:51 PM, mikhailo.seledt...@oracle.com wrote:

Looks good,

Misha


On 5/7/19 1:25 PM, Bob Vandette wrote:

Please review this simple fix for a TestCgroupMetrics.java test failure.

--- 
a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
+++ 
b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java

@@ -108,7 +108,7 @@
  try {
  List lines = 
Files.readAllLines(Paths.get(subsystem.path(), param));

  for (String line: lines) {
-    if (line.contains(match)) {
+    if (line.startsWith(match)) {
  retval = conversion.apply(line);
  break;
  }

Under docker we typically only see a single block I/O device so
the test passed since both lines containing “Total” are the same value.

8:16 Read 4452352
8:16 Write 0
8:16 Sync 0
8:16 Async 4452352
8:16 Total 4452352
Total 4452352

It’s possible and likely that there will be multiple devices causing 
failures

since the test and the Metrics API are not examining the same lines.

249:0 Read 10477568
249:0 Write 294431895552
249:0 Sync 17292476416
249:0 Async 277149896704
249:0 Total 294442373120 <——— The API was returning this
8:16 Read 19017216
8:16 Write 326780178432
8:16 Sync 17398915072
8:16 Async 309400280576
8:16 Total 326799195648
8:0 Read 27092992
8:0 Write 31070281728
8:0 Sync 10728873984
8:0 Async 20368500736
8:0 Total 31097374720
Total 652338943488 <—— Test test was comparing to this

We are after the line that “startsWith” “Total”.

Bob.

Re: RFR: 8222533 - jtreg test jdk/internal/platform/cgroup/TestCgroupMetrics.java fails on SLES12.3 linux ppc64le machine

[mikhailo . seledtsov]

Looks good,

Misha


On 5/7/19 1:25 PM, Bob Vandette wrote:

Please review this simple fix for a TestCgroupMetrics.java test failure.

--- a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
+++ b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
@@ -108,7 +108,7 @@
  try {
  List lines = 
Files.readAllLines(Paths.get(subsystem.path(), param));
  for (String line: lines) {
-if (line.contains(match)) {
+if (line.startsWith(match)) {
  retval = conversion.apply(line);
  break;
  }

Under docker we typically only see a single block I/O device so
the test passed since both lines containing “Total” are the same value.

8:16 Read 4452352
8:16 Write 0
8:16 Sync 0
8:16 Async 4452352
8:16 Total 4452352
Total 4452352

It’s possible and likely that there will be multiple devices causing failures
since the test and the Metrics API are not examining the same lines.

249:0 Read 10477568
249:0 Write 294431895552
249:0 Sync 17292476416
249:0 Async 277149896704
249:0 Total 294442373120 <——— The API was returning this
8:16 Read 19017216
8:16 Write 326780178432
8:16 Sync 17398915072
8:16 Async 309400280576
8:16 Total 326799195648
8:0 Read 27092992
8:0 Write 31070281728
8:0 Sync 10728873984
8:0 Async 20368500736
8:0 Total 31097374720
Total 652338943488 <—— Test test was comparing to this

We are after the line that “startsWith” “Total”.

Bob.

Re: RFR(S) : 8219139 : move hotspot tests from test/jdk/vm

[mikhailo . seledtsov]

Looks good to me,

Thank you,

Misha


On 3/14/19 2:38 PM, Igor Ignatyev wrote:

Hi Misha,

thanks for your suggestions, I have moved all runtime tests into 
subdirectories. here is the updated webrev: 
http://cr.openjdk.java.net/~iignatyev//8219139/webrev.01/index.html 



Thanks,
-- Igor

On Mar 4, 2019, at 1:57 PM, mikhailo.seledt...@oracle.com 
 wrote:


Hi Igor,

  Sorry it took a while to get back to you on this one. See my 
comment below



On 2/22/19 10:35 AM,mikhailo.seledt...@oracle.com 
wrote:
Overall the change looks good; thank you Igor for doing this. I have 
couple of comments:


- I am in favor of the approach where we move tests first under 
corresponding sub-component directories in
test/hotspot sub-tree, and then figure out whether to keep them. 
Even though in general I am in favor
of removing tests that are obsolete or of questionable value, this 
requires time, consideration and discussions.
Hence, I recommend filing an RFE for that, which can be addressed 
after the migration.


- Runtime normally avoids tests directly in test/hotspot/jtreg/runtime
The tests should go into underlying sub-directories which best match 
functional area or topic of that test.
In most cases you did it in your change, but there are several tests 
that your change places directly under

test/hotspot/jtreg/runtime/:

ExplicitArithmeticCheck.java
MonitorCacheMaybeExpand_DeadLock.java
ReflectStackOverflow.java
ShiftTest.java - David commented this can go under compiler (a jit test)
WideStrictInline.java
I have looked at the tests in more detail, and here are my 
recommendation of placements:

    ExplicitArithmeticCheck
    This test checks that ArithmeticException is thrown when 
appropriate

    I would recommend placing it under runtime/ErrorHandling
MonitorCacheMaybeExpand_DeadLock
    Existing folder: runtime/Thread (it does have a locking test)
    Or, alternatively, create a new folder: 'locking' or 'monitors'
    ReflectStackOverflow
    Uses recursive reflection attempting to provoke stack overflow
    Can go under: runtime/reflect
    WideStrictInline:
    checks for correct FP inlining by the interpreter
    I could not find existing sections; perhaps create 'interpreter'
    folder under 'runtime'

Thank you,
Misha


Since we plan (as discussed) to follow up this work with an RFE to 
review and consider removal of old and
not-that-useful tests, you could place them under 'misc' for now. 
Alternatively, find the best match

or create new sub-directories under runtime/ if necessary.


Thank you,
Misha


On 2/21/19 11:53 AM, Igor Ignatyev wrote:


On Feb 21, 2019, at 12:11 AM, David Holmes 
mailto:david.hol...@oracle.com>> wrote:


Hi Igor,

On 21/02/2019 3:19 pm, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8219139/webrev.00/index.html 


40 lines changed: 17 ins; 2 del; 21 mod;

Hi all,
could you please review this small patch which moves tests from 
test/jdk/vm?
I find some of these tests - the runtime ones at least - of 
extremely dubious value. They either cover basic functionality 
that is already well covered, or are regression tests for bugs in 
code that hasn't existed for many many years!
as I wrote in another related email: "one of the reason I'm 
proposing this move is exactly questionable value of these tests, I 
want to believe that having these tests in hotspot/ test 
directories will bring more attention to them from corresponding 
component teams and hence they will be removed/reworked/re-whatever 
faster and better. and I also believe that one of the reason we got 
duplications exactly because these tests were located in jdk test 
suite."



BTW:

test/hotspot/jtreg/runtime/ShiftTest.java

is actually a jit test according to the test comment.

sure, I will move it to hotspot/compiler.
there are 16 tests in test/jdk/vm directory. all but 
JniInvocationTest are hotspot tests, so they are moved to 
test/hotspot test suite; JniInvocationTest is a launcher test
No its a JNI invocation API test - nothing to do with our 
launcher. It belongs in runtime/jni. But we already have tests in 
runtime that use the JNI invocation API so this test adds no new 
coverage.
this is actually was my first reaction, and I even have the webrev 
which moves it to runtime/jni, but then I looked at the associated 
bug and it is filed against tools/launcher. and I even got a false 
(as I know by now) memory that I saw JLI_ method being called from 
the test. there is actually another test 
(dk/tools/launcher/exeJliLaunchTest.c) associated w/ this bug which 
calls JLI_Launch. anyhow, I'll move this test to hotspot/runtime/jni.


I really think the value of these tests needs to be examined 
before they are brought over.
I'd prefer to have 

Re: RFR(S) : 8219139 : move hotspot tests from test/jdk/vm

[mikhailo . seledtsov]

Hi Igor,

  Sorry it took a while to get back to you on this one. See my comment 
below



On 2/22/19 10:35 AM, mikhailo.seledt...@oracle.com wrote:
Overall the change looks good; thank you Igor for doing this. I have 
couple of comments:


  - I am in favor of the approach where we move tests first under 
corresponding sub-component directories in
    test/hotspot sub-tree, and then figure out whether to keep them. 
Even though in general I am in favor
    of removing tests that are obsolete or of questionable value, this 
requires time, consideration and discussions.
    Hence, I recommend filing an RFE for that, which can be addressed 
after the migration.


  - Runtime normally avoids tests directly in test/hotspot/jtreg/runtime
    The tests should go into underlying sub-directories which best 
match functional area or topic of that test.
    In most cases you did it in your change, but there are several 
tests that your change places directly under

 test/hotspot/jtreg/runtime/:

 ExplicitArithmeticCheck.java
 MonitorCacheMaybeExpand_DeadLock.java
 ReflectStackOverflow.java
 ShiftTest.java - David commented this can go under compiler (a 
jit test)

 WideStrictInline.java
I have looked at the tests in more detail, and here are my 
recommendation of placements:

    ExplicitArithmeticCheck
    This test checks that ArithmeticException is thrown when 
appropriate

    I would recommend placing it under runtime/ErrorHandling
    MonitorCacheMaybeExpand_DeadLock
    Existing folder: runtime/Thread (it does have a locking test)
    Or, alternatively, create a new folder: 'locking' or 'monitors'
    ReflectStackOverflow
    Uses recursive reflection attempting to provoke stack overflow
    Can go under: runtime/reflect
    WideStrictInline:
    checks for correct FP inlining by the interpreter
    I could not find existing sections; perhaps create 'interpreter'
    folder under 'runtime'

Thank you,
Misha


 Since we plan (as discussed) to follow up this work with an RFE 
to review and consider removal of old and
 not-that-useful tests, you could place them under 'misc' for now. 
Alternatively, find the best match

 or create new sub-directories under runtime/ if necessary.


Thank you,
Misha


On 2/21/19 11:53 AM, Igor Ignatyev wrote:


On Feb 21, 2019, at 12:11 AM, David Holmes  
wrote:


Hi Igor,

On 21/02/2019 3:19 pm, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8219139/webrev.00/index.html

40 lines changed: 17 ins; 2 del; 21 mod;

Hi all,
could you please review this small patch which moves tests from 
test/jdk/vm?
I find some of these tests - the runtime ones at least - of 
extremely dubious value. They either cover basic functionality that 
is already well covered, or are regression tests for bugs in code 
that hasn't existed for many many years!
as I wrote in another related email: "one of the reason I'm proposing 
this move is exactly questionable value of these tests, I want to 
believe that having these tests in hotspot/ test directories will 
bring more attention to them from corresponding component teams and 
hence they will be removed/reworked/re-whatever faster and better. 
and I also believe that one of the reason we got duplications exactly 
because these tests were located in jdk test suite."



BTW:

test/hotspot/jtreg/runtime/ShiftTest.java

is actually a jit test according to the test comment.

sure, I will move it to hotspot/compiler.
there are 16 tests in test/jdk/vm directory. all but 
JniInvocationTest are hotspot tests, so they are moved to 
test/hotspot test suite; JniInvocationTest is a launcher test
No its a JNI invocation API test - nothing to do with our launcher. 
It belongs in runtime/jni. But we already have tests in runtime that 
use the JNI invocation API so this test adds no new coverage.
this is actually was my first reaction, and I even have the webrev 
which moves it to runtime/jni, but then I looked at the associated 
bug and it is filed against tools/launcher. and I even got a false 
(as I know by now) memory that I saw JLI_ method being called from 
the test. there is actually another test 
(dk/tools/launcher/exeJliLaunchTest.c) associated w/ this bug which 
calls JLI_Launch. anyhow, I'll move this test to hotspot/runtime/jni.


I really think the value of these tests needs to be examined before 
they are brought over.
I'd prefer to have follow-up RFEs/tasks, b/c the longer we keep 
jdk/vm directory the more tests can end up there and the more rotten 
these tests become.


Thanks,
-- Igor

Thanks,
David
-

and hence it's moved to test/jdk/tools/launcher. as 
hotspot/compiler and hotspot/gc tests use packages, the tests moved 
there have been updated to have a package statement.
webrev: 
http://cr.openjdk.java.net/~iignatyev//8219139/webrev.00/index.html

JBS: https://bugs.openjdk.java.net/browse/JDK-8219139
testing: tier[1-2] (in progress), all the touched tests locally

Re: RFR(S) : 8219139 : move hotspot tests from test/jdk/vm

[mikhailo . seledtsov]

  Overall the change looks good; thank you Igor for doing this. I have 
couple of comments:


  - I am in favor of the approach where we move tests first under 
corresponding sub-component directories in
    test/hotspot sub-tree, and then figure out whether to keep them. 
Even though in general I am in favor
    of removing tests that are obsolete or of questionable value, this 
requires time, consideration and discussions.
    Hence, I recommend filing an RFE for that, which can be addressed 
after the migration.


  - Runtime normally avoids tests directly in test/hotspot/jtreg/runtime
    The tests should go into underlying sub-directories which best 
match functional area or topic of that test.
    In most cases you did it in your change, but there are several 
tests that your change places directly under

 test/hotspot/jtreg/runtime/:

 ExplicitArithmeticCheck.java
 MonitorCacheMaybeExpand_DeadLock.java
 ReflectStackOverflow.java
 ShiftTest.java - David commented this can go under compiler (a jit 
test)

 WideStrictInline.java

 Since we plan (as discussed) to follow up this work with an RFE to 
review and consider removal of old and
 not-that-useful tests, you could place them under 'misc' for now. 
Alternatively, find the best match

 or create new sub-directories under runtime/ if necessary.


Thank you,
Misha


On 2/21/19 11:53 AM, Igor Ignatyev wrote:



On Feb 21, 2019, at 12:11 AM, David Holmes  wrote:

Hi Igor,

On 21/02/2019 3:19 pm, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8219139/webrev.00/index.html

40 lines changed: 17 ins; 2 del; 21 mod;

Hi all,
could you please review this small patch which moves tests from test/jdk/vm?

I find some of these tests - the runtime ones at least - of extremely dubious 
value. They either cover basic functionality that is already well covered, or 
are regression tests for bugs in code that hasn't existed for many many years!

as I wrote in another related email: "one of the reason I'm proposing this move is 
exactly questionable value of these tests, I want to believe that having these tests in 
hotspot/ test directories will bring more attention to them from corresponding component 
teams and hence they will be removed/reworked/re-whatever faster and better. and I also 
believe that one of the reason we got duplications exactly because these tests were 
located in jdk test suite."


BTW:

test/hotspot/jtreg/runtime/ShiftTest.java

is actually a jit test according to the test comment.

sure, I will move it to hotspot/compiler.

there are 16 tests in test/jdk/vm directory. all but JniInvocationTest are 
hotspot tests, so they are moved to test/hotspot test suite; JniInvocationTest 
is a launcher test

No its a JNI invocation API test - nothing to do with our launcher. It belongs 
in runtime/jni. But we already have tests in runtime that use the JNI 
invocation API so this test adds no new coverage.

this is actually was my first reaction, and I even have the webrev which moves 
it to runtime/jni, but then I looked at the associated bug and it is filed 
against tools/launcher. and I even got a false (as I know by now) memory that I 
saw JLI_ method being called from the test. there is actually another test 
(dk/tools/launcher/exeJliLaunchTest.c) associated w/ this bug which calls 
JLI_Launch. anyhow, I'll move this test to hotspot/runtime/jni.



I really think the value of these tests needs to be examined before they are 
brought over.

I'd prefer to have follow-up RFEs/tasks, b/c the longer we keep jdk/vm 
directory the more tests can end up there and the more rotten these tests 
become.

Thanks,
-- Igor
  

Thanks,
David
-


and hence it's moved to test/jdk/tools/launcher. as hotspot/compiler and 
hotspot/gc tests use packages, the tests moved there have been updated to have 
a package statement.
webrev: http://cr.openjdk.java.net/~iignatyev//8219139/webrev.00/index.html
JBS: https://bugs.openjdk.java.net/browse/JDK-8219139
testing: tier[1-2] (in progress), all the touched tests locally
Thanks,
-- Igor

Re: Ping!! Re: RFR: 8203357 Container Metrics

[Mikhailo Seledtsov]

Hi Bob,

  I looked at the tests. In general they look good. I am a bit 
concerned about the use of ERROR_MARGIN in one of the tests. We need to 
make sure that the tests are stable, and do not produce intermittent 
failures.



Thank you,
Misha

On 6/7/18, 10:43 AM, Bob Vandette wrote:

Can I get one more reviewer for this RFE so I can integrate it?


http://cr.openjdk.java.net/~bobv/8203357/webrev.01

Mandy Chung has reviewed this change.

I’ve run Mach5 hotspot and core lib tests.

I’ve reviewed the tests which were written by Harsha Wardhana

I filed a CSR for the command line change and it’s now approved and closed.

Thanks,
Bob.



On May 30, 2018, at 3:45 PM, Bob Vandette  wrote:

Please review the following RFE which adds an internal API, along with jtreg 
tests that provide
access to Docker container configuration data and metrics.  In addition to the 
API which we hope to
take advantage of in the future with Java Flight Recorder and a JMX Mbean, I’ve 
added an additional
option to -XshowSettings:system than dumps out the container or host cgroup 
confguration
information.  See the sample output below:

RFE: Container Metrics

https://bugs.openjdk.java.net/browse/JDK-8203357

WEBREV:

http://cr.openjdk.java.net/~bobv/8203357/webrev.01


This commit will also include a fix for the following bug.

BUG: [TESTBUG] Test /runtime/containers/cgroup/PlainRead.java fails

https://bugs.openjdk.java.net/browse/JDK-8203691

WEBREV:

http://cr.openjdk.java.net/~bobv/8203357/webrev.00/test/hotspot/jtreg/runtime/containers/cgroup/PlainRead.java.sdiff.html

SAMPLE USAGE and OUTPUT:

docker run —memory=256m --cpuset-cpus 4-7 -it ubuntu bash
./java -XshowSettings:system
Operating System Metrics:
Provider: cgroupv1
Effective CPU Count: 4
CPU Period: 10
CPU Quota: -1
CPU Shares: -1
List of Processors, 4 total:
4 5 6 7
List of Effective Processors, 4 total:
4 5 6 7
List of Memory Nodes, 2 total:
0 1
List of Available Memory Nodes, 2 total:
0 1
CPUSet Memory Pressure Enabled: false
Memory Limit: 256.00M
Memory Soft Limit: Unlimited
Memory&  Swap Limit: 512.00M
Kernel Memory Limit: Unlimited
TCP Memory Limit: Unlimited
Out Of Memory Killer Enabled: true

TEST RESULTS:

testing runtime container APIs
Directory "JTwork" not found: creating
Passed: runtime/containers/cgroup/PlainRead.java
Passed: runtime/containers/docker/DockerBasicTest.java
Passed: runtime/containers/docker/TestCPUAwareness.java
Passed: runtime/containers/docker/TestCPUSets.java
Passed: runtime/containers/docker/TestMemoryAwareness.java
Passed: runtime/containers/docker/TestMisc.java
Test results: passed: 6
Results written to /export/users/bobv/jdk11/build/jtreg/JTwork

testing jdk.internal.platform APIs
Passed: jdk/internal/platform/cgroup/TestCgroupMetrics.java
Passed: jdk/internal/platform/docker/TestDockerCpuMetrics.java
Passed: jdk/internal/platform/docker/TestDockerMemoryMetrics.java
Passed: jdk/internal/platform/docker/TestSystemMetrics.java
Test results: passed: 4
Results written to /export/users/bobv/jdk11/build/jtreg/JTwork

testing -XshowSettings:system launcher option
Passed: tools/launcher/Settings.java
Test results: passed: 1


Bob.

Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled

[Mikhailo Seledtsov]

Changes look good to me.

Misha

On 6/2/2015 11:55 AM, Chris Plummer wrote:
I'm going to have to separate out the ProcessTool.java changes into a 
separate changeset (and CR). In the meantime, feel free to review what 
I have below. The code won't be changing at all when I separate out 
the ProcessTool.java changes.


thanks,

Chris

On 6/2/15 12:36 AM, Chris Plummer wrote:
[Adding core-libs-dev@openjdk.java.net since this update includes 
changes to jdk/test library code]


Please review the updated webrev:

Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054386

There were concerns about the new hotspot tests referencing jdk 
tests. One concern was that if the jdk tests change, they could break 
the hotspot tests, and this might initially go undetected. The other 
concern is that if the jdk and hotspot tests are placed in separate 
test bundles, then it would not be possible to run the hotspot tests.


Because of these concerns, I moved the tests that were in 
hotspot/test/runtime/CDSJDITests to instead be in 
jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the 
tests in the process. Also, I had to update the jdk version of 
ProcessTool.java to include the createJavaProcessBuilder() variant 
that is in the hotspot version of ProcessTool.java.


Lastly, in CDSJITTest.java I changed:

OutputAnalyzer output = new OutputAnalyzer(pb.start());

to instead be:

OutputAnalyzer output = ProcessTools.executeProcess(pb);

I had to do this since the jdk version of the OutputAnalyzer 
constructor is not public. The 1st version is what is commonly used 
in hostspot tests, and the 2nd version is what is commonly used in 
jdk tests. I decided to adopt the jdk way rather than make the 
OutputAnalyzer constructors public, although this will probably 
happen eventually when the two versions are unified.


thanks,

Chris


On 5/19/15 7:25 AM, Chris Plummer wrote:

Hi,

Please review the following changes for allowing java debugging when 
CDS is enabled.


Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/
Bug:https://bugs.openjdk.java.net/browse/JDK-8054386

The VM changes are simple. I removed the check that prevents 
debugging with CDS enabled, and added logic that will map the CDS 
archive RW when debugging is enabled.


The tests are a bit more complex. There are a bunch of existing JDI 
tests for testing debugging support. Rather than start from scratch 
or clone them, I instead just wrote wrapper tests that put the 
relevant JDI test classes in the archive, and then invoke the JDI 
test. I did this for 3 of the JDI tests. If you feel there are 
others that would be good candidates, I'd be happy to add them. I'm 
looking for ones that would result in modification of the RO class 
metadata, such as setting a breakpoint (for which I already added 
two tests).


Testing done:
-Using JPRT to run the new jtreg tests on all platforms.
-Using JPRT to run all jtreg runtime tests on linux x86 and x_64.
-Regular JPRT -testset hotspot run
-Putting the JCK JVMTI tests in the archive and then running them.
-Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and 
then running them.
-Putting a simple test class in the archive and then setting a 
breakpoint on it using jdb


Some of the above testing resulted in the discovery of bugs that 
still need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181.


I also verified that without the change to map the archive RW, the 
above testing resulted in a SEGV, which is what you would expect 
(and actually want to see to prove that the testing is effective).


thanks,

Chris