RFR : 8196062 : Enable docker container related tests for linux ppc64le
Baesken, Matthias
matthias.baesken at sap.com
Tue Feb 6 07:50:28 UTC 2018
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