PublicArchive/valkey
2
0
Fork 0
mirror of https://github.com/valkey-io/valkey.git synced 2026-05-06 05:26:42 -04:00
Code Issues Packages Projects Releases Wiki Activity

Fix slot-migration-max-failover-repl-bytes unable to accept -1 (#3443)

Browse Source 
In valkey.conf, slot-migration-max-failover-repl-bytes allows setting
to -1 to disable the limit.
```
Setting this to -1 will disable this limit
```

But slot-migration-max-failover-repl-bytes is defined as MEMORY_CONFIG
and memtoull() rejects negative inputs, making it impossible to set the
value to -1 via config file or CONFIG SET.
```
>>> 'slot-migration-max-failover-repl-bytes "-1"'
argument must be a memory value
```

Introduce SIGNED_MEMORY_CONFIG flag for memory configs that also accept
plain negative number. When memtoull() fails and this flag is set, fall back to string2ll()
for parsing. Use ll2string() for CONFIG GET and rewriteConfigNumericalOption() for
CONFIG REWRITE when the value is negative.

Add a serverAssert in initConfigValues() to enforce that PERCENT_CONFIG
and SIGNED_MEMORY_CONFIG are never combined on the same config, since
both use negative values with different semantics.

This means we have had this issue since it was introduced in #1949.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
This commit is contained in:
Binbin
2026-04-08 16:26:29 +08:00
committed by GitHub
parent e6bbd4e0d2
commit dba128f13b
5 changed files with 90 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/cluster_migrateslots.c
+2 -2
View File
@@ -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
+24 -1
View File
@@ -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
+7 -5
View File
@@ -3599,11 +3599,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
+44 -1
View File
@@ -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
+13 -1
View File
@@ -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}