RFR : 8196062 : Enable docker container related tests for linux ppc64le

[mikhailo]

Hi Matthias,


On 01/25/2018 12:15 AM, Baesken, Matthias wrote:
>> Perhaps, you could add code to DockerTestUtils.buildJdkDockerImage()
>> that does the following or similar:
>>       1. Construct a name for platform-specific docker file:
>>              String platformSpecificDockerfile = dockerfile + "-" +
>> Platform.getOsArch();
>>              (Platform is jdk.test.lib.Platform)
>>
> Hello,  the doc  says :
>
>       * Build a docker image that contains JDK under test.
>       * The jdk will be placed under the "/jdk/" folder inside the docker file system.
>       .....
>       param dockerfile    name of the dockerfile residing in the test source
>       .....
>      public static void   buildJdkDockerImage(String imageName, String dockerfile, String buildDirName)
>
>
>
> It does not say anything about doing hidden insertions of  some platform names into  the dockerfile name.
> So should the jtreg API doc be changed ?
> If so who needs to approve this ?
Thank you for your concerns about the clarity of API and corresponding 
documentation. This is a test library API, so no need to file CCC or CSR.

This API can be changed via a regular RFR/webrev review process, as soon 
as on one objects. I am a VM SQE engineer covering the docker and Linux 
container area, I am OK with this change.
And I agree with you, we should update the javadoc header on this method 
to reflect this implicit part of API contract.


Thank you,
Misha



> (as far as I see so far only the  test  at hotspot/jtreg/runtime/containers/docker/   use this  so it should not be a big deal to change the interface?)
>
> Best regards, Matthias
>
>
>
>
>> -----Original Message-----
>> From: mikhailo [mailto:mikhailo.seledtsov at oracle.com]
>> Sent: Mittwoch, 24. Januar 2018 20:09
>> To: Bob Vandette <bob.vandette at oracle.com>; Baesken, Matthias
>> <matthias.baesken at sap.com>
>> Cc: hotspot-dev at openjdk.java.net; Lindenmaier, Goetz
>> <goetz.lindenmaier at sap.com>; Langer, Christoph
>> <christoph.langer at sap.com>; Doerr, Martin <martin.doerr at sap.com>
>> Subject: Re: RFR : 8196062 : Enable docker container related tests for linux
>> ppc64le
>>
>> Hi Matthias,
>>
>>      Please see my comments about the test changes inline.
>>
>>
>> On 01/24/2018 07:13 AM, Bob Vandette wrote:
>>> osContainer_linux.cpp:
>>>
>>> Can you add "return;" in each test for subsystem not found messages and
>>> remove these 3 lines OR move your tests for NULL & messages inside.  The
>> compiler can
>>> probably optimize this but I’d prefer more compact code.
>>>
>>> if (memory == NULL || cpuset == NULL || cpu == NULL || cpuacct == NULL)
>> {
>>>    342     return;
>>>    343   }
>>>
>>>
>>> The other changes in osContainer_linux.cpp look ok.
>>>
>>> I forwarded your test changes to Misha, who wrote these.
>>>
>>> Since it’s likely that other platforms, such as aarch64, are going to run into
>> the same problem,
>>> It would have been better to enable the tests based on the existence of an
>> arch specific
>>> Dockerfile-BasicTest-{os.arch} rather than enabling specific arch’s in
>> VPProps.java.
>>> This approach would reduce the number of changes significantly and allow
>> support to
>>> be added with 1 new file.
>>>
>>> You wouldn’t need "String dockerFileName =
>> Common.getDockerFileName();”
>>> in every test.  Just make DockerTestUtils automatically add arch.
>> I like Bob's idea on handling platform-specific Dockerfiles.
>>
>> Perhaps, you could add code to DockerTestUtils.buildJdkDockerImage()
>> that does the following or similar:
>>       1. Construct a name for platform-specific docker file:
>>              String platformSpecificDockerfile = dockerfile + "-" +
>> Platform.getOsArch();
>>              (Platform is jdk.test.lib.Platform)
>>
>>       2. Check if platformSpecificDockerfile file exists in the test
>> source directory
>>             File.exists(Paths.get(Utils.TEST_SRC, platformSpecificDockerFile)
>>             If it does, then use it. Otherwise continue using the
>> default/original dockerfile name.
>>
>> I think this will considerably simplify your change, as well as make it
>> easy to extend support to other platforms/configurations
>> in the future. Let us know what you think of this approach ?
>>
>>
>> Once your change gets (R)eviewed and approved, I can sponsor the push.
>>
>>
>> Thank you,
>> Misha
>>
>>
>>
>>> Bob.
>>>
>>>
>>>
>>>> On Jan 24, 2018, at 9:24 AM, Baesken, Matthias
>> <matthias.baesken at sap.com> wrote:
>>>> Hello,  could you please review the following change :  8196062 :  Enable
>> docker container related tests for linux ppc64le  .
>>>> It  adds  docker container testing   for linux ppc64 le  (little endian) .
>>>>
>>>> A number of things had to be done :
>>>> 	• Add a  separate  docker file
>> test/hotspot/jtreg/runtime/containers/docker/Dockerfile-BasicTest-ppc64le
>> for linux ppc64 le     which uses   Ubuntu ( the  Oracle Linux 7.2  used  for
>> x86_64  seems not to be available for ppc64le )
>>>> 	• Fix  parsing    /proc/self/mountinfo    and   /proc/self/cgroup  in
>> src/hotspot/os/linux/osContainer_linux.cpp    , it could  not  handle  the
>> format seen  on SUSE LINUX 12.1 ppc64le (Host)  and  Ubuntu (Docker
>> container)
>>>> 	• Add a bit  more logging
>>>>
>>>>
>>>> Webrev :
>>>>
>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8196062/
>>>>
>>>>
>>>> Bug :
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8196062
>>>>
>>>>
>>>> After these adjustments I could run the    runtime/containers/docker    -
>> jtreg tests successfully .
>>>>
>>>> Best regards, Matthias