RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration
Mikhailo Seledtsov
mikhailo.seledtsov at oracle.com
Thu Nov 9 19:15:38 UTC 2017
Here is a differential webrev:
http://cr.openjdk.java.net/~mseledtsov/8189762.01-02/webrev/
Misha
On 11/8/17, 5:26 PM, Mikhailo Seledtsov wrote:
> Here is an update webrev:
> http://cr.openjdk.java.net/~mseledtsov/8189762.02/
> I also tried to generate a diff between 01 and 02, but could not. My
> script does not seem to work any more, or I am missing something.
> Igor, please let me know if you have scripts or command line to
> generate incremental webrev (I committed my initial changes locally,
> then applied the new changes on top).
>
> Summary for updated webrev.02:
> - implemented review feedback from Bob and Igor
> - added @driver to all tests, since the actual testing happens in the
> child docker process; the main is just a test driver
> - configured docker directory for exclusive access by jtreg tests,
> since there are potential and actual issues when running these tests
> concurrently
> - modified test groups to add a hotspot_docker test group, and to
> exclude docker testing from lower tiers
> (once the infra is ready, I plan to add this execution initially
> at higher tiers; first will run it for a while as custom runs)
>
> My next steps:
> - rebase to new jdk tree
> - update/merge
> - retest
>
> Thank you,
> Misha
>
>
> On 11/6/17, 9:20 PM, Mikhailo Seledtsov wrote:
>> Hi Igor,
>>
>> Thank you for reviewing the code.
>>
>> On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
>>> Hi Misha,
>>>
>>> I have several comments:
>>> 1. whitebox.cpp : WB_IsContainerized has an incorrect signature, it
>>> should be WB_IsContainerized(JNIEnv*, jobject)
>> Fixed
>>> 2. CPUSetsReader.java : listToString can be rewritten using stream
>>> api as
>>> list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
>> Thank you. I will try it out.
>>> 3. in several files, I have noticed some problems w/ indents which
>>> make code quite hard to read, for example
>>>> 73 Common.run( Common.newOpts(imageName)
>>>> 74 .addDockerOpts("--memory", valueToSet))
>>>> 75 .shouldMatch("Memory Limit is:.*" +
>>>> expectedTraceValue);
>>> or
>>>> 96 DockerRunOptions opts = Common.newOpts(imageName,
>>>> "AttemptOOM")
>>>> 97 .addDockerOpts("--memory", dockerMemLimit,
>>>> "--memory-swap", dockerMemLimit);
>>>> 98 opts.classParams.add("" + sizeToAllocInMb);
>> OK. I will unwind these expressions, and make sure indentation looks
>> good.
>>> 4. TestCPUSets.java : it's better to use
>>> Assert.assertEquals(parts.length, 2) than
>>> Asserts.assertTrue(parts.length == 2) as the former will also print
>>> the actual value of parts.length. so you won't need to have L#103
>> OK
>>> 5. Test*: there is no need to have parentheses in '@requires
>>> (docker.support)'
>> OK
>>> 6. Test*: ClassFileInstaller doesn't have to be run by JDK under
>>> test, so you can use '@run driver' to run it
>> Will fix.
>>> 7. TestCPUSets.java : L#72, to get a proper new line symbol you
>>> should use %n in format string. also System.out::printf seems to be
>>> more popular in our code than System.out::format
>> OK.
>>> 8. TestCPUSets.java: shouldn't checkResult assert that we found
>>> lineMarker?
>> Oops. Missed it. Will add code to check it.
>>> 9. Test*: would you consider adding .shouldHaveExitValue(0) to all
>>> Common.run calls which aren't expected to fail, e.g. all test* in
>>> TestCPUAwareness, testMemorySoftLimit and testMemoryLimit in
>>> TestMemoryAwareness
>> Common.run() already checks for .shouldHaveExitValue(0) after running
>> the container.
>>
>>
>> I will apply feedback from Bob and you, and will post a new webrev.
>>
>>
>> Thank you,
>> Misha
>>>
>>> Thanks,
>>> -- Igor
>>>
>>>> On Nov 1, 2017, at 8:11 PM,
>>>> mikhailo<mikhailo.seledtsov at oracle.com> wrote:
>>>>
>>>> Please review these tests that were developed to test JVM's
>>>> container awareness in Docker environment.
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>>>> Webrev:
>>>> Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/
>>>> WB API:
>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/
>>>> Testing:
>>>> 1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>>> Ran the developed tests via jtreg
>>>> Pass
>>>>
>>>> 2. Automated testing system - run these tests
>>>> In progress
>>>>
>>>> Thank you,
>>>> Misha
>>>>
More information about the hotspot-runtime-dev
mailing list