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

[mikhailo]

Thank you for review,

Misha


On 10/09/2017 12:57 PM, Igor Ignatyev wrote:
> Hi Misha,
>
> since 'docker.support' is true only if we are on linux-x64, you can remove L#28 (@requires (sun.arch.data.model != "32") & (os.family == "linux")) from DockerBasicTest. the rest looks good to me.
> no need to send a new webrev.
Fixed
>
> Thanks,
> -- Igor
>> On Oct 4, 2017, at 6:08 PM, mikhailo <mikhailo.seledtsov at oracle.com> wrote:
>>
>> 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
>>>>>>>>