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

Thu Nov 9 21:04:21 UTC 2017

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
>>>>>>>>
>>>
>