fix memory limit with croup

[Nicholaswang]

use hierarchical_memory_limit of memory.stat instead of memory.limit_in_bytes since limit_in_bytes is often inherited from pod node and sometimes it's unlimited. hierarchical_memory_limit is more accurate.

[rkoster]

@Nicholaswang thanks for you Pull Request, could you also add a test in: https://github.com/cloudfoundry/gosigar/blob/master/sigar_linux_test.go

[rkoster]

@Nicholaswang in its current form we won't be able to accept this PR? Would you be able the test as requested in the previous comment?

[Nicholaswang]

Sorry for the message loss, I will extend the unit tests ASAP.

[Nicholaswang]

@rkoster hi, unit tests have been extended

[ramonskie]

looks good to me.

[klakin-pivotal]

Yeah, this is definitely something we missed in the original work. Thanks much for the PR.

Once you resolve the two issues I've mentioned, this PR should be good to merge and run through our CI machinery.

(For what it's worth, I'm not sure why we would ever use memory.limit_in_bytes instead of hierarchical_memory_limit, but I don't know enough about cgroups to be suggest that we rely on h_m_l exclusively... so the "Use h_m_l if m.limit_in_bytes is 'unlimited'." logic seems safe and good to me.)

=================================================

For the benefit of others who might come across this PR in the future, this is what I did to validate that the reported problem is a problem and that the proposed solution is a good one:

# uname -r
4.15.0-175-generic
# cd /sys/fs/cgroup/memory
# mkdir lakin-cgroup
# mkdir lakin-cgroup/lakin-subcgroup
# cd lakin-cgroup
# cat memory.limit_in_bytes
9223372036854771712
# cat lakin-subcgroup/memory.limit_in_bytes
9223372036854771712
# echo "1073741824" > memory.limit_in_bytes
# cat memory.limit_in_bytes
1073741824
# cat lakin-subcgroup/memory.limit_in_bytes
9223372036854771712
# grep hierarchical_memory_limit memory.stat
hierarchical_memory_limit 1073741824
# grep hierarchical_memory_limit lakin-subcgroup/memory.stat
hierarchical_memory_limit 1073741824

[metonymic-smokey]

.

[metonymic-smokey]

@klakin-pivotal @ramonskie for containers, could you explain why you picked memory.high instead of memory.max to get the memory limit?
I think memory.max is the total available memory limit right, and memory.high indicates memory usage is high and now throttled?
I'm running examples/free.go inside a docker container.
Please let me know what you think.
Thanks!

[klakin-pivotal]

Hello, @metonymic-smokey !

@andreas-kupries was the fellow who wrote that code, so he'd be most likely to be able to give you the best answer. However, the choice seemed reasonable to me when I merged in that code (in PR #50), and still seems reasonable today.

I will attempt to justify the decision, and would appreciate corrections for any part that I get wrong.

According to the documentation (https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html#memory-interface-files), exceeding memory.high will never invoke the OOM killer, but runs the risk of significant performance degradation:

Memory usage throttle limit. This is the main mechanism to control memory usage of a cgroup. If a cgroup’s usage goes over the high boundary, the processes of the cgroup are throttled and put under heavy reclaim pressure.

The documentation for memory.max explicitly mentions that it exists as a memory usage barrier of last resort, and your process has a real risk of getting killed by the OOM killer if this limit is reached:

Memory usage hard limit. This is the final protection mechanism. If a cgroup’s memory usage reaches this limit and can’t be reduced, the OOM killer is invoked in the cgroup. ...
As long as the high limit is used and monitored properly, this limit’s utility is limited to providing the final safety net.

The documentation indicates that it is a bad thing to reach memory.max. If your process has nothing that can be reclaimed, you will be killed by the OOM killer... which is never a good thing.

You might also find the explanation here to be informative: https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html#id1 Among other things, it describes the shortcomings of how cgroup v1 handled its high memory limit and why there's a good reason to use memory.max when using cgroup v2.

[andreas-kupries]

Hello, @metonymic-smokey !

@andreas-kupries was the fellow who wrote that code, so he'd be most likely to be able to give you the best answer. However, the choice seemed reasonable to me when I merged in that code (in PR #50), and still seems reasonable today.

Hi. This is now long enough in the past that I honestly do not fully recall anymore.
Per-using the referenced documentation to refresh my memory I stand by my choice from that time. Better a degradation when going to/over the limit than abruptly thrown out.
@klakin-pivotal 's explanation and discussion below looks very fine to me.

I will attempt to justify the decision, and would appreciate corrections for any part that I get wrong.

[klakin-pivotal]

Looks good to me!

[klakin-pivotal]

@Nicholaswang Thanks much for the PR, and taking the time to make the corrections I requested.

I have squashed down the commits into a single commit and have written up a commit message that -hopefully- correctly describes the problem that the commit solves, and how it solves it. I do hope that you are not unhappy with the commit message.

The commit has been merge into master, and all of the automated testing has succeeded. I will create a release containing this fix very soon.

[klakin-pivotal]

And we now have a release: https://github.com/cloudfoundry/gosigar/releases/tag/v1.3.4