RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Igor Ignatyev igor.ignatyev at oracle.com
Wed Nov 15 21:49:19 UTC 2017 

looks good to me

-- Igor

> On Nov 15, 2017, at 1:37 PM, Mikhailo Seledtsov <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/ <http://cr.openjdk.java.net/~mseledtsov/8189762.02-03/>
>   Full:                http://cr.openjdk.java.net/~mseledtsov/8189762.03/ <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 <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