Add containerized linux memory manager #22997

mohsaka · 2024-06-12T23:35:23Z

Description

This PR helps shrink memory usage when we detect at the system level that Prestissimo is running out of
memory.

This PR implements a LinuxMemoryChecker which can be enabled by setting the CMake flag PRESTO_MEMORY_CHECKER_TYPE=LINUX_MEMORY_CHECKER.

We currently are using this memory manager for our deployments to prevent OOMKills. It has been well tested on CentOS.

Note that mallocBytes, periodicCb, heapDumpCb, removeDumpFile are not supported.

LinuxMemoryChecker:
We are currently using inactive_anon + active_anon to track the current system memory usage.

Our first attempt was using memInfo memTotal - memAvailable. However memInfo is not containerized so we reserve this as a last resort.

Next we tried to use what docker/kubernetes uses for their calculation. cgroup usage_in_bytes - total_inactive_files. However we found out that usage_in_bytes is a fuzz value and has a chance for the sync to occur after the shrink polling interval. This would result in double shrinks.

Therefore we decided on values from the memory.stat file that are real time statistics. At first we tried to use the calculation suggested by the kernel team RSS+CACHE(+SWAP). However we noticed that this value was not closely related to the value in usage_in_bytes which is used to OOMKill. We then looked at all of the values in the stat file and decided that inactive_anon + active_anon moves closest to that of usage_in_bytes

NOTE: We do not know if cgroup V2 memory.current is a fuzz value. It may be better than what we currently use. For consistency we will match cgroup V1 and change if necessary.

Motivation and Context

Impact

Test Plan

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... :pr:`12345`
* ... :pr:`12345`

Hive Connector Changes
* ... :pr:`12345`
* ... :pr:`12345`

If release note is NOT required, use:

== NO RELEASE NOTE ==

czentgr

Thanks for adding this!

presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp

presto-native-execution/presto_cpp/main/LinuxMemoryChecker.h

presto-native-execution/presto_cpp/main/PeriodicMemoryChecker.cpp

presto-native-execution/presto_cpp/main/PrestoServer.cpp

linux-foundation-easycla · 2024-07-19T20:23:10Z

The committers listed above are authorized under a signed CLA.

✅ login: minhancao / name: Minhan Cao (ae622b4)
✅ login: mohsaka (40a3a63)

presto-native-execution/presto_cpp/main/PrestoServer.cpp

presto-native-execution/presto_cpp/main/PrestoServer.h

czentgr

Thank you. Please squash the commits into a single commit and rebase.

presto-native-execution/presto_cpp/main/PrestoServer.cpp

presto-native-execution/presto_cpp/main/PeriodicMemoryChecker.cpp

czentgr

@majetideepak Can you please also take a look when you get a chance?

presto-native-execution/presto_cpp/main/PrestoServer.cpp

presto-native-execution/CMakeLists.txt

aditi-pandit

Thanks @minhancao and @mohsaka. Have some comments related to this code.

presto-native-execution/presto_cpp/main/LinuxMemoryChecker.h

presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp

presto-native-execution/presto_cpp/main/LinuxMemoryChecker.h

presto-native-execution/presto_cpp/main/PrestoServer.cpp

steveburnett

Thanks for the doc! Only a few nits of suggested formatting and phrasing.

presto-docs/src/main/sphinx/presto_cpp/features.rst

steveburnett

LGTM! (docs)

Pull updated branch, new doc build. Thanks!

majetideepak · 2024-08-14T15:13:43Z

I have some feedback on this. I will complete the review by EOD.

czentgr · 2024-08-14T20:56:31Z

@minhancao Can you please squash the commits into a single commit?

presto-native-execution/presto_cpp/main/PrestoServer.cpp

presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp

czentgr

One more minor fix in the doc. Rest looks good.

presto-docs/src/main/sphinx/presto_cpp/properties.rst

czentgr

Looks good. Thanks!

minhancao · 2024-10-07T20:52:16Z

@bikramSingh91 The comments are all now addressed and is ready to merge. Please take another look when you can, thanks!

bikramSingh91 · 2024-10-07T22:15:32Z

@bikramSingh91 The comments are all now addressed and is ready to merge. Please take another look when you can, thanks!

Looks good to me!

majetideepak · 2024-10-08T11:52:59Z

@xiaoxmeng, @bikramSingh91 can you please import this again?
Meta CI is required and is failing.
Meta Internal-Only Changes Check — The Diff and Pull Request are not in sync!

xiaoxmeng · 2024-10-08T18:43:23Z

@xiaoxmeng, @bikramSingh91 can you please import this again? Meta CI is required and is failing. Meta Internal-Only Changes Check — The Diff and Pull Request are not in sync!

@majetideepak @bikramSingh91 will take care once this change is landed on github.

majetideepak · 2024-10-08T19:17:47Z

@tdcmeehan can you please approve and help land this? It looks like a committer approval is needed. Thanks.

majetideepak · 2024-10-08T19:45:51Z

@xiaoxmeng You need to import it to get this to land. A required CI test is failing
Internal-Only Changes Check — The Diff and Pull Request are not in sync!

presto-docs/src/main/sphinx/presto_cpp/properties.rst

majetideepak

We saw an issue with this internally. I will update once this is fixed.

czentgr · 2024-10-09T17:14:52Z

Please also rebase this PR.

czentgr

Thank you!

presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp

czentgr · 2024-10-10T16:57:04Z

presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp

+    if (boost::regex_match(line.data(), match, regex)) {
+      if (match.size() > 1) {
+        std::string numStr(match[1].str());
+        return std::stoul(numStr);


This is incorrect. This will return a 32-bit integer which will overflow and is limited. Errno could be set to indicate a truncation warning.
It either needs to be std::stroull or use the original folly::to<size_t>(numStr);.

Thanks for the catch! Changing to use folly::to<size_t>(numStr);

Co-authored-by: Minhan Cao <minhan.duc.cao@gmail.com>

mohsaka requested review from majetideepak and czentgr June 12, 2024 23:35

mohsaka self-assigned this Jun 12, 2024

mohsaka requested a review from a team as a code owner June 12, 2024 23:35

czentgr reviewed Jun 28, 2024

View reviewed changes

minhancao self-assigned this Jul 19, 2024

minhancao force-pushed the linux-memory-manager branch from a36acf4 to dbd7e48 Compare July 22, 2024 19:14

czentgr reviewed Jul 23, 2024

View reviewed changes

presto-native-execution/presto_cpp/main/PrestoServer.cpp Outdated Show resolved Hide resolved

presto-native-execution/presto_cpp/main/PrestoServer.h Outdated Show resolved Hide resolved

minhancao force-pushed the linux-memory-manager branch 2 times, most recently from 53b5e8e to 772fd86 Compare July 23, 2024 21:12

czentgr reviewed Jul 26, 2024

View reviewed changes

presto-native-execution/presto_cpp/main/PrestoServer.cpp Outdated Show resolved Hide resolved

presto-native-execution/presto_cpp/main/PeriodicMemoryChecker.cpp Outdated Show resolved Hide resolved

minhancao force-pushed the linux-memory-manager branch 2 times, most recently from 233ea59 to 0007587 Compare July 30, 2024 19:58

czentgr reviewed Jul 30, 2024

View reviewed changes

presto-native-execution/presto_cpp/main/PrestoServer.cpp Outdated Show resolved Hide resolved

minhancao force-pushed the linux-memory-manager branch from b2d95cd to 10b65f5 Compare August 5, 2024 20:49

czentgr reviewed Aug 6, 2024

View reviewed changes

presto-native-execution/CMakeLists.txt Outdated Show resolved Hide resolved

aditi-pandit reviewed Aug 7, 2024

View reviewed changes

minhancao force-pushed the linux-memory-manager branch from 6c27e02 to f4a03c4 Compare August 8, 2024 23:22

minhancao requested review from steveburnett, elharo and a team as code owners August 8, 2024 23:22

minhancao requested a review from presto-oss August 8, 2024 23:22

steveburnett requested changes Aug 9, 2024

View reviewed changes

presto-docs/src/main/sphinx/presto_cpp/features.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/presto_cpp/features.rst Outdated Show resolved Hide resolved

steveburnett previously approved these changes Aug 9, 2024

View reviewed changes

minhancao dismissed steveburnett’s stale review via 138d6bc August 9, 2024 21:43

minhancao force-pushed the linux-memory-manager branch from 664dd0d to 2928a7e Compare August 14, 2024 21:05

aditi-pandit reviewed Aug 16, 2024

View reviewed changes

czentgr reviewed Oct 4, 2024

View reviewed changes

presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved

minhancao force-pushed the linux-memory-manager branch from 51639a8 to fc98629 Compare October 4, 2024 21:53

majetideepak approved these changes Oct 5, 2024

View reviewed changes

czentgr previously approved these changes Oct 7, 2024

View reviewed changes

xiaoxmeng previously approved these changes Oct 8, 2024

View reviewed changes

tdcmeehan previously approved these changes Oct 8, 2024

View reviewed changes

czentgr reviewed Oct 9, 2024

View reviewed changes

presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved

majetideepak requested changes Oct 9, 2024

View reviewed changes

minhancao dismissed stale reviews from tdcmeehan, xiaoxmeng, and czentgr via 290a656 October 9, 2024 17:07

minhancao force-pushed the linux-memory-manager branch from fc98629 to 290a656 Compare October 9, 2024 17:07

minhancao force-pushed the linux-memory-manager branch 2 times, most recently from 1b93112 to 1b03a96 Compare October 9, 2024 19:55

czentgr previously approved these changes Oct 9, 2024

View reviewed changes

majetideepak previously approved these changes Oct 9, 2024

View reviewed changes

presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp Show resolved Hide resolved

czentgr reviewed Oct 10, 2024

View reviewed changes

minhancao dismissed stale reviews from majetideepak and czentgr via 1ea7b0a October 10, 2024 17:09

minhancao force-pushed the linux-memory-manager branch 2 times, most recently from 1ea7b0a to 40a3a63 Compare October 10, 2024 18:39

mohsaka and others added 2 commits October 10, 2024 11:40

Add Linux Memory Manager

40a3a63

Co-authored-by: Minhan Cao <minhan.duc.cao@gmail.com>

Fixed boost::regex_match to use line.begin() and line.end()

ae622b4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add containerized linux memory manager #22997

Add containerized linux memory manager #22997

mohsaka commented Jun 12, 2024 •

edited by minhancao

Loading

czentgr left a comment

linux-foundation-easycla bot commented Jul 19, 2024 •

edited

Loading

czentgr left a comment

czentgr left a comment

aditi-pandit left a comment

steveburnett left a comment

steveburnett left a comment

majetideepak commented Aug 14, 2024

czentgr commented Aug 14, 2024

czentgr left a comment

czentgr left a comment

minhancao commented Oct 7, 2024

bikramSingh91 commented Oct 7, 2024 •

edited

Loading

majetideepak commented Oct 8, 2024

xiaoxmeng commented Oct 8, 2024

majetideepak commented Oct 8, 2024

majetideepak commented Oct 8, 2024

majetideepak left a comment

czentgr commented Oct 9, 2024

czentgr left a comment

czentgr Oct 10, 2024 •

edited

Loading

minhancao Oct 10, 2024 •

edited

Loading

Add containerized linux memory manager #22997

Are you sure you want to change the base?

Add containerized linux memory manager #22997

Conversation

mohsaka commented Jun 12, 2024 • edited by minhancao Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

czentgr left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Jul 19, 2024 • edited Loading

czentgr left a comment

Choose a reason for hiding this comment

czentgr left a comment

Choose a reason for hiding this comment

aditi-pandit left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

majetideepak commented Aug 14, 2024

czentgr commented Aug 14, 2024

czentgr left a comment

Choose a reason for hiding this comment

czentgr left a comment

Choose a reason for hiding this comment

minhancao commented Oct 7, 2024

bikramSingh91 commented Oct 7, 2024 • edited Loading

majetideepak commented Oct 8, 2024

xiaoxmeng commented Oct 8, 2024

majetideepak commented Oct 8, 2024

majetideepak commented Oct 8, 2024

majetideepak left a comment

Choose a reason for hiding this comment

czentgr commented Oct 9, 2024

czentgr left a comment

Choose a reason for hiding this comment

czentgr Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

minhancao Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

mohsaka commented Jun 12, 2024 •

edited by minhancao

Loading

linux-foundation-easycla bot commented Jul 19, 2024 •

edited

Loading

bikramSingh91 commented Oct 7, 2024 •

edited

Loading

czentgr Oct 10, 2024 •

edited

Loading

minhancao Oct 10, 2024 •

edited

Loading