RFR: 8292083: Detected container memory limit may exceed physical machine memory [v2]

[Jonathan Dowland]

On Tue, 16 Aug 2022 17:46:22 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> Jonathan Dowland has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Separate out debug logging for three invalid memory limit scenarios
>>    
>>    Refactor the ternary expression into an if/else chain and expand it
>>    to the third case (memory limit equal to or exceeding physical RAM)
>>    
>>    Format the trace log message for that case to match that of the other
>>    two
>>    
>>    Adjust the other two to incorporate physical RAM into the log message
>>  - Ensure trace log is enabled before trace logging
>>    
>>    Thanks Severin
>
> src/hotspot/os/linux/osContainer_linux.cpp line 71:
> 
>> 69:     os::Linux::set_physical_memory(mem_limit);
>> 70:     log_info(os, container)("Memory Limit is: " JLONG_FORMAT, mem_limit);
>> 71:   } else if (mem_limit >= host_memory) {
> 
> We should probably change this to `else if (log_is_enabled(Trace, os, container) && mem_limit >= host_memory) {...` as we only do trace logs in that branch.

Thanks for the catch. Addressed in `7a63709ff7a`

> src/hotspot/os/linux/os_linux.cpp line 228:
> 
>> 226:     }
>> 227:     log_debug(os, container)("container memory limit %s: " JLONG_FORMAT ", using host value",
>> 228:                             mem_limit == OSCONTAINER_ERROR ? "failed" : "unlimited", mem_limit);
> 
> These log lines now result in messages like:
> 
> 
> [0.037s][debug][os,container] container memory limit unlimited: 38654705664, using host value
> 
> 
> It looks a bit strange, so perhaps we should account for the exceeding-host-memory case. Thoughts?

You're right. I've hopefully addressed this in  `8d7e80c6253`

> Could we give this test a better name? How about `testContainerMemExceedsPhysical`?

Of course - that name is good. 

>  Anyway, we could perhaps write a WhiteBox based one which asserts that the os::physical_memory() never exceeds the host memory. See WhiteBox.printOsInfo() for an example as to how that's done.

Do you mean as well as, or instead of, the `OperatingSystemMXBean` approach I've currently taken? (I'm aware it's a bit of a smell. I did test preliminary backports to 11u and 8u, and this is one area that isn't clean)

Looking at `printOsInfo`: is your suggestion to call that and inspect the output, or extend `WhiteBox` itself with a physical memory limit method? I lean towards the latter

-------------

PR: https://git.openjdk.org/jdk/pull/9880