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

mikhailo mikhailo.seledtsov at oracle.com
Thu Oct 5 01:08:46 UTC 2017 

Here is the updated webrev that addresses feedback from Igor and Leonid:

http://cr.openjdk.java.net/~mseledtsov/8181592.03/


Thank you,

Misha


On 10/04/2017 05:52 PM, mikhailo 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
>>>>>>
>>>>>
>>>>
>>
>

More information about the hotspot-runtime-dev mailing list