rgw: ops large budget caused by rate limiter was enabled and max_ops set to 0

Signed-off-by: morphes1995 <morphes1995@gmail.com>

diff --git a/src/rgw/rgw_ratelimit.h b/src/rgw/rgw_ratelimit.h
index 3da2430630731dfc95dd7ff36cdb123c84686fc9..6410d197758a86c5a4ee06cf02fd2405a297568e 100644 (file)
--- a/src/rgw/rgw_ratelimit.h
+++ b/src/rgw/rgw_ratelimit.h
@@ -61,7 +61,12 @@ class RateLimiterEntry {
     return true;
   }
     // we don't want to reduce ops' tokens if we've rejected it.
-    read.ops -= fixed_point_rgw_ratelimit;
+    if (read_ops() > 0)
+    {
+      // make sure read.ops always greater or equal than 0
+      // to avoid large budget caused by the case that rate limiter was enabled and read.ops set to 0
+      read.ops -= fixed_point_rgw_ratelimit;
+    }
     return false;
   }
   bool should_rate_limit_list(int64_t ops_limit)
@@ -69,7 +74,12 @@ class RateLimiterEntry {
     if ((list_ops() - 1 < 0) && (ops_limit > 0)) {
       return true;
     }
-    list.ops -= fixed_point_rgw_ratelimit;
+    if (list_ops() > 0)
+    {
+      // make sure list.ops always greater or equal than 0
+      // to avoid large budget caused by the case that rate limiter was enabled and list.ops set to 0
+      list.ops -= fixed_point_rgw_ratelimit;
+    }
     return false;
   }
   bool should_rate_limit_delete(int64_t ops_limit)
@@ -77,7 +87,12 @@ class RateLimiterEntry {
     if ((delete_ops() - 1 < 0) && (ops_limit > 0)) {
       return true;
     }
-    del.ops -= fixed_point_rgw_ratelimit;
+    if (delete_ops() > 0)
+    {
+      // make sure del.ops always greater or equal than 0
+      // to avoid large budget caused by the case that rate limiter was enabled and del.ops set to 0
+      del.ops -= fixed_point_rgw_ratelimit;
+    }
     return false;
   }
   bool should_rate_limit_write(int64_t ops_limit, int64_t bw_limit) 
@@ -90,7 +105,12 @@ class RateLimiterEntry {
     }
 
     // we don't want to reduce ops' tokens if we've rejected it.
-    write.ops -= fixed_point_rgw_ratelimit;
+    if (write_ops() > 0)
+    {
+      // make sure write.ops always greater or equal than 0
+      // to avoid large budget caused by the case that rate limiter was enabled and write.ops set to 0
+      write.ops -= fixed_point_rgw_ratelimit;
+    }
     return false;
   }
   /* The purpose of this function is to minimum time before overriding the stored timestamp

diff --git a/src/test/rgw/test_rgw_ratelimit.cc b/src/test/rgw/test_rgw_ratelimit.cc
index 09e088204a43e3a35b6a7e95c7f8ae094eacf5d5..88dae1be544f0e8689ff968488dd2547bc22bf49 100644 (file)
--- a/src/test/rgw/test_rgw_ratelimit.cc
+++ b/src/test/rgw/test_rgw_ratelimit.cc
@@ -189,6 +189,50 @@ TEST(RGWRateLimit, allow_unlimited_access)
   EXPECT_EQ(false, success);
 }
 
+TEST(RGWRateLimit, unlimited_access_not_left_large_read_ops_budget)
+{
+  // 0 values in RGWRateLimitInfo should allow unlimited access
+  std::atomic_bool replacing;
+  std::condition_variable cv;
+  RateLimiter ratelimit(g_ceph_context, replacing, cv);
+  RGWRateLimitInfo info;
+  info.enabled = true;
+  auto time = ceph::coarse_real_clock::now();
+  std::string key = "uuser123";
+
+  for (int i = 0; i < 10; i++)
+  {
+    bool success = ratelimit.should_rate_limit("GET", key, time, &info, "");
+    EXPECT_EQ(false, success);
+  }
+  time += 61s;
+  info.max_read_ops = 1; // make read ops limited
+  bool success = ratelimit.should_rate_limit("GET", key, time, &info, "");
+  EXPECT_EQ(false, success);
+}
+
+TEST(RGWRateLimit, unlimited_access_not_left_large_write_ops_budget)
+{
+  // 0 values in RGWRateLimitInfo should allow unlimited access
+  std::atomic_bool replacing;
+  std::condition_variable cv;
+  RateLimiter ratelimit(g_ceph_context, replacing, cv);
+  RGWRateLimitInfo info;
+  info.enabled = true;
+  auto time = ceph::coarse_real_clock::now();
+  std::string key = "uuser123";
+
+  for (int i = 0; i < 10; i++)
+  {
+    bool success = ratelimit.should_rate_limit("PUT", key, time, &info, "");
+    EXPECT_EQ(false, success);
+  }
+  time += 61s;
+  info.max_write_ops = 1; // make write ops limited
+  bool success = ratelimit.should_rate_limit("PUT", key, time, &info, "");
+  EXPECT_EQ(false, success);
+}
+
 TEST(RGWRateLimitGC, NO_GC_AHEAD_OF_TIME)
 {
   // Test if GC is not starting the replace before getting to map_size * 0.9
@@ -489,6 +533,28 @@ TEST(RGWRateLimit, list_limit_does_not_affect_writes)
   EXPECT_EQ(false, success);
 }
 
+TEST(RGWRateLimit, unlimited_access_not_left_large_list_ops_budget)
+{
+  // 0 values in RGWRateLimitInfo should allow unlimited access
+  std::atomic_bool replacing;
+  std::condition_variable cv;
+  RateLimiter ratelimit(g_ceph_context, replacing, cv);
+  RGWRateLimitInfo info;
+  info.enabled = true;
+  auto time = ceph::coarse_real_clock::now();
+  std::string key = "uuser_list";
+
+  for (int i = 0; i < 10; i++)
+  {
+    bool success = ratelimit.should_rate_limit("GET", key, time, &info, RES_LIST_TYPE_2);
+    EXPECT_EQ(false, success);
+  }
+  time += 61s;
+  info.max_list_ops = 1; // make list ops limited
+  bool success = ratelimit.should_rate_limit("GET", key, time, &info, RES_LIST_TYPE_2);
+  EXPECT_EQ(false, success);
+}
+
 TEST(RGWRateLimit, write_limit_does_not_affect_lists)
 {
   // write limit does not affect lists
@@ -908,6 +974,28 @@ TEST(RGWRateLimit, delete_limit_does_not_affect_writes)
   EXPECT_EQ(false, success);
 }
 
+TEST(RGWRateLimit, unlimited_access_not_left_large_delete_ops_budget)
+{
+  // 0 values in RGWRateLimitInfo should allow unlimited access
+  std::atomic_bool replacing;
+  std::condition_variable cv;
+  RateLimiter ratelimit(g_ceph_context, replacing, cv);
+  RGWRateLimitInfo info;
+  info.enabled = true;
+  auto time = ceph::coarse_real_clock::now();
+  std::string key = "uuser_delete";
+
+  for (int i = 0; i < 10; i++)
+  {
+    bool success = ratelimit.should_rate_limit("DELETE", key, time, &info, "");
+    EXPECT_EQ(false, success);
+  }
+  time += 61s;
+  info.max_delete_ops = 1; // make delete ops limited
+  bool success = ratelimit.should_rate_limit("DELETE", key, time, &info, "");
+  EXPECT_EQ(false, success);
+}
+
 TEST(RGWRateLimitEntry, reject_delete_op_over_limit)
 {
   // check that DELETE request is being rejected because there are not enough tokens