RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration
Mikhailo Seledtsov
mikhailo.seledtsov at oracle.com
Thu Nov 16 00:20:30 UTC 2017
Bob, Igor,
Thank you for review.
Misha
On 11/15/17, 3:12 PM, Bob Vandette wrote:
> Looks good to me.
>
> Bob.
>
>
>> On Nov 15, 2017, at 4:37 PM, Mikhailo Seledtsov
>> <mikhailo.seledtsov at oracle.com
>> <mailto:mikhailo.seledtsov at oracle.com>> wrote:
>>
>> 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