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

[Leonid Mesnik]

Looks good.

Leonid

> On Oct 23, 2019, at 5:25 PM, mikhailo.seledtsov at oracle.com wrote:
> 
> 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
>>>>