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

[mikhailo]

Hi Leonid,

   Thank you for review. Please see my comments inline:


On 10/04/2017 11:19 AM, Leonid Mesnik wrote:
> 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/%7Emseledtsov/8181592.02/test/jtreg-ext/requires/VMProps.java.sdiff.html>
>
>  307             System.err.println("dockerSupprt() threw exception: " 
> + e);
> Missed ‘o’ in Support.
>
Fixed
> 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.
Fixed. I added 10 seconds for timeout, which should be sufficient.
IMO, if it takes more than 10 sec to invoke this basic command then 
something is wrong, and the docker tests should not be run.
>
> http://cr.openjdk.java.net/~mseledtsov/8181592.02/test/hotspot/jtreg/runtime/containers/docker/DockerBasicTest.java.html 
> <http://cr.openjdk.java.net/%7Emseledtsov/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?
Added.
>
> http://cr.openjdk.java.net/~mseledtsov/8181592.02/test/lib/jdk/test/lib/containers/docker/DockerRunOptions.java.html 
> <http://cr.openjdk.java.net/%7Emseledtsov/8181592.02/test/lib/jdk/test/lib/containers/docker/DockerRunOptions.java.html>
> 44     public DockerRunOptions(){}
> Why this empty constructor is needed?
Fixed - removed.
> Does it make also to make this class immutable i.e. init all 
> properties in constructors and have them read-only.
The idea of this class is that fields can be modified by the user. The 
default constructor fills out values for most common use case; when user 
decided to modify these options to specify different test run conditions 
they should be able to modify it.

I could surround values by getters or setters, or make every new set 
return a new copy of the object (an immutable pattern), but I did not 
wish to complicate the code. The uses of this class a fairly simple, and 
are unlikely to result in data races. The test create runOptions object, 
specifies/modifies fields if necessary, then passes to DockerTestUtils 
for running docker container with these options.

I hope you do not mind if I leave it as is.


Thank you for review,
Misha
>
> Leonid
>
>> On Sep 28, 2017, at 3:11 PM, Mikhailo Seledtsov 
>> <mikhailo.seledtsov at oracle.com 
>> <mailto:mikhailo.seledtsov at oracle.com>> wrote:
>>
>> Here is the updated webrev:
>> http://cr.openjdk.java.net/~mseledtsov/8181592.02/ 
>> <http://cr.openjdk.java.net/%7Emseledtsov/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> 
>>>> <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> 
>>>> <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> 
>>>> <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> 
>>>>> <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/> 
>>>>> <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
>>>>>
>>>>
>>>
>