From 33d7e6bea8935d8e26e880b3625c41692f280b04 Mon Sep 17 00:00:00 2001 From: "gd.zhou" Date: Mon, 31 Aug 2026 11:10:25 +0800 Subject: [PATCH] [feat] make min_reserved_fds runtime-configurable - add 'min-reserved-fds' config option (default 32, MODIFIABLE) - replace hardcoded CONFIG_MIN_RESERVED_FDS with server.min_reserved_fds in event loop sizing, OOM score adj and adjustOpenFilesLimit - add tryResizeSetAeSize helper to resize aeSet on value change - update swapBatchTest to size aeSet consistently --- src/config.c | 69 +++++++++++++++++++++++---- src/ctrip_swap_batch.c | 2 +- src/server.c | 10 ++-- src/server.h | 3 +- tests/swap/unit/monitor.tcl | 28 +++++++++++ tests/unit/limits.tcl | 94 +++++++++++++++++++++++++++++++++++++ 6 files changed, 190 insertions(+), 16 deletions(-) diff --git a/src/config.c b/src/config.c index 456b5ee9ffa..e4b50a73269 100644 --- a/src/config.c +++ b/src/config.c @@ -33,6 +33,7 @@ #include #include +#include /*----------------------------------------------------------------------------- * Config file name-value maps. @@ -2774,6 +2775,62 @@ static int updateSighandlerEnabled(int val, int prev, const char **err) { return 1; } +static int tryResizeSetAeSize(const char** err) { + if ((unsigned int) aeGetSetSize(server.el) < + server.maxclients + server.min_reserved_fds + CONFIG_FDSET_INCR) + { + if (aeResizeSetSize(server.el, + server.maxclients + server.min_reserved_fds + CONFIG_FDSET_INCR) == AE_ERR) + { + *err = "The event loop API used by Redis is not able to handle the specified number of clients"; + return 0; + } + } + return 1; +} + +/* Try to raise RLIMIT_NOFILE to fit server.maxclients + server.min_reserved_fds. + * Returns 1 on success (or no-op), 0 on failure with *err set. + * MUST NOT exit() — called from CONFIG SET hot path. */ +static int tryRaiseOpenFilesLimit(const char **err) { + rlim_t wanted = (rlim_t)server.maxclients + (rlim_t)server.min_reserved_fds; + struct rlimit limit; + + if (getrlimit(RLIMIT_NOFILE, &limit) == -1) { //get limit + return 1; + } + if (limit.rlim_cur >= wanted) return 1; + + struct rlimit new_limit = limit; + new_limit.rlim_cur = wanted; + new_limit.rlim_max = wanted; + if (setrlimit(RLIMIT_NOFILE, &new_limit) == -1) { //try update limit + static char msg[160]; + snprintf(msg, sizeof(msg), + "Unable to set RLIMIT_NOFILE to %llu (current %llu): %s. " + "min-reserved-fds would exceed available file descriptors.", + (unsigned long long)wanted, + (unsigned long long)limit.rlim_cur, + strerror(errno)); + *err = msg; + return 0; + } + return 1; +} + + +static int updateMinReservedFds(long long val, long long prev, const char **err) { + if (val > prev) { + if (tryRaiseOpenFilesLimit(err) == 0) { + return 0; + } + if (tryResizeSetAeSize(err) == 0) { + return 0; + } + } + return 1; +} + static int updateMaxclients(long long val, long long prev, const char **err) { /* Try to check if the OS is capable of supporting so many FDs. */ if (val > prev) { @@ -2788,15 +2845,8 @@ static int updateMaxclients(long long val, long long prev, const char **err) { } return 0; } - if ((unsigned int) aeGetSetSize(server.el) < - server.maxclients + CONFIG_FDSET_INCR) - { - if (aeResizeSetSize(server.el, - server.maxclients + CONFIG_FDSET_INCR) == AE_ERR) - { - *err = "The event loop API used by Redis is not able to handle the specified number of clients"; - return 0; - } + if (tryResizeSetAeSize(err) == 0) { + return 0; } } return 1; @@ -3119,6 +3169,7 @@ standardConfig configs[] = { /* Unsigned int configs */ createUIntConfig("max-tracking-clients-to-write", NULL, MODIFIABLE_CONFIG, 1, UINT_MAX, server.max_tracking_clients_to_write, 16, INTEGER_CONFIG, NULL, NULL), createUIntConfig("maxclients", NULL, MODIFIABLE_CONFIG, 1, UINT_MAX, server.maxclients, 10000, INTEGER_CONFIG, NULL, updateMaxclients), + createUIntConfig("min-reserved-fds", NULL, MODIFIABLE_CONFIG, 32, UINT_MAX, server.min_reserved_fds, CONFIG_MIN_RESERVED_FDS, INTEGER_CONFIG, NULL, updateMinReservedFds), #ifdef ENABLE_SWAP createUIntConfig("swap-ttl-compact-expire-percentile", NULL, MODIFIABLE_CONFIG, 1, 100, server.swap_ttl_compact_expire_percentile, 99, INTEGER_CONFIG, NULL, NULL), #endif diff --git a/src/ctrip_swap_batch.c b/src/ctrip_swap_batch.c index a420d81f761..fddada1d479 100644 --- a/src/ctrip_swap_batch.c +++ b/src/ctrip_swap_batch.c @@ -578,7 +578,7 @@ int swapBatchTest(int argc, char *argv[], int accurate) { swapRequest *out_req1, *out_req2, *utils_req; swapThreadsInit(); - server.el = aeCreateEventLoop(server.maxclients+CONFIG_FDSET_INCR); + server.el = aeCreateEventLoop(server.maxclients+ server.min_reserved_fds + CONFIG_FDSET_INCR); asyncCompleteQueueInit(); /* flush empty ctx => nop */ diff --git a/src/server.c b/src/server.c index 479671184d5..ff15125fdde 100644 --- a/src/server.c +++ b/src/server.c @@ -3230,13 +3230,13 @@ int setOOMScoreAdj(int process_class) { * max number of clients, the function will do the reverse setting * server.maxclients to the value that we can actually handle. */ void adjustOpenFilesLimit(void) { - rlim_t maxfiles = server.maxclients+CONFIG_MIN_RESERVED_FDS; + rlim_t maxfiles = server.maxclients+server.min_reserved_fds; struct rlimit limit; if (getrlimit(RLIMIT_NOFILE,&limit) == -1) { serverLog(LL_WARNING,"Unable to obtain the current NOFILE limit (%s), assuming 1024 and setting the max clients configuration accordingly.", strerror(errno)); - server.maxclients = 1024-CONFIG_MIN_RESERVED_FDS; + server.maxclients = 1024-server.min_reserved_fds; } else { rlim_t oldlimit = limit.rlim_cur; @@ -3269,11 +3269,11 @@ void adjustOpenFilesLimit(void) { if (bestlimit < maxfiles) { unsigned int old_maxclients = server.maxclients; - server.maxclients = bestlimit-CONFIG_MIN_RESERVED_FDS; + server.maxclients = bestlimit-server.min_reserved_fds; /* maxclients is unsigned so may overflow: in order * to check if maxclients is now logically less than 1 * we test indirectly via bestlimit. */ - if (bestlimit <= CONFIG_MIN_RESERVED_FDS) { + if (bestlimit <= server.min_reserved_fds) { serverLog(LL_WARNING,"Your current 'ulimit -n' " "of %llu is not enough for the server to start. " "Please increase your open file limit to at least " @@ -3538,7 +3538,7 @@ void initServer(void) { adjustOpenFilesLimit(); const char *clk_msg = monotonicInit(); serverLog(LL_NOTICE, "monotonic clock: %s", clk_msg); - server.el = aeCreateEventLoop(server.maxclients+CONFIG_FDSET_INCR); + server.el = aeCreateEventLoop(server.maxclients + server.min_reserved_fds + CONFIG_FDSET_INCR); if (server.el == NULL) { serverLog(LL_WARNING, "Failed creating the event loop. Error message: '%s'", diff --git a/src/server.h b/src/server.h index f8b4fad4e45..30d276acf2d 100644 --- a/src/server.h +++ b/src/server.h @@ -174,7 +174,7 @@ typedef long long ustime_t; /* microsecond time type. */ * of file descriptors we can handle are server.maxclients + RESERVED_FDS + * a few more to stay safe. Since RESERVED_FDS defaults to 32, we add 96 * in order to make sure of not over provisioning more than 128 fds. */ -#define CONFIG_FDSET_INCR (CONFIG_MIN_RESERVED_FDS+96) +#define CONFIG_FDSET_INCR 96 /* OOM Score Adjustment classes. */ #define CONFIG_OOM_MASTER 0 @@ -1578,6 +1578,7 @@ struct redisServer { int get_ack_from_slaves; /* If true we send REPLCONF GETACK. */ /* Limits */ unsigned int maxclients; /* Max number of simultaneous clients */ + unsigned int min_reserved_fds; unsigned long long maxmemory; /* Max number of memory bytes to use */ ssize_t maxmemory_tracking_clients; /* Memory limit for total tracking client buffers */ int maxmemory_policy; /* Policy for key eviction */ diff --git a/tests/swap/unit/monitor.tcl b/tests/swap/unit/monitor.tcl index ea4b3a004a9..400e90fde36 100644 --- a/tests/swap/unit/monitor.tcl +++ b/tests/swap/unit/monitor.tcl @@ -39,4 +39,32 @@ start_server [list overrides [list ctrip-monitor-port $mport] tags "ctrip_monito assert_match [$r3 ping] "PONG" } + test {CONFIG SET min-reserved-fds accepts a valid value at runtime} { + # Boundary: a valid value can be set and CONFIG GET returns the same value + # Monitor port still serves connections (verifies the change does not break existing behavior) + r config set min-reserved-fds 64 + assert_equal 64 [lindex [r config get min-reserved-fds] 1] + set m [redis $redis_host $mport 0 0] + assert_match [$m ping] "PONG" + # Restore + r config set min-reserved-fds 32 + } + + test {CONFIG SET min-reserved-fds huge value rejected without killing server} { + # Key regression (BUG 1): when CONFIG SET min-reserved-fds exceeds current RLIMIT_NOFILE, + # it must return an error and the CONFIG framework must roll back to the previous value; + # the server must NOT be killed by exit(1). + # UINT_MAX (4294967295) is used to trigger the failure path; + # catch + if-else keeps the test stable in the rare case where ulimit is unusually high. + if {[catch {r config set min-reserved-fds 4294967295} err]} { + # Failure path: server must still respond to PING and the previous value must remain + assert_equal PONG [r ping] + assert_equal 32 [lindex [r config get min-reserved-fds] 1] + } else { + # Rare environments where ulimit >= UINT_MAX: configuration succeeds and value must match + assert_equal 4294967295 [lindex [r config get min-reserved-fds] 1] + r config set min-reserved-fds 32 + } + } + } \ No newline at end of file diff --git a/tests/unit/limits.tcl b/tests/unit/limits.tcl index 51122e8f5b3..c4b8894d40a 100644 --- a/tests/unit/limits.tcl +++ b/tests/unit/limits.tcl @@ -18,4 +18,98 @@ start_server {tags {"limits network"} overrides {maxclients 10}} { assert {$c > 8 && $c <= 10} set e } $expected_code + + # === min-reserved-fds boundary tests === + # Purpose: cover input validation and runtime regression points for CONFIG SET min-reserved-fds + # Defaults: default 32, lower_bound 32, upper_bound UINT_MAX + + test {CONFIG SET min-reserved-fds accepts a valid value} { + # Boundary 1: a valid value can be set and CONFIG GET returns the same value + assert_equal OK [r config set min-reserved-fds 64] + assert_equal 64 [lindex [r config get min-reserved-fds] 1] + # Restore to default to avoid polluting subsequent tests + r config set min-reserved-fds 32 + } + + test {CONFIG SET min-reserved-fds rejects value below lower bound (32)} { + # Boundary 2: values below 32 must be rejected and the existing value must remain + assert_error "*inclusive*" {r config set min-reserved-fds 16} + assert_equal 32 [lindex [r config get min-reserved-fds] 1] + } + + test {CONFIG SET min-reserved-fds rejects non-integer value} { + # Boundary 3: non-integer values must be rejected ("argument couldn't be parsed into an integer") + assert_error "*integer*" {r config set min-reserved-fds abc} + } + + test {CONFIG SET min-reserved-fds does not modify maxclients} { + # Boundary 4: changing min-reserved-fds must not silently change maxclients + # Regression point: the original adjustOpenFilesLimit path used to mutate maxclients + set original_maxclients [lindex [r config get maxclients] 1] + r config set min-reserved-fds 64 + assert_equal $original_maxclients [lindex [r config get maxclients] 1] + # Restore + r config set min-reserved-fds 32 + } + + test {CONFIG SET min-reserved-fds shrink succeeds} { + # Boundary 5: grow then shrink; CONFIG stays consistent (does not depend on ae setsize shrinking) + r config set min-reserved-fds 96 + assert_equal OK [r config set min-reserved-fds 32] + assert_equal 32 [lindex [r config get min-reserved-fds] 1] + } + + test {CONFIG SET min-reserved-fds huge value rejected without killing server} { + # Key regression (BUG 1): when CONFIG SET min-reserved-fds exceeds current RLIMIT_NOFILE, + # it must return an error and the CONFIG framework must roll back to the previous value; + # the server must NOT be killed by exit(1). + # UINT_MAX (4294967295) is used to trigger the failure path (far beyond any common ulimit); + # catch + if-else keeps the test stable in the rare case where ulimit is unusually high. + if {[catch {r config set min-reserved-fds 4294967295} err]} { + # Failure path: server must still respond to PING and the previous value must remain + assert_equal PONG [r ping] + assert_equal 32 [lindex [r config get min-reserved-fds] 1] + } else { + # Rare environments where ulimit >= UINT_MAX: configuration succeeds and value must match + assert_equal 4294967295 [lindex [r config get min-reserved-fds] 1] + r config set min-reserved-fds 32 + } + } + + # === maxclients boundary tests === + # Purpose: cover the same risk classes that min-reserved-fds guards against, + # because updateMaxclients and updateMinReservedFds both rely on adjustOpenFilesLimit + # and share tryResizeSetAeSize. + + test {CONFIG SET maxclients huge value rejected without silently reducing maxclients} { + # Regression for the same BUG 1 class as min-reserved-fds: + # adjustOpenFilesLimit may silently shrink server.maxclients when ulimit is too small, + # and (in extreme cases) call exit(1) when bestlimit <= server.min_reserved_fds. + # updateMaxclients guards against silent shrinkage by rolling server.maxclients back to prev, + # and the CONFIG framework also rolls back when update_fn returns 0. + # UINT_MAX (4294967295) triggers the failure path on any common ulimit; + # catch + if-else keeps the test stable in the rare case where ulimit is unusually high. + set original [lindex [r config get maxclients] 1] + if {[catch {r config set maxclients 4294967295} err]} { + # Failure path: maxclients must remain at its previous value + assert_equal $original [lindex [r config get maxclients] 1] + assert_match "*not able to handle*" $err + } else { + # Rare environments where ulimit >= UINT_MAX: configuration succeeds and value must match + assert_equal 4294967295 [lindex [r config get maxclients] 1] + r config set maxclients $original + } + # Server must still be alive + assert_equal PONG [r ping] + } + + test {CONFIG SET maxclients smaller value accepted} { + # val < prev path: updateMaxclients returns 1 without touching adjustOpenFilesLimit + # or tryResizeSetAeSize (the ae array is not shrunk — same behavior as min-reserved-fds shrink). + # Verify CONFIG consistency and that the server still responds. + assert_equal OK [r config set maxclients 5] + assert_equal 5 [lindex [r config get maxclients] 1] + # Restore + r config set maxclients 10 + } }