RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration
Mikhailo Seledtsov
mikhailo.seledtsov at oracle.com
Wed Nov 15 21:37:22 UTC 2017
Hi Igor, Bob,
Webrev containing latest changes based on feedback from you:
Diff from 02: http://cr.openjdk.java.net/~mseledtsov/8189762.02-03/
Full: http://cr.openjdk.java.net/~mseledtsov/8189762.03/
Thank you,
Misha
On 11/14/17, 4:00 PM, Igor Ignatyev wrote:
> Hi Misha,
>
> a few more comments:
> - are you using CPUSetsReader::listToString(boolean abc, List<Integer>
> list, int maxCount) method? I wasn't able to find any usages of it.
> - CPUSetsReader uses Asserts.assertTrue(parts.length == 2) instead of
> Asserts.assertEQ(parts.length, 2)
> - instead of using Stream::collect in
> CPUSetsReader::readFromProcStatus, you can use Stream::findFirst(). so
> it'll be smth like
>> + try (Stream<String> stream = Files.lines(Paths.get(path))) {
>> + o = stream
>> + .filter(line -> line.contains(setType))
>> + .findFirst();
>> + } catch (IOException e) {
>> + return null;
>> + }
>> +
>> + if (!o.isPresent()) {
>> + return null;
>> + }
>> +
>> + String[] parts = o.get().replaceAll("\\s","").split(":");
> - you don't need to box int into Integer in TestCPUSets::checkResult
> L#108:
>> Asserts.assertEquals(new Integer(parts.length), new Integer(2));
> - IIRC, in order to properly use whitebox API, you have to have
> both sun.hotspot.WhiteBox and sun.hotspot.WhiteBox$WhiteBoxPermission
> in BCP, but '@run driver ClassFileInstaller -jar whitebox.jar
> sun.hotspot.WhiteBox', which is used in all your tests, will put only
> s.h.WhiteBox in whitebox.jar
> - how do you plan to use :hotspot_containers test group? can't you use
> the test directory instead?
>
> the rest looks good to me.
>
> Thanks,
> -- Igor
>
>> On Nov 9, 2017, at 1:04 PM, Mikhailo Seledtsov
>> <mikhailo.seledtsov at oracle.com
>> <mailto:mikhailo.seledtsov at oracle.com>> wrote:
>>
>> OK, will remove it.
>>
>> Misha
>>
>> On 11/9/17, 12:38 PM, Bob Vandette wrote:
>>> Since that path is not guaranteed to work, I’d remove
>>> CGROUP_CPUSET_PATH and the function.
>>>
>>> Bob.
>>>
>>>> On Nov 9, 2017, at 3:31 PM, Mikhailo Seledtsov
>>>> <mikhailo.seledtsov at oracle.com
>>>> <mailto:mikhailo.seledtsov at oracle.com>> wrote:
>>>>
>>>> I am not using it. I thought it may be helpful if we go back to
>>>> that method of extracting cpuset values for some reason.
>>>> On the other hand, dead code is not a good thing. So I am a bit
>>>> undecided.
>>>> I will delete it if you think it is better to do so.
>>>>
>>>> Misha
>>>>
>>>> On 11/9/17, 12:00 PM, Bob Vandette wrote:
>>>>> Do you still need this function? I don’t see it being used.
>>>>>
>>>>> 84 public static String read(String fileName) {
>>>>> 85 String path = CGROUP_CPUSET_PATH + fileName;
>>>>> 86 try {
>>>>> 87 return readLineFromFile(path);
>>>>> 88 } catch (Exception e) {
>>>>> 89 System.err.println("Exception reading " + path);
>>>>> 90 System.err.println("Exception: " + e);
>>>>> 91 }
>>>>> 92
>>>>> 93 return null;
>>>>> 94 }
>>>>>
>>>>> Bob.
>>>>>
>>>>>> On Nov 9, 2017, at 2:15 PM, Mikhailo Seledtsov
>>>>>> <mikhailo.seledtsov at oracle.com
>>>>>> <mailto:mikhailo.seledtsov at oracle.com>> wrote:
>>>>>>
>>>>>> Here is a differential webrev:
>>>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.01-02/webrev/
>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/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/
>>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/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
>>>>>>>>>> <mailto: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/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00/>
>>>>>>>>>> WB API:
>>>>>>>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/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