RFR : 8196062 : Enable docker container related tests for linux ppc64le
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Thu Feb 1 10:39:50 UTC 2018
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