RFR : 8196062 : Enable docker container related tests for linux ppc64le
Mikhailo Seledtsov
mikhailo.seledtsov at oracle.com
Wed Feb 7 03:28:48 UTC 2018
I am running pre-integration testing; will push unless the testing finds
any issues.
Regards,
Misha
On 2/5/18, 11:50 PM, Baesken, Matthias wrote:
> I only had to correct some whitespace changes found by hg jcheck , updated :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8196062.2/
>
> The fix has been reviewed by goetz, dsamersoff, bobv .
> Feedback was added, Summary updated too (suggested by Goetz).
>
> Tested with docker on SLES 12.1 / Ubuntu based container .
>
> Best regards, Matthias
>
>
>> -----Original Message-----
>> From: Mikhailo Seledtsov [mailto:mikhailo.seledtsov at oracle.com]
>> Sent: Samstag, 3. Februar 2018 00:01
>> To: Baesken, Matthias<matthias.baesken at sap.com>
>> Cc: Bob Vandette<bob.vandette at oracle.com>; Lindenmaier, Goetz
>> <goetz.lindenmaier at sap.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
>>
>> 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