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

Thu Nov 9 20:31:47 UTC 2017

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