RFR : 8196062 : Enable docker container related tests for linux ppc64le
mikhailo
mikhailo.seledtsov at oracle.com
Wed Feb 7 18:24:08 UTC 2018
Thank you for the update. I will import your latest webrev, and start
the testing.
Regard,
Misha
On 02/07/2018 08:12 AM, Baesken, Matthias wrote:
> 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