RFR : 8196062 : Enable docker container related tests for linux ppc64le
Baesken, Matthias
matthias.baesken at sap.com
Wed Feb 7 16:12:34 UTC 2018
Hi Mikhailo, sorry for causing the issue.
Looks like the change to test/lib/jdk/test/lib/Platform.java had unexpected consequences .
I created a new webrev without the Platform.java change :
http://cr.openjdk.java.net/~mbaesken/webrevs/8196062.3/
Best regards, Matthias
> -----Original Message-----
> From: Mikhailo Seledtsov [mailto:mikhailo.seledtsov at oracle.com]
> Sent: Mittwoch, 7. Februar 2018 06:09
> To: Baesken, Matthias <matthias.baesken at sap.com>
> Cc: hotspot-dev at openjdk.java.net
> Subject: Re: RFR : 8196062 : Enable docker container related tests for linux
> ppc64le
>
> Hi Matthias,
>
> Unfortunately one test failed during the pre-integration testing.
> The following test failed during the pre-integration testing:
> open/test/hotspot/jtreg/testlibrary_tests/TestMutuallyExclusivePlatformPr
> edicates.java
>
>
> Reproducible: 100%, Linux and MAC
>
> Failure: TEST RESULT: Failed. Execution failed: `main' threw exception:
> java.lang.RuntimeException: All Platform's methods with signature '():Z'
> should be tested. Missing: isPPC64le: expected true, was false
>
> See my comments in the RFE for details, and a suggested fix.
>
>
> Thank you,
> Misha
>
> On 2/6/18, 7:28 PM, Mikhailo Seledtsov wrote:
> > 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