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

[mikhailo]

Thank you for review,


Misha


On 10/09/2017 10:41 AM, Leonid Mesnik wrote:
> Hi
>
> Thank you for fixes and explanations. Looks good for me now.
>
> Leonid
>> On Oct 4, 2017, at 5:52 PM, mikhailo <mikhailo.seledtsov at oracle.com 
>> <mailto:mikhailo.seledtsov at oracle.com>> wrote:
>>
>> 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
>>>>>>>
>>>>>>
>>>>>
>>>
>>
>