feat(server): introduce rss oom limit #3702

adiholden · 2024-09-12T13:03:31Z

adiholden · 2024-09-19T07:52:47Z

src/server/main_service.cc

@@ -103,6 +103,11 @@ ABSL_FLAG(double, oom_deny_ratio, 1.1,
          "commands with flag denyoom will return OOM when the ratio between maxmemory and used "
          "memory is above this value");

+ABSL_FLAG(double, rss_oom_deny_ratio, 1.1,


@romange do you think this should be enabled by default or disabled by default?

enabled but maybe increase the default ratio to 1.25 because it affects maxmemory semantics

we also need to provide explanation in release notes

what if one upgrades to new version and does not use admin port. if he hits the rss limit he will not be able to even connect to the server as we will reject new connections

hmm, you are right but how we introduce this behavior then? I do think that some sane multiplier is needed. This can be eve 2,3 but if RSS jumps by a huge factor - it's not ok.

maybe we should reject new connections only if admin port is set?

src/server/main_service.cc

chakaz · 2024-09-19T08:39:36Z

src/server/main_service.cc

+    auto memory_stats = etl.GetMemoryUsage(start_ns);
    double oom_deny_ratio = GetFlag(FLAGS_oom_deny_ratio);
-    if (used_memory > (max_memory_limit * oom_deny_ratio)) {
-      DLOG(WARNING) << "Out of memory, used " << used_memory << " vs limit " << max_memory_limit;
+    double rss_oom_deny_ratio = GetFlag(FLAGS_rss_oom_deny_ratio);
+    if (memory_stats.used_mem > (max_memory_limit * oom_deny_ratio) ||
+        (rss_oom_deny_ratio > 0 &&
+         memory_stats.rss_mem > (max_memory_limit * rss_oom_deny_ratio))) {
+      DLOG(WARNING) << "Out of memory, used " << memory_stats.used_mem << " ,rss "
+                    << memory_stats.rss_mem << " ,limit " << max_memory_limit;


I'd wrap this logic, possibly including the DLOG part, in a function

chakaz · 2024-09-19T08:40:16Z

src/server/main_service.cc

+    auto memory_stats = etl.GetMemoryUsage(start_ns);
    double oom_deny_ratio = GetFlag(FLAGS_oom_deny_ratio);


Are we ok with reading 2 flags in the hot path?
We could have thread-local caches, and update them upon flag update

ok using thread local now

chakaz

Please wait for Roman's approval on the default value

romange · 2024-09-19T12:53:04Z

src/server/main_service.cc

-optional<ErrorReply> Service::VerifyCommandExecution(const CommandId* cid,
-                                                     const ConnectionContext* cntx,
-                                                     CmdArgList tail_args) {
+static optional<ErrorReply> ShouldDenyOnOOM(const CommandId* cid) {


it will be simpler if this function returns bool

romange · 2024-09-19T12:58:27Z

src/server/main_service.cc

@@ -925,7 +955,12 @@ void Service::Init(util::AcceptServer* acceptor, std::vector<facade::Listener*>
  }

  // Initialize shard_set with a global callback running once in a while in the shard threads.
-  shard_set->Init(shard_num, [this] { server_family_.GetDflyCmd()->BreakStalledFlowsInShard(); });
+  shard_set->Init(shard_num, [this] {
+    server_family_.GetDflyCmd()->BreakStalledFlowsInShard();


Please note that EngineShard::RunPeriodic runs shard_handler once in 100ms, even though the loop inside RunPeriodic is once in 1ms.
So with this code now server_family_.UpdateMemoryGlobalStats(); will run once in 100ms as well. I think it's not ok. I suggest that you move the 100ms check here and make it special only for BreakStalledFlowsInShard() so that shard_handler will be called every iteration inside RunPeriodic.

romange · 2024-09-19T13:09:37Z

src/server/server_family.cc

+    return;
+  }
+  time_t curr_time = time(nullptr);
+  if (curr_time == global_stats_update_time_) {  // Runs one a second.


I see you changed semantics of the previous code by updating used_mem_peak once in a second. I am confused now whether we need to run UpdateMemoryGlobalStats as frequently as 1ms. Maybe you are right and it's not needed. But I think it's an opportunity to simplify this logic so that timing rules will be clearer.

Lets run UpdateMemoryGlobalStats once in 100ms, and the whole shard_handler will run once in 100ms. But then lets remove the 1s restriction here and update used_mem_peak, rss_mem_current, rss_mem_peak once in 100ms as wll

ok I removed the logic to restrict this logic to run only once a second

romange · 2024-09-19T13:10:56Z

src/server/server_family.cc

+    if (rss_mem_peak.load(memory_order_relaxed) < total_rss) {
+      rss_mem_peak.store(total_rss, memory_order_relaxed);
+    }
+    double rss_oom_deny_ratio = absl::GetFlag(FLAGS_rss_oom_deny_ratio);


you added rss_oom_deny_ratio to server state but here you use the flag.

romange · 2024-09-19T14:22:19Z

src/server/server_family.h

  Service& service_;

  util::AcceptServer* acceptor_ = nullptr;
  std::vector<facade::Listener*> listeners_;
+  bool accepting_connections_ = true;
+  time_t global_stats_update_time_ = 0;


no need for global_stats_update_time_

romange

lgtm

Signed-off-by: adi_holden <adi@dragonflydb.io>

Co-authored-by: Shahar Mike <chakaz@users.noreply.github.com> Signed-off-by: adiholden <adi@dragonflydb.io>

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden force-pushed the rss_oom_ratio branch from e2671af to 678a6c9 Compare September 17, 2024 21:17

adiholden changed the title ~~Do not review yet: rss oom limit~~ feat(server): introduce rss oom limit Sep 19, 2024

adiholden requested review from romange and chakaz September 19, 2024 07:51

adiholden commented Sep 19, 2024

View reviewed changes

chakaz reviewed Sep 19, 2024

View reviewed changes

adiholden requested a review from chakaz September 19, 2024 12:34

chakaz previously approved these changes Sep 19, 2024

View reviewed changes

romange reviewed Sep 19, 2024

View reviewed changes

adiholden dismissed chakaz’s stale review via b6dc09e September 19, 2024 13:26

romange reviewed Sep 19, 2024

View reviewed changes

adiholden added 15 commits September 19, 2024 17:32

Do not review yet: rss oom limit

080af2f

Signed-off-by: adi_holden <adi@dragonflydb.io>

unset in unit test

b84600e

Signed-off-by: adi_holden <adi@dragonflydb.io>

add pytest

1c16bc1

Signed-off-by: adi_holden <adi@dragonflydb.io>

add pytest

8d2e8fc

Signed-off-by: adi_holden <adi@dragonflydb.io>

add prints to pytest

44cfe3a

Signed-off-by: adi_holden <adi@dragonflydb.io>

increase sleep

1bbd7ed

Signed-off-by: adi_holden <adi@dragonflydb.io>

add prints

ac52313

Signed-off-by: adi_holden <adi@dragonflydb.io>

update oom stats on info

d5ef120

Signed-off-by: adi_holden <adi@dragonflydb.io>

update test

cf295a7

Signed-off-by: adi_holden <adi@dragonflydb.io>

prints for debug

60a825a

Signed-off-by: adi_holden <adi@dragonflydb.io>

remove vlog

301e447

Signed-off-by: adi_holden <adi@dragonflydb.io>

fix build

9323ed0

Signed-off-by: adi_holden <adi@dragonflydb.io>

add more prints for debuging

97d33ab

Signed-off-by: adi_holden <adi@dragonflydb.io>

add more prints for debuging

5337bbc

Signed-off-by: adi_holden <adi@dragonflydb.io>

increase rss deny ratio

fb19e92

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden and others added 8 commits September 19, 2024 17:33

cleanup

5019335

Signed-off-by: adi_holden <adi@dragonflydb.io>

cleanup

b760037

Signed-off-by: adi_holden <adi@dragonflydb.io>

use thread local instead of flags

974a2a5

Signed-off-by: adi_holden <adi@dragonflydb.io>

Update src/server/main_service.cc

3d6d5ca

Co-authored-by: Shahar Mike <chakaz@users.noreply.github.com> Signed-off-by: adiholden <adi@dragonflydb.io>

use thread local instead of flags

0c1bfb1

Signed-off-by: adi_holden <adi@dragonflydb.io>

change default val

94d80b3

Signed-off-by: adi_holden <adi@dragonflydb.io>

fix pr

4f083db

Signed-off-by: adi_holden <adi@dragonflydb.io>

fix pr

bfbde9e

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden force-pushed the rss_oom_ratio branch from 0bb67ed to bfbde9e Compare September 19, 2024 14:33

adiholden added 2 commits September 19, 2024 17:43

fix pr

31e00d8

Signed-off-by: adi_holden <adi@dragonflydb.io>

fix unit test

016c1dc

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden requested a review from romange September 19, 2024 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): introduce rss oom limit #3702

feat(server): introduce rss oom limit #3702

adiholden commented Sep 12, 2024

adiholden Sep 19, 2024

romange Sep 19, 2024

romange Sep 19, 2024

adiholden Sep 19, 2024

romange Sep 19, 2024

adiholden Sep 19, 2024

chakaz Sep 19, 2024

chakaz Sep 19, 2024

adiholden Sep 19, 2024

chakaz left a comment

romange Sep 19, 2024

romange Sep 19, 2024

romange Sep 19, 2024

adiholden Sep 19, 2024

romange Sep 19, 2024

adiholden Sep 19, 2024

romange Sep 19, 2024

romange left a comment

		auto memory_stats = etl.GetMemoryUsage(start_ns);
		double oom_deny_ratio = GetFlag(FLAGS_oom_deny_ratio);

feat(server): introduce rss oom limit #3702

Are you sure you want to change the base?

feat(server): introduce rss oom limit #3702

Conversation

adiholden commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chakaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange left a comment

Choose a reason for hiding this comment