RFR : 8196062 : Enable docker container related tests for linux ppc64le
Mikhailo Seledtsov
mikhailo.seledtsov at oracle.com
Fri Feb 2 23:00:46 UTC 2018
Hi Matthias,
I can sponsor your change if you'd like.
Once you addressed all the feedback from code review, please sync to the
tip, build and test.
Then export the changeset and send it to me (see:
http://openjdk.java.net/sponsor/)
I will import your change set, run all required testing and push the change.
Thank you,
Misha
On 2/2/18, 12:39 AM, Baesken, Matthias wrote:
> Thanks for the reviews .
>
> I added info about the fix for /proc/self/cgroup and /proc/self/mountinfo parsing to the bug :
>
> https://bugs.openjdk.java.net/browse/JDK-8196062
>
> Guess I need a sponsor now to get it pushed ?
>
>
> Best regards, Matthias
>
>
>
>> -----Original Message-----
>> From: Bob Vandette [mailto:bob.vandette at oracle.com]
>> Sent: Donnerstag, 1. Februar 2018 17:53
>> To: Lindenmaier, Goetz<goetz.lindenmaier at sap.com>
>> Cc: Baesken, Matthias<matthias.baesken at sap.com>; mikhailo
>> <mikhailo.seledtsov at oracle.com>; hotspot-dev at openjdk.java.net; 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
>>
>> 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