RFR : 8196062 : Enable docker container related tests for linux ppc64le
Baesken, Matthias
matthias.baesken at sap.com
Fri Feb 2 08:39:36 UTC 2018
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