RFR : 8196062 : Enable docker container related tests for linux ppc64le
Dmitry Samersov
dmitry.samersoff at bell-sw.com
Fri Feb 2 09:49:29 UTC 2018
Matthias,
The fix looks good to me.
PS: I would prefer to check that file path fits to MAXPATHLEN before
doing any copying and save a bit of computer power or, ever better,
use snprintf here but these changes are clearly out of scope of your fix.
-Dmitry
On 31.01.2018 17:15, Baesken, Matthias wrote:
> Hello , I created a second webrev :
>
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8196062.1/8196062.1/webrev/
>
> - 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