From: morphes1995 Date: Thu, 4 Dec 2025 10:15:19 +0000 (+0000) Subject: rgw: ops large budget caused by rate limiter was enabled and max_ops set to 0 X-Git-Tag: testing/wip-vshankar-testing-20260213.071255~10^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=91e9f00a7543c4651a983c2d785696a069d8ab28;p=ceph-ci.git rgw: ops large budget caused by rate limiter was enabled and max_ops set to 0 Signed-off-by: morphes1995 --- diff --git a/src/rgw/rgw_ratelimit.h b/src/rgw/rgw_ratelimit.h index 3da24306307..6410d197758 100644 --- 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 09e088204a4..88dae1be544 100644 --- 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