RFR(XS): 8227317: [TESTBUG] jdk docker/TestDockerMemoryMetrics.java fails on systems w/o kernel memory accounting

[mikhailo.seledtsov at oracle.com]

Here is the updated webrev: 
http://cr.openjdk.java.net/~mseledtsov/8227317.02/


Thanks,

Misha

On 10/23/19 3:26 PM, mikhailo.seledtsov at oracle.com wrote:
> Hi Severin,
>
> On 10/23/19 2:08 AM, Severin Gehwolf wrote:
>> Hi Misha,
>>
>> On Tue, 2019-10-22 at 11:50 -0700, mikhailo.seledtsov at oracle.com wrote:
>>> Please review this change that adds extra check for test case
>>> testKernelMemoryLimit(). If system does not support kernel memory
>>> accounting then verification for this test case is skipped.
>>>
>>>       JBS: https://bugs.openjdk.java.net/browse/JDK-8227317
>>>       Webrev: http://cr.openjdk.java.net/~mseledtsov/8227317.01/
>>   123         OutputAnalyzer oa = DockerTestUtils.dockerRunJava(opts);
>>   124         if (oa.getStderr().contains("kernel memory accounting 
>> disabled")) {
>>   125             System.out.println("Kernel memory accounting 
>> disabled on this system, " +
>>   126                                        "skipping the test case");
>>   127             return;
>>   128         }
>>   129         oa.shouldHaveExitValue(0).shouldContain("TEST PASSED!!!");
>>   130     }
>>
>>  From what I understand this message comes from some container runtimes
>> (runc in this case), right? It seems to be a case where the system
>> potentially has kernel memory accounting, but the container runtime was
>> compiled without support for it. Have you checked whether systems where
>> the test fails also have 'cgroup.memory=nokmem' as kernel
>> commandline[1]?
>
> I am not sure, could be either case. In any case, the tests should be 
> able to handle this type of error.
>
> Perhaps, I should change the message to simply echo the original 
> message from the docker command: "Kernel memory accounting disabled, 
> skipping test case",
>
> that is w/o mentioning "on this system".
>
>
>> In either case it seems diligent to add a comment prior the if.
>>
>> Suggestion:
>>
>>         OutputAnalyzer oa = DockerTestUtils.dockerRunJava(opts);
>>         // Some container runtimes (e.g. runc, docker 18.09)
>>         // have been built without kernel memory accounting. In
>>         // that case, the runtime issues a message on stderr saying
>>         // so. Skip the test in that case.
>>         if (oa.getStderr().contains("kernel memory accounting 
>> disabled")) {
>
> Thanks. I will add the comment that you recommend.
>
> Thank you,
>
> Misha
>
>>
>> Thanks,
>> Severin
>>
>> [1] cat /proc/cmdline | grep cgroup.memory=nokmem
>>
>>>       Testing:
>>>          1. Ran the test on systems with disabled kernel memory
>>> accounting before and after the fix - fix works
>>>          2. Ran the test case 60 times on multiple Linux-x64 systems 
>>> - no
>>> failures
>>>
>>>
>>> Thank you,
>>> Misha
>>>