RFR : 8196062 : Enable docker container related tests for linux ppc64le
Bob Vandette
bob.vandette at oracle.com
Thu Feb 1 16:52:43 UTC 2018
Looks good to me.
Bob.
> On Feb 1, 2018, at 5:39 AM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com> wrote:
>
> Hi Matthias,
>
> thanks for enabling this test. Looks good.
> I would appreciate if you would add a line
> "Summary: also fix cgroup subsystem recognition"
> to the bug description. Else this might be mistaken
> for a mere testbug.
>
> Best regards,
> Goetz.
>
>
>> -----Original Message-----
>> From: Baesken, Matthias
>> Sent: Mittwoch, 31. Januar 2018 15:15
>> To: mikhailo <mikhailo.seledtsov at oracle.com>; Bob Vandette
>> <bob.vandette at oracle.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>;
>> Dmitry Samersoff <dmitry.samersoff at bell-sw.com>
>> Subject: RE: RFR : 8196062 : Enable docker container related tests for linux
>> ppc64le
>>
>> Hello , I created a second webrev :
>>
>>
>> http://cr.openjdk.java.net/~mbaesken/webrevs/8196062.1/8196062.1/webr
>> ev/
>>
>> - changed DockerTestUtils.buildJdkDockerImage in the suggested way (this
>> should be extendable to linux s390x soon)
>>
>>>>>> Can you add "return;" in each test for subsystem not found messages
>>
>> - added returns in the tests for the subsystems in osContainer_linux.cpp
>>
>> - moved some checks at the beginning of subsystem_file_contents
>> (suggested by Dmitry)
>>
>>
>> Best regards, Matthias
>>
>>
>>
>>> -----Original Message-----
>>> From: mikhailo [mailto:mikhailo.seledtsov at oracle.com]
>>> Sent: Donnerstag, 25. Januar 2018 18:43
>>> To: Baesken, Matthias <matthias.baesken at sap.com>; Bob Vandette
>>> <bob.vandette at oracle.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,
>>>
>>>
>>> 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
>
More information about the hotspot-dev
mailing list