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

Tue Oct 3 23:29:43 UTC 2017

Hi Misha,

1st of all, thank you for working on this very important piece. it generally looks great, although I have a number of mostly stylistic comments.

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>
>   29  * @requires (docker.support == "true")
1. although all properties in requires context are strings, jtreg does support boolean, so this line can be simplified to '@requires (docker.support'
>   31  * @modules java.base/jdk.internal.misc
>   32  * @modules java.management
>   33  *          jdk.jartool/sun.tools.jar
2. could you please use one style? either add @modules to all lines or leave it only at L#31 

>   35  * @run main DockerBasicTest
3. do we have to run DockerBasicTest in JDK under test? can't it be run as @run driver? 
>   46         if (!DockerTestUtils.canTestDocker())
>   47             return;
4. it's been proven to be less error prone to have { } even for one-line blocks, could you please add them here?

>   72             .addDockerOpts("--volume", System.getProperty("test.classes") + ":/test-classes/");
5. as you already depend on jdk.test.lib, it might be better to reuse Utils::TEST_CLASSES
>   74         DockerTestUtils.dockerRunJava(opts).
>   75             shouldHaveExitValue(0).shouldContain("Hello Docker");
6. lines shouldn't end w/ '.', it should be moved to next line and preferable be alined w/ 1st '.' in prev line. I have seen in other files as well.
I'd also like to see exact one method call per line. 

http://cr.openjdk.java.net/~mseledtsov/8181592.02/test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java.html <http://cr.openjdk.java.net/~mseledtsov/8181592.02/test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java.html>
>   47     // Diagnostics: set to true to enable more diagnostic info
>   48     private static final boolean DEBUG = false;
1. I'd prefer to always have all available diagnostic info. is there any problem w/ having DEBUG=true?
>  129         Files.createDirectories(jdkDstDir);
> ...
>  133         Files.walkFileTree(jdkSrcDir, new CopyFileVisitor(jdkSrcDir, jdkDstDir));
2. since jdkDstDir is supposed to be empty, you can use Files::copy method to copy the dir.

>  126         Path jdkSrcDir = Paths.get(System.getProperty("test.jdk"));
3. again, as you already depend on jdk.test.lib, it might be better to reuse Utils::TEST_JDK

>  149     public static void
>  150         buildDockerImage(String imageName, Path dockerfile, Path buildDir) throws Exception {

>  213     public static OutputAnalyzer execute(boolean retainChildStdout, String... command)
>  214         throws Exception {

>  230     public static OutputAnalyzer execute(boolean retainChildStdout, List<String> command)
>  231         throws Exception {
4. when you wrap a line, you should use double indentation, otherwise one can misread it, in last two examples I read throws Exception as 1st statement of a method.

5. your 'execute(boolean, String...)' creates a list from 'command' array and calls 'execute(boolean, List<String>)', which create an array from 'command' list and pass it to ProcessBuilder::new. should we make 'execute(boolean, List<String>)' call 'execute(boolean, String...)' and move all the logic to the latter?

also, there are easier ways to get a list from an arrays -- j.u.Arrays.asList(T...) or j.u.List.of(T...)

Thanks,
-- Igor

> 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
>>>> 
>>> 
>> 