Fix slot-migration-max-failover-repl-bytes unable to accept -1

src/cluster_migrateslots.c

@@ -1448,8 +1448,8 @@ int slotExportTryDoPause(slotMigrationJob *job) {
         return C_ERR;
     }
     serverLog(LL_NOTICE,
-              "Pausing writes to allow slot migration %s to finalize failover.",
-              job->description);
+              "Pausing writes (remaining_repl_size is %lld) to allow slot migration %s to finalize failover.",
+              job->client->reply_bytes, job->description);
     job->mf_end = mstime() + server.cluster_mf_timeout * CLUSTER_MF_PAUSE_MULT;
     pauseActions(PAUSE_DURING_SLOT_MIGRATION, job->mf_end,
                  PAUSE_ACTIONS_CLIENT_WRITE_SET);

src/config.c

@@ -2168,6 +2168,13 @@ static int numericParseString(standardConfig *config, sds value, const char **er
         int memerr;
         *res = memtoull(value, &memerr);
         if (!memerr) return 1;
+
+        /* memtoull rejects negative values, but some memory configs accept
+         * special negative values (e.g. -1 to disable a limit). Fall back
+         * to plain integer parsing for those. */
+        if (config->data.numeric.flags & SIGNED_MEMORY_CONFIG) {
+            if (string2ll(value, sdslen(value), res)) return 1;
+        }
     }
 
     /* Attempt to parse as percent */
@@ -2241,6 +2248,8 @@ static sds numericConfigGet(standardConfig *config) {
         int len = ll2string(buf, sizeof(buf), -value);
         buf[len] = '%';
         buf[len + 1] = '\0';
+    } else if (config->data.numeric.flags & SIGNED_MEMORY_CONFIG && value < 0) {
+        ll2string(buf, sizeof(buf), value);
     } else if (config->data.numeric.flags & MEMORY_CONFIG) {
         ull2string(buf, sizeof(buf), value);
     } else if (config->data.numeric.flags & OCTAL_CONFIG) {
@@ -2260,6 +2269,8 @@ static void numericConfigRewrite(standardConfig *config, const char *name, struc
 
     if (config->data.numeric.flags & PERCENT_CONFIG && value < 0) {
         rewriteConfigPercentOption(state, name, -value, config->data.numeric.default_value);
+    } else if (config->data.numeric.flags & SIGNED_MEMORY_CONFIG && value < 0) {
+        rewriteConfigNumericalOption(state, name, value, config->data.numeric.default_value);
     } else if (config->data.numeric.flags & MEMORY_CONFIG) {
         rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value);
     } else if (config->data.numeric.flags & OCTAL_CONFIG) {
@@ -3452,7 +3463,7 @@ standardConfig static_configs[] = {
     createSizeTConfig("tracking-table-max-keys", NULL, MODIFIABLE_CONFIG, 0, LONG_MAX, server.tracking_table_max_keys, 1000000, INTEGER_CONFIG, NULL, NULL),                                      /* Default: 1 million keys max. */
     createSizeTConfig("client-query-buffer-limit", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, 1024 * 1024, LONG_MAX, server.client_max_querybuf_len, 1024 * 1024 * 1024, MEMORY_CONFIG, NULL, NULL), /* Default: 1GB max query buffer. */
     createSSizeTConfig("maxmemory-clients", NULL, MODIFIABLE_CONFIG, -100, SSIZE_MAX, server.maxmemory_clients, 0, MEMORY_CONFIG | PERCENT_CONFIG, NULL, applyClientMaxMemoryUsage),
-    createSSizeTConfig("slot-migration-max-failover-repl-bytes", NULL, MODIFIABLE_CONFIG, -1, SSIZE_MAX, server.slot_migration_max_failover_repl_bytes, 0, MEMORY_CONFIG, NULL, NULL),
+    createSSizeTConfig("slot-migration-max-failover-repl-bytes", NULL, MODIFIABLE_CONFIG, -1, SSIZE_MAX, server.slot_migration_max_failover_repl_bytes, 0, MEMORY_CONFIG | SIGNED_MEMORY_CONFIG, NULL, NULL),
 
     /* Other configs */
     createTimeTConfig("repl-backlog-ttl", NULL, MODIFIABLE_CONFIG, 0, LONG_MAX, server.repl_backlog_time_limit, 60 * 60, INTEGER_CONFIG, NULL, NULL), /* Default: 1 hour */
@@ -3519,6 +3530,18 @@ void initConfigValues(void) {
     dictExpand(configs, sizeof(static_configs) / sizeof(standardConfig));
     for (standardConfig *config = static_configs; config->name != NULL; config++) {
         if (config->interface.init) config->interface.init(config);
+
+        if (config->type == NUMERIC_CONFIG) {
+            /* SIGNED_MEMORY_CONFIG must be used together with MEMORY_CONFIG. */
+            serverAssert(!((config->data.numeric.flags & SIGNED_MEMORY_CONFIG) &&
+                           !(config->data.numeric.flags & MEMORY_CONFIG)));
+
+            /* PERCENT_CONFIG and SIGNED_MEMORY_CONFIG both use negative values
+             * with different semantics, so they must not be combined. */
+            serverAssert(!((config->data.numeric.flags & PERCENT_CONFIG) &&
+                           (config->data.numeric.flags & SIGNED_MEMORY_CONFIG)));
+        }
+
         /* Add the primary config to the dictionary. */
         int ret = registerConfigValue(config->name, config, 0);
         serverAssert(ret);

src/server.h

@@ -3640,11 +3640,13 @@ sds keyspaceEventsFlagsToString(int flags);
                                          * to apply the configuration change even if the new config value is the same as    \
                                          * the old. */
 
-#define INTEGER_CONFIG 0         /* No flags means a simple integer configuration */
-#define MEMORY_CONFIG (1 << 0)   /* Indicates if this value can be loaded as a memory value */
-#define PERCENT_CONFIG (1 << 1)  /* Indicates if this value can be loaded as a percent (and stored as a negative int) */
-#define OCTAL_CONFIG (1 << 2)    /* This value uses octal representation */
-#define UNSIGNED_CONFIG (1 << 3) /* This value uses unsigned representation */
+/* Numeric Flags */
+#define INTEGER_CONFIG 0              /* No flags means a simple integer configuration */
+#define MEMORY_CONFIG (1 << 0)        /* Indicates if this value can be loaded as a memory value */
+#define PERCENT_CONFIG (1 << 1)       /* Indicates if this value can be loaded as a percent (and stored as a negative int) */
+#define OCTAL_CONFIG (1 << 2)         /* This value uses octal representation */
+#define UNSIGNED_CONFIG (1 << 3)      /* This value uses unsigned representation */
+#define SIGNED_MEMORY_CONFIG (1 << 4) /* A MEMORY_CONFIG that also accepts plain negative integers */
 
 /* Enum Configs contain an array of configEnum objects that match a string with an integer. */
 typedef struct configEnum {

tests/unit/cluster/cluster-migrateslots.tcl

@@ -2450,7 +2450,7 @@ start_cluster 3 0 {tags {logreqres:skip external:skip cluster} overrides {cluste
         set 16383_slot_tag "{6ZJ}"
         set_debug_prevent_pause 1
 
-        # Move 16383 from R0 to R2
+        # Move 16383 from R2 to R0.
         assert_match "OK" [R 2 CLUSTER MIGRATESLOTS SLOTSRANGE 16383 16383 NODE [R 0 CLUSTER MYID]]
         set jobname [get_job_name 2 16383]
         wait_for_migration_field 2 $jobname state waiting-to-pause
@@ -2482,5 +2482,48 @@ start_cluster 3 0 {tags {logreqres:skip external:skip cluster} overrides {cluste
         set export_migration [get_migration_by_name 2 $jobname]
         assert_equal [dict get $export_migration remaining_repl_size] 0
         assert_equal [dict get $import_migration remaining_repl_size] 0
+
+        # Check the remaining_repl_size log is OK.
+        verify_log_message -2 "*Pausing writes (remaining_repl_size is 0) to allow slot migration*" 0
+    }
+}
+
+start_cluster 3 0 {tags {logreqres:skip external:skip cluster} overrides {cluster-require-full-coverage no slot-migration-max-failover-repl-bytes -1}} {
+    test "slot-migration-max-failover-repl-bytes -1 disables repl bytes limit" {
+        set 16383_slot_tag "{6ZJ}"
+
+        # Use PREVENT-PAUSE to hold the migration at the waiting-to-pause
+        # state so we can fill the replication buffer first.
+        R 2 DEBUG SLOTMIGRATION PREVENT-PAUSE 1
+
+        # Move slot 16383 from R2 to R0.
+        assert_match "OK" [R 2 CLUSTER MIGRATESLOTS SLOTSRANGE 16383 16383 NODE [R 0 CLUSTER MYID]]
+        set jobname [get_job_name 2 16383]
+        wait_for_migration_field 2 $jobname state waiting-to-pause
+
+        # Pause the target (R0) process and generate data to fill the
+        # replication buffer on the source (R2).
+        pause_process [srv 0 pid]
+        set bigstr [string repeat x 1024000]
+        for {set j 0} {$j < 50} {incr j} {
+            R 2 set "$16383_slot_tag:key:$j" $bigstr
+        }
+
+        # Confirm the replication buffer has accumulated data.
+        set export_migration [get_migration_by_name 2 $jobname]
+        set remaining_repl_size [dict get $export_migration remaining_repl_size]
+        assert_morethan $remaining_repl_size [expr 1024000 * 25]
+
+        # Resume the PREVENT-PAUSE.
+        # With slot-migration-max-failover-repl-bytes -1, the source (R2) should proceed
+        # to pause writes regardless of the large remaining_repl_size.
+        R 2 DEBUG SLOTMIGRATION PREVENT-PAUSE 0
+        wait_for_log_messages -2 {"*Pausing writes*"} 0 1000 10
+        set pattern "*Pausing writes (remaining_repl_size is $remaining_repl_size) to allow slot migration*"
+        verify_log_message -2 $pattern 0
+
+        # Resume R0 and wait for R0 to finish the migration.
+        resume_process [srv 0 pid]
+        wait_for_migration 0 16383
     }
 }

tests/unit/introspection.tcl

@@ -1964,10 +1964,22 @@ test {CONFIG REWRITE handles alias config properly} {
     }
 } {} {external:skip}
 
+test {SIGNED MEMORY CONFIG allows negative number} {
+    start_server {tags {"introspection"}} {
+        r config set slot-migration-max-failover-repl-bytes -1
+        assert_equal [lindex [r config get slot-migration-max-failover-repl-bytes] 1] -1
+        assert_error {*argument must be between -1 and *} {r config set slot-migration-max-failover-repl-bytes -2}
+
+        r config rewrite
+        restart_server 0 true false
+        assert_equal [lindex [r config get slot-migration-max-failover-repl-bytes] 1] -1
+    }
+} {} {external:skip}
+
 test {CONFIG hash-seed is immutable and settable at startup} {
     start_server {tags {"introspection"} overrides {hash-seed aabbccddeeffgghh}} {
         assert_error "ERR CONFIG SET failed (possibly related to argument 'hash-seed') - can't set immutable config*" {
             r config set hash-seed newseed
         }
     }
-} {} {external:skip}
+} {} {external:skip}