Unverified Commit 41e6e05d authored by Oran Agra's avatar Oran Agra Committed by GitHub
Browse files

Allow most CONFIG SET during loading, block some commands in async-loading (#9878)

## background
Till now CONFIG SET was blocked during loading.
(In the not so distant past, GET was disallowed too)

We recently (not released yet) added an async-loading mode, see #9323,
and during that time it'll serve CONFIG SET and any other command.
And now we realized (#9770) that some configs, and commands are dangerous
during async-loading.

## changes
* Allow most CONFIG SET during loading (both on async-loading and normal loading)
* Allow CONFIG REWRITE and CONFIG RESETSTAT during loading
* Block a few config during loading (`appendonly`, `repl-diskless-load`, and `dir`)
* Block a few commands during loading (list below)

## the blocked commands:
* SAVE - obviously we don't wanna start a foregreound save during loading 8-)
* BGSAVE - we don't mind to schedule one, but we don't wanna fork now
* BGREWRITEAOF - we don't mind to schedule one, but we don't wanna fork now
* MODULE - we obviously don't wanna unload a module during replication / rdb loading
  (MODULE HELP and MODULE LIST are not blocked)
* SYNC / PSYNC - we're in the middle of RDB loading from master, must not allow sync
  requests now.
* REPLICAOF / SLAVEOF - we're in the middle of replicating, maybe it makes sense to let
  the user abort it, but he couldn't do that so far, i don't wanna take any risk of bugs due to odd state.
* CLUSTER - only allow [HELP, SLOTS, NODES, INFO, MYID, LINKS, KEYSLOT, COUNTKEYSINSLOT,
  GETKEYSINSLOT, RESET, REPLICAS, COUNT_FAILURE_REPORTS], for others, preserve the status quo

## other fixes
* processEventsWhileBlocked had an issue when being nested, this could happen with a busy script
  during async loading (new), but also in a busy script during AOF loading (old). this lead to a crash in
  the scenario described in #6988
parent ad55fbaa
......@@ -8,6 +8,7 @@
"container": "MODULE",
"function": "moduleCommand",
"command_flags": [
"NO_ASYNC_LOADING",
"ADMIN",
"NOSCRIPT",
"PROTECTED"
......
......@@ -6,6 +6,7 @@
"arity": -3,
"function": "syncCommand",
"command_flags": [
"NO_ASYNC_LOADING",
"ADMIN",
"NOSCRIPT"
],
......
......@@ -7,6 +7,7 @@
"arity": 3,
"function": "replicaofCommand",
"command_flags": [
"NO_ASYNC_LOADING",
"ADMIN",
"NOSCRIPT",
"STALE"
......
......@@ -7,6 +7,7 @@
"arity": 1,
"function": "saveCommand",
"command_flags": [
"NO_ASYNC_LOADING",
"ADMIN",
"NOSCRIPT"
]
......
......@@ -7,6 +7,7 @@
"arity": 3,
"function": "replicaofCommand",
"command_flags": [
"NO_ASYNC_LOADING",
"ADMIN",
"NOSCRIPT",
"STALE"
......
......@@ -6,6 +6,7 @@
"arity": 1,
"function": "syncCommand",
"command_flags": [
"NO_ASYNC_LOADING",
"ADMIN",
"NOSCRIPT"
]
......
......@@ -269,6 +269,7 @@ typedef struct standardConfig {
#define MULTI_ARG_CONFIG (1ULL<<3) /* This config receives multiple arguments. */
#define HIDDEN_CONFIG (1ULL<<4) /* This config is hidden in `config get <pattern>` (used for tests/debugging) */
#define PROTECTED_CONFIG (1ULL<<5) /* Becomes immutable if enable-protected-configs is enabled. */
#define DENY_LOADING_CONFIG (1ULL<<6) /* This config is forbidden during loading. */
standardConfig configs[];
......@@ -710,7 +711,7 @@ void configSetCommand(client *c) {
sds *old_values = NULL;
apply_fn *apply_fns; /* TODO: make this a set for better performance */
int config_count, i, j;
int invalid_args = 0;
int invalid_args = 0, deny_loading_error = 0;
int *config_map_fns;
/* Make sure we have an even number of arguments: conf-val pairs */
......@@ -749,6 +750,12 @@ void configSetCommand(client *c) {
invalid_args = 1;
}
if (server.loading && config->flags & DENY_LOADING_CONFIG) {
/* Note: we don't abort the loop since we still want to handle redacting sensitive configs (above) */
deny_loading_error = 1;
invalid_args = 1;
}
/* If this config appears twice then fail */
for (j = 0; j < i; j++) {
if (set_configs[j] == config) {
......@@ -819,7 +826,10 @@ void configSetCommand(client *c) {
goto end;
err:
if (invalid_arg_name) {
if (deny_loading_error) {
/* We give the loading error precedence because it may be handled by clients differently, unlike a plain -ERR. */
addReplyErrorObject(c,shared.loadingerr);
} else if (invalid_arg_name) {
addReplyErrorFormat(c,"Unknown option or number of arguments for CONFIG SET - '%s'", invalid_arg_name);
} else if (errstr) {
addReplyErrorFormat(c,"CONFIG SET failed (possibly related to argument '%s') - %s", err_arg_name, errstr);
......@@ -2624,7 +2634,7 @@ standardConfig configs[] = {
createBoolConfig("activedefrag", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, server.active_defrag_enabled, 0, isValidActiveDefrag, NULL),
createBoolConfig("syslog-enabled", NULL, IMMUTABLE_CONFIG, server.syslog_enabled, 0, NULL, NULL),
createBoolConfig("cluster-enabled", NULL, IMMUTABLE_CONFIG, server.cluster_enabled, 0, NULL, NULL),
createBoolConfig("appendonly", NULL, MODIFIABLE_CONFIG, server.aof_enabled, 0, NULL, updateAppendonly),
createBoolConfig("appendonly", NULL, MODIFIABLE_CONFIG | DENY_LOADING_CONFIG, server.aof_enabled, 0, NULL, updateAppendonly),
createBoolConfig("cluster-allow-reads-when-down", NULL, MODIFIABLE_CONFIG, server.cluster_allow_reads_when_down, 0, NULL, NULL),
createBoolConfig("crash-log-enabled", NULL, MODIFIABLE_CONFIG, server.crashlog_enabled, 1, NULL, updateSighandlerEnabled),
createBoolConfig("crash-memcheck-enabled", NULL, MODIFIABLE_CONFIG, server.memcheck_enabled, 1, NULL, NULL),
......@@ -2660,7 +2670,7 @@ standardConfig configs[] = {
/* Enum Configs */
createEnumConfig("supervised", NULL, IMMUTABLE_CONFIG, supervised_mode_enum, server.supervised_mode, SUPERVISED_NONE, NULL, NULL),
createEnumConfig("syslog-facility", NULL, IMMUTABLE_CONFIG, syslog_facility_enum, server.syslog_facility, LOG_LOCAL0, NULL, NULL),
createEnumConfig("repl-diskless-load", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, repl_diskless_load_enum, server.repl_diskless_load, REPL_DISKLESS_LOAD_DISABLED, NULL, NULL),
createEnumConfig("repl-diskless-load", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG | DENY_LOADING_CONFIG, repl_diskless_load_enum, server.repl_diskless_load, REPL_DISKLESS_LOAD_DISABLED, NULL, NULL),
createEnumConfig("loglevel", NULL, MODIFIABLE_CONFIG, loglevel_enum, server.verbosity, LL_NOTICE, NULL, NULL),
createEnumConfig("maxmemory-policy", NULL, MODIFIABLE_CONFIG, maxmemory_policy_enum, server.maxmemory_policy, MAXMEMORY_NO_EVICTION, NULL, NULL),
createEnumConfig("appendfsync", NULL, MODIFIABLE_CONFIG, aof_fsync_enum, server.aof_fsync, AOF_FSYNC_EVERYSEC, NULL, NULL),
......@@ -2772,7 +2782,7 @@ standardConfig configs[] = {
#endif
/* Special configs */
createSpecialConfig("dir", NULL, MODIFIABLE_CONFIG | PROTECTED_CONFIG, setConfigDirOption, getConfigDirOption, rewriteConfigDirOption, NULL),
createSpecialConfig("dir", NULL, MODIFIABLE_CONFIG | PROTECTED_CONFIG | DENY_LOADING_CONFIG, setConfigDirOption, getConfigDirOption, rewriteConfigDirOption, NULL),
createSpecialConfig("save", NULL, MODIFIABLE_CONFIG | MULTI_ARG_CONFIG, setConfigSaveOption, getConfigSaveOption, rewriteConfigSaveOption, NULL),
createSpecialConfig("client-output-buffer-limit", NULL, MODIFIABLE_CONFIG | MULTI_ARG_CONFIG, setConfigClientOutputBufferLimitOption, getConfigClientOutputBufferLimitOption, rewriteConfigClientOutputBufferLimitOption, NULL),
createSpecialConfig("oom-score-adj-values", NULL, MODIFIABLE_CONFIG | MULTI_ARG_CONFIG, setConfigOOMScoreAdjValuesOption, getConfigOOMScoreAdjValuesOption, rewriteConfigOOMScoreAdjValuesOption, updateOOMScoreAdj),
......
......@@ -3625,8 +3625,11 @@ void processEventsWhileBlocked(void) {
/* Note: when we are processing events while blocked (for instance during
* busy Lua scripts), we set a global flag. When such flag is set, we
* avoid handling the read part of clients using threaded I/O.
* See https://github.com/redis/redis/issues/6988 for more info. */
ProcessingEventsWhileBlocked = 1;
* See https://github.com/redis/redis/issues/6988 for more info.
* Note that there could be cases of nested calls to this function,
* specifically on a busy script during async_loading rdb, and scripts
* that came from AOF. */
ProcessingEventsWhileBlocked++;
while (iterations--) {
long long startval = server.events_processed_while_blocked;
long long ae_events = aeProcessEvents(server.el,
......@@ -3641,7 +3644,8 @@ void processEventsWhileBlocked(void) {
whileBlockedCron();
ProcessingEventsWhileBlocked = 0;
ProcessingEventsWhileBlocked--;
serverAssert(ProcessingEventsWhileBlocked >= 0);
}
/* ==========================================================================
......
......@@ -3236,6 +3236,8 @@ int processCommand(client *c) {
(c->cmd->proc == execCommand && (c->mstate.cmd_inv_flags & CMD_LOADING));
int is_may_replicate_command = (c->cmd->flags & (CMD_WRITE | CMD_MAY_REPLICATE)) ||
(c->cmd->proc == execCommand && (c->mstate.cmd_flags & (CMD_WRITE | CMD_MAY_REPLICATE)));
int is_deny_async_loading_command = (c->cmd->flags & CMD_NO_ASYNC_LOADING) ||
(c->cmd->proc == execCommand && (c->mstate.cmd_flags & CMD_NO_ASYNC_LOADING));
if (authRequired(c)) {
/* AUTH and HELLO and no auth commands are valid even in
......@@ -3434,6 +3436,12 @@ int processCommand(client *c) {
return C_OK;
}
/* During async-loading, block certain commands. */
if (server.async_loading && is_deny_async_loading_command) {
rejectCommand(c,shared.loadingerr);
return C_OK;
}
/* Lua script too slow? Only allow a limited number of commands.
* Note that we need to allow the transactions commands, otherwise clients
* sending a transaction with pipelining without error checking, may have
......
......@@ -204,9 +204,9 @@ extern int configOOMScoreAdjValuesDefaults[CONFIG_OOM_COUNT];
#define CMD_ONLY_SENTINEL (1ULL<<18)
#define CMD_NO_MANDATORY_KEYS (1ULL<<19)
#define CMD_PROTECTED (1ULL<<20)
/* Command flags used by the module system. */
#define CMD_MODULE_GETKEYS (1ULL<<21) /* Use the modules getkeys interface. */
#define CMD_MODULE_NO_CLUSTER (1ULL<<22) /* Deny on Redis Cluster. */
#define CMD_NO_ASYNC_LOADING (1ULL<<23)
/* Command flags that describe ACLs categories. */
#define ACL_CATEGORY_KEYSPACE (1ULL<<0)
......@@ -2008,6 +2008,9 @@ typedef int redisGetKeysProc(struct redisCommand *cmd, robj **argv, int argc, ge
*
* CMD_LOADING: Allow the command while loading the database.
*
* CMD_NO_ASYNC_LOADING: Deny during async loading (when a replica uses diskless
sync swapdb, and allows access to the old dataset)
*
* CMD_STALE: Allow the command while a slave has stale data but is not
* allowed to serve this data. Normally no command is accepted
* in this condition but just a few.
......
......@@ -568,6 +568,23 @@ foreach testType {Successful Aborted} {
assert_equal [$replica dbsize] 2001
}
test {Busy script during async loading} {
set rd_replica [redis_deferring_client -1]
$replica config set lua-time-limit 10
$rd_replica eval {while true do end} 0
after 200
assert_error {BUSY*} {$replica ping}
$replica script kill
after 200 ; # Give some time to Lua to call the hook again...
assert_equal [$replica ping] "PONG"
$rd_replica close
}
test {Blocked commands and configs during async-loading} {
assert_error {LOADING*} {$replica config set appendonly no}
assert_error {LOADING*} {$replica REPLICAOF no one}
}
# Make sure that next sync will not start immediately so that we can catch the replica in between syncs
$master config set repl-diskless-sync-delay 5
......
......@@ -461,3 +461,30 @@ start_server {config "minimal.conf" tags {"introspection external:skip"} overrid
}
} {} {needs:debug}
}
test {config during loading} {
start_server [list overrides [list key-load-delay 50 rdbcompression no]] {
# create a big rdb that will take long to load. it is important
# for keys to be big since the server processes events only once in 2mb.
# 100mb of rdb, 100k keys will load in more than 5 seconds
r debug populate 100000 key 1000
restart_server 0 false false
# make sure it's still loading
assert_equal [s loading] 1
# verify some configs are allowed during loading
r config set loglevel debug
assert_equal [lindex [r config get loglevel] 1] debug
# verify some configs are forbidden during loading
assert_error {LOADING*} {r config set dir asdf}
# make sure it's still loading
assert_equal [s loading] 1
# no need to keep waiting for loading to complete
exec kill [srv 0 pid]
}
} {} {external:skip}
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment