RFR(S): 8181592: [TESTBUG] Docker test utils and docker jdk basic test

[Leonid Mesnik]

Hi

Overall looks good. There few comments:
http://cr.openjdk.java.net/~mseledtsov/8181592.02/test/jtreg-ext/requires/VMProps.java.sdiff.html <http://cr.openjdk.java.net/~mseledtsov/8181592.02/test/jtreg-ext/requires/VMProps.java.sdiff.html>

 307             System.err.println("dockerSupprt() threw exception: " + e);
Missed ‘o’ in Support.

316         p.waitFor();
I think that it is needed to guard execution with timeout. I am not sure that jtreg is going to run properties with timeout.

http://cr.openjdk.java.net/~mseledtsov/8181592.02/test/hotspot/jtreg/runtime/containers/docker/DockerBasicTest.java.html <http://cr.openjdk.java.net/~mseledtsov/8181592.02/test/hotspot/jtreg/runtime/containers/docker/DockerBasicTest.java.html>
28  * @requires (sun.arch.data.model != "32") & (os.family == "linux")
Is it possible to check this inside docker.support and use only “docker.support” property?

http://cr.openjdk.java.net/~mseledtsov/8181592.02/test/lib/jdk/test/lib/containers/docker/DockerRunOptions.java.html <http://cr.openjdk.java.net/~mseledtsov/8181592.02/test/lib/jdk/test/lib/containers/docker/DockerRunOptions.java.html>
44     public DockerRunOptions(){}
Why this empty constructor is needed? 
Does it make also to make this class immutable i.e. init all properties in constructors and have them read-only.

Leonid

> On Sep 28, 2017, at 3:11 PM, Mikhailo Seledtsov <mikhailo.seledtsov at oracle.com> wrote:
> 
> Here is the updated webrev:
>    http://cr.openjdk.java.net/~mseledtsov/8181592.02/
> 
> 
> Leonid, George - thank you for your comments.
> In addition to addressing your feedback, I also did:
>  - implemented @requires docker.support
>  - added dockerRunJava() method and associated data structure DockerRunOptions, for running Java tests inside
>     docker environment, and to account for java opts, test java opts, docker opts, classes and class params
>  - added a simple HelloWorld test case that runs HelloWorld inside a container
>  - ran jtreg with extra Java options, make sure they are added correctly to the docker run command
>  - added docker image cleanup after testing is done
> 
> 
> Please review.
> 
> Misha
> 
> 
> On 9/27/17, 11:00 AM, mikhailo wrote:
>> Leonid,
>> 
>> Thank you for review and constructive feedback. See my comment in line.
>> 
>> 
>> On 09/26/2017 11:19 AM, Leonid Mesnik wrote:
>>> Misha
>>> 
>>> http://cr.openjdk.java.net/~mseledtsov/8181592.00/test/hotspot/jtreg/runtime/containers/docker/DockerBasicTest.java.html <http://cr.openjdk.java.net/%7Emseledtsov/8181592.00/test/hotspot/jtreg/runtime/containers/docker/DockerBasicTest.java.html> 
>>> Copyright is incorrect, need to updated it for GPL.
>> Fixed
>>> 
>>> The Hotspot is Oracle VM name only so test might fail for OpenJDK. I think you need to fix this check.
>> I see. I fixed this by using Platform.vmName which should be correct in all cases. I double-checked with OpenJDK also.
>>> 
>>> The requires checks only that test is executed only on the 64-bit linux. Does it make a sense to introduce more docker-specific check?
>> I agree this is a better way. I will do some prototyping; if such check is feasible and efficient in at requires then I will add it.
>>> 
>>> 
>>> http://cr.openjdk.java.net/~mseledtsov/8181592.00/test/hotspot/jtreg/runtime/containers/docker/Dockerfile-BasicTest.html <http://cr.openjdk.java.net/%7Emseledtsov/8181592.00/test/hotspot/jtreg/runtime/containers/docker/Dockerfile-BasicTest.html> 
>>> Could you please explain why oraclelinux 7.0 is used as a base image for test.
>> I have upgraded to Oracle Linux 7.2. If we have specific requirement I will change it to that. If we have requirements in the future to support multiple OS, I can add Dockerfile generation.
>> For this basic sanity tests I think this should suffice.
>>> 
>>> http://cr.openjdk.java.net/~mseledtsov/8181592.00/test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java.html <http://cr.openjdk.java.net/%7Emseledtsov/8181592.00/test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java.html> 
>>> The content looks fine.
>>> 
>>> I don’t see anything to clean up docker images on the system. Could you please explain how tests are going to cleanup images.
>> To clean up containers I will add "--rm" to the 'docker run' command. This should ensure that container data is removed after container stops.
>> As for the image - I use the same image name. The image will stay in the local registry unless manually removed. I should probably do 'docker rmi' at the end of the test to clean this up.
>> 
>> 
>> Once I implement these changes I will send the updated webrev.
>> 
>> Thank you,
>> Misha
>>> 
>>> Leonid
>>> 
>>> 
>>>> On Sep 21, 2017, at 5:58 PM, mikhailo <mikhailo.seledtsov at oracle.com <mailto:mikhailo.seledtsov at oracle.com>> wrote:
>>>> 
>>>> Please review this initial drop of Docker test utils and a sanity test. This change lays ground
>>>> for further test development and test utils improvement in this area.
>>>> 
>>>>    JBS: https://bugs.openjdk.java.net/browse/JDK-8181592
>>>>    Webrev: http://cr.openjdk.java.net/~mseledtsov/8181592.00/ <http://cr.openjdk.java.net/%7Emseledtsov/8181592.00/>
>>>>    Testing:
>>>>       - run this test on machine with Docker enabled - works
>>>>       - run this test on Linux-x64 with no Docker engine or Docker disabled - test skipped (as expected)
>>>>       - run this test on automated system - in progress
>>>> 
>>>> 
>>>> Thank you,
>>>> Misha
>>>> 
>>> 
>>