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

[Severin Gehwolf]

On Fri, 26 Aug 2022 09:57:30 GMT, Jonathan Dowland <jdowland at openjdk.org> wrote:

>>> > I think the gist of my remark is that I would like the layers to behave consistently.
>>> > I see that `CgroupSubsystem::memory_limit_in_bytes()` is only used in two places, `os::Linux::available_memory() ` and `os::physical_memory`.
>>> 
>>> You mean `OSContainer::memory_limit_in_bytes()` right?
>> 
>> Right. 
>> 
>>> 
>>> > I would say let the `os` layer lie and `Linux` and `CgroupSystem` be the truth. Then we end up with a clear hierarchy:
>>> 
>>> There is also this `OSContainer` hybrid ;-)
>> 
>> Yes, that is what I meant with "CgroupSystem". Sorry, I collapsed that with its CGV1/V2 implementations for less confusion :)
>
> Hi @tstuefe ,
> 
> Thanks for the detailed explanation (and diagrams!) of your concern about the complexity of this. I understand what you mean. This PR was my first proper look at these subsystems, and I had some trouble unwinding it all.
> 
> The structuring you lay out in [this comment](https://github.com/openjdk/jdk/pull/9880#issuecomment-1227579739) sounds good to me, with some caveats:
> 
>> In addition, let the cgroup subsystem return defined values for "invalid" (if that is possible).
> 
> Sadly, in the case of cgroups v1, not only is there no notion of "invalid" to report, but we also need to know, at that level of abstraction, if the values we are reading exceed physical memory, because that's used to decide whether to check for a "heirarchical memory limit" or not. So, even after arranging things as you suggest, there will be a need for the bottom cgroups layer (in the v1 case) to call up into the Linux:: layer.
> 
> @jerboaa :
>> There is also this OSContainer hybrid ;-)
> 
> I got the impression this exists because the abstractions as they existed prior to v2 support needed to be extended to support cgroups v2 later on. As part of revisiting this, I wonder if we could merge OSContainer/CgroupSystem.
> 
> @tstuefe:
>> Would that make sense? I don't think this would be a huge effort. We also could do it in a separate RFE.
> 
> It would be my preference to do this in a separate RFE if that's acceptable to you. In an ideal world we would get this bug fix into jdk mainline this side of the October CPU cut-off (Aug 30 I think), as I also plan to backport to 17, 11 and 8. If you are happy with that, please mark "reviewed" in GitHub and I'll integrate and raise the RFE issue.

@jmtd Please file an RFE ticket for the proposed API cleanup. Thanks!

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

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