RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration
Bob Vandette
bob.vandette at oracle.com
Thu Nov 9 20:38:04 UTC 2017
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> 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