Unverified Commit 0e5b813e authored by yoav-steinberg's avatar yoav-steinberg Committed by GitHub
Browse files

Multiparam config set (#9748)

We can now do: `config set maxmemory 10m repl-backlog-size 5m`

## Basic algorithm to support "transaction like" config sets:

1. Backup all relevant current values (via get).
2. Run "verify" and "set" on everything, if we fail run "restore".
3. Run "apply" on everything (optional optimization: skip functions already run). If we fail run "restore".
4. Return success.

### restore
1. Run set on everything in backup. If we fail log it and continue (this puts us in an undefined
   state but we decided it's better than the alternative of panicking). This indicates either a bug
   or some unsupported external state.
2. Run apply on everything in backup (optimization: skip functions already run). If we fail log
   it (see comment above).
3. Return error.

## Implementation/design changes:
* Apply function are idempotent (have no effect if they are run more than once for the same config).
* No indication in set functions if we're reading the config or running from the `CONFIG SET` command
   (removed `update` argument).
* Set function should set some config variable and assume an (optional) apply function will use that
   later to apply. If we know this setting can be safely applied immediately and can always be reverted
   and doesn't depend on any other configuration we can apply immediately from within the set function
   (and not store the setting anywhere). This is the case of this `dir` config, for example, which has no
   apply function. No apply function is need also in the case that setting the variable in the `server` struct
   is all that needs to be done to make the configuration take effect. Note that the original concept of `update_fn`,
   which received the old and new values was removed and replaced by the optional apply function.
* Apply functions use settings written to the `server` struct and don't receive any inputs.
* I take care that for the generic (non-special) configs if there's no change I avoid calling the setter (possible
   optimization: avoid calling the apply function as well).
* Passing the same config parameter more than once to `config set` will fail. You can't do `config set my-setting
   value1 my-setting value2`.

Note that getting `save` in the context of the conf file parsing to work here as before was a pain.
The conf file supports an aggregate `save` definition, where each `save` line is added to the server's
save params. This is unlike any other line in the config file where each line overwrites any previous
configuration. Since we now support passing multiple save params in a single line (see top comments
about `save` in https://github.com/redis/redis/pull/9644) we should deprecate the aggregate nature of
this config line and perhaps reduce this ugly code in the future.
parent 21aa1d4b
...@@ -378,10 +378,10 @@ proc-title-template "{title} {listen-addr} {server-mode}" ...@@ -378,10 +378,10 @@ proc-title-template "{title} {listen-addr} {server-mode}"
# Save the DB to disk. # Save the DB to disk.
# #
# save <seconds> <changes> # save <seconds> <changes> [<seconds> <changes> ...]
# #
# Redis will save the DB if both the given number of seconds and the given # Redis will save the DB if the given number of seconds elapsed and it
# number of write operations against the DB occurred. # surpassed the given number of write operations against the DB.
# #
# Snapshotting can be completely disabled with a single empty string argument # Snapshotting can be completely disabled with a single empty string argument
# as in following example: # as in following example:
...@@ -393,11 +393,9 @@ proc-title-template "{title} {listen-addr} {server-mode}" ...@@ -393,11 +393,9 @@ proc-title-template "{title} {listen-addr} {server-mode}"
# * After 300 seconds (5 minutes) if at least 100 keys changed # * After 300 seconds (5 minutes) if at least 100 keys changed
# * After 60 seconds if at least 10000 keys changed # * After 60 seconds if at least 10000 keys changed
# #
# You can set these explicitly by uncommenting the three following lines. # You can set these explicitly by uncommenting the following line.
# #
# save 3600 1 # save 3600 1 300 100 60 10000
# save 300 100
# save 60 10000
# By default Redis will stop accepting writes if RDB snapshots are enabled # By default Redis will stop accepting writes if RDB snapshots are enabled
# (at least one save point) and the latest background save failed. # (at least one save point) and the latest background save failed.
......
This diff is collapsed.
...@@ -122,17 +122,13 @@ void createReplicationBacklog(void) { ...@@ -122,17 +122,13 @@ void createReplicationBacklog(void) {
} }
/* This function is called when the user modifies the replication backlog /* This function is called when the user modifies the replication backlog
* size at runtime. It is up to the function to both update the * size at runtime. It is up to the function to resize the buffer and setup it
* server.repl_backlog_size and to resize the buffer and setup it so that * so that it contains the same data as the previous one (possibly less data,
* it contains the same data as the previous one (possibly less data, but * but the most recent bytes, or the same data and more free space in case the
* the most recent bytes, or the same data and more free space in case the
* buffer is enlarged). */ * buffer is enlarged). */
void resizeReplicationBacklog(long long newsize) { void resizeReplicationBacklog(void) {
if (newsize < CONFIG_REPL_BACKLOG_MIN_SIZE) if (server.repl_backlog_size < CONFIG_REPL_BACKLOG_MIN_SIZE)
newsize = CONFIG_REPL_BACKLOG_MIN_SIZE; server.repl_backlog_size = CONFIG_REPL_BACKLOG_MIN_SIZE;
if (server.repl_backlog_size == newsize) return;
server.repl_backlog_size = newsize;
if (server.repl_backlog) if (server.repl_backlog)
incrementalTrimReplicationBacklog(REPL_BACKLOG_TRIM_BLOCKS_PER_CALL); incrementalTrimReplicationBacklog(REPL_BACKLOG_TRIM_BLOCKS_PER_CALL);
} }
......
...@@ -200,7 +200,7 @@ struct redisServer server; /* Server global state */ ...@@ -200,7 +200,7 @@ struct redisServer server; /* Server global state */
*/ */
struct redisCommand configSubcommands[] = { struct redisCommand configSubcommands[] = {
{"set",configSetCommand,4, {"set",configSetCommand,-4,
"admin ok-stale no-script"}, "admin ok-stale no-script"},
{"get",configGetCommand,3, {"get",configGetCommand,3,
...@@ -3838,8 +3838,6 @@ static void readOOMScoreAdj(void) { ...@@ -3838,8 +3838,6 @@ static void readOOMScoreAdj(void) {
* depending on current role. * depending on current role.
*/ */
int setOOMScoreAdj(int process_class) { int setOOMScoreAdj(int process_class) {
if (server.oom_score_adj == OOM_SCORE_ADJ_NO) return C_OK;
if (process_class == -1) if (process_class == -1)
process_class = (server.masterhost ? CONFIG_OOM_REPLICA : CONFIG_OOM_MASTER); process_class = (server.masterhost ? CONFIG_OOM_REPLICA : CONFIG_OOM_MASTER);
...@@ -3850,11 +3848,14 @@ int setOOMScoreAdj(int process_class) { ...@@ -3850,11 +3848,14 @@ int setOOMScoreAdj(int process_class) {
int val; int val;
char buf[64]; char buf[64];
val = server.oom_score_adj_values[process_class]; if (server.oom_score_adj != OOM_SCORE_ADJ_NO) {
if (server.oom_score_adj == OOM_SCORE_RELATIVE) val = server.oom_score_adj_values[process_class];
val += server.oom_score_adj_base; if (server.oom_score_adj == OOM_SCORE_RELATIVE)
if (val > 1000) val = 1000; val += server.oom_score_adj_base;
if (val < -1000) val = -1000; if (val > 1000) val = 1000;
if (val < -1000) val = -1000;
} else
val = server.oom_score_adj_base;
snprintf(buf, sizeof(buf) - 1, "%d\n", val); snprintf(buf, sizeof(buf) - 1, "%d\n", val);
...@@ -7190,57 +7191,19 @@ void redisAsciiArt(void) { ...@@ -7190,57 +7191,19 @@ void redisAsciiArt(void) {
zfree(buf); zfree(buf);
} }
int changeBindAddr(sds *addrlist, int addrlist_len) { int changeBindAddr(void) {
int i;
int result = C_OK;
char *prev_bindaddr[CONFIG_BINDADDR_MAX];
int prev_bindaddr_count;
/* Close old TCP and TLS servers */ /* Close old TCP and TLS servers */
closeSocketListeners(&server.ipfd); closeSocketListeners(&server.ipfd);
closeSocketListeners(&server.tlsfd); closeSocketListeners(&server.tlsfd);
/* Keep previous settings */
prev_bindaddr_count = server.bindaddr_count;
memcpy(prev_bindaddr, server.bindaddr, sizeof(server.bindaddr));
/* Copy new settings */
memset(server.bindaddr, 0, sizeof(server.bindaddr));
for (i = 0; i < addrlist_len; i++) {
server.bindaddr[i] = zstrdup(addrlist[i]);
}
server.bindaddr_count = addrlist_len;
/* Bind to the new port */ /* Bind to the new port */
if ((server.port != 0 && listenToPort(server.port, &server.ipfd) != C_OK) || if ((server.port != 0 && listenToPort(server.port, &server.ipfd) != C_OK) ||
(server.tls_port != 0 && listenToPort(server.tls_port, &server.tlsfd) != C_OK)) { (server.tls_port != 0 && listenToPort(server.tls_port, &server.tlsfd) != C_OK)) {
serverLog(LL_WARNING, "Failed to bind, trying to restore old listening sockets."); serverLog(LL_WARNING, "Failed to bind");
/* Restore old bind addresses */
for (i = 0; i < addrlist_len; i++) {
zfree(server.bindaddr[i]);
}
memcpy(server.bindaddr, prev_bindaddr, sizeof(server.bindaddr));
server.bindaddr_count = prev_bindaddr_count;
/* Re-Listen TCP and TLS */
server.ipfd.count = 0;
if (server.port != 0 && listenToPort(server.port, &server.ipfd) != C_OK) {
serverPanic("Failed to restore old listening TCP socket.");
}
server.tlsfd.count = 0; closeSocketListeners(&server.ipfd);
if (server.tls_port != 0 && listenToPort(server.tls_port, &server.tlsfd) != C_OK) { closeSocketListeners(&server.tlsfd);
serverPanic("Failed to restore old listening TLS socket."); return C_ERR;
}
result = C_ERR;
} else {
/* Free old bind addresses */
for (i = 0; i < prev_bindaddr_count; i++) {
zfree(prev_bindaddr[i]);
}
} }
/* Create TCP and TLS event handlers */ /* Create TCP and TLS event handlers */
...@@ -7253,15 +7216,17 @@ int changeBindAddr(sds *addrlist, int addrlist_len) { ...@@ -7253,15 +7216,17 @@ int changeBindAddr(sds *addrlist, int addrlist_len) {
if (server.set_proc_title) redisSetProcTitle(NULL); if (server.set_proc_title) redisSetProcTitle(NULL);
return result; return C_OK;
} }
int changeListenPort(int port, socketFds *sfd, aeFileProc *accept_handler) { int changeListenPort(int port, socketFds *sfd, aeFileProc *accept_handler) {
socketFds new_sfd = {{0}}; socketFds new_sfd = {{0}};
/* Close old servers */
closeSocketListeners(sfd);
/* Just close the server if port disabled */ /* Just close the server if port disabled */
if (port == 0) { if (port == 0) {
closeSocketListeners(sfd);
if (server.set_proc_title) redisSetProcTitle(NULL); if (server.set_proc_title) redisSetProcTitle(NULL);
return C_OK; return C_OK;
} }
...@@ -7277,9 +7242,6 @@ int changeListenPort(int port, socketFds *sfd, aeFileProc *accept_handler) { ...@@ -7277,9 +7242,6 @@ int changeListenPort(int port, socketFds *sfd, aeFileProc *accept_handler) {
return C_ERR; return C_ERR;
} }
/* Close old servers */
closeSocketListeners(sfd);
/* Copy new descriptors */ /* Copy new descriptors */
sfd->count = new_sfd.count; sfd->count = new_sfd.count;
memcpy(sfd->fd, new_sfd.fd, sizeof(new_sfd.fd)); memcpy(sfd->fd, new_sfd.fd, sizeof(new_sfd.fd));
......
...@@ -2302,7 +2302,7 @@ void replicationCron(void); ...@@ -2302,7 +2302,7 @@ void replicationCron(void);
void replicationStartPendingFork(void); void replicationStartPendingFork(void);
void replicationHandleMasterDisconnection(void); void replicationHandleMasterDisconnection(void);
void replicationCacheMaster(client *c); void replicationCacheMaster(client *c);
void resizeReplicationBacklog(long long newsize); void resizeReplicationBacklog();
void replicationSetMaster(char *ip, int port); void replicationSetMaster(char *ip, int port);
void replicationUnsetMaster(void); void replicationUnsetMaster(void);
void refreshGoodSlavesCount(void); void refreshGoodSlavesCount(void);
...@@ -2497,7 +2497,7 @@ void setupSignalHandlers(void); ...@@ -2497,7 +2497,7 @@ void setupSignalHandlers(void);
void removeSignalHandlers(void); void removeSignalHandlers(void);
int createSocketAcceptHandler(socketFds *sfd, aeFileProc *accept_handler); int createSocketAcceptHandler(socketFds *sfd, aeFileProc *accept_handler);
int changeListenPort(int port, socketFds *sfd, aeFileProc *accept_handler); int changeListenPort(int port, socketFds *sfd, aeFileProc *accept_handler);
int changeBindAddr(sds *addrlist, int addrlist_len); int changeBindAddr(void);
struct redisCommand *lookupCommand(robj **argv ,int argc); struct redisCommand *lookupCommand(robj **argv ,int argc);
struct redisCommand *lookupCommandBySdsLogic(dict *commands, sds s); struct redisCommand *lookupCommandBySdsLogic(dict *commands, sds s);
struct redisCommand *lookupCommandBySds(sds s); struct redisCommand *lookupCommandBySds(sds s);
...@@ -3064,6 +3064,7 @@ void swapMainDbWithTempDb(redisDb *tempDb); ...@@ -3064,6 +3064,7 @@ void swapMainDbWithTempDb(redisDb *tempDb);
void tlsInit(void); void tlsInit(void);
void tlsCleanup(void); void tlsCleanup(void);
int tlsConfigure(redisTLSContextConfig *ctx_config); int tlsConfigure(redisTLSContextConfig *ctx_config);
int isTlsConfigured(void);
#define redisDebug(fmt, ...) \ #define redisDebug(fmt, ...) \
printf("DEBUG %s:%d > " fmt "\n", __FILE__, __LINE__, __VA_ARGS__) printf("DEBUG %s:%d > " fmt "\n", __FILE__, __LINE__, __VA_ARGS__)
......
...@@ -372,6 +372,12 @@ error: ...@@ -372,6 +372,12 @@ error:
return C_ERR; return C_ERR;
} }
/* Return 1 if TLS was already configured, 0 otherwise.
*/
int isTlsConfigured(void) {
return redis_tls_ctx != NULL;
}
#ifdef TLS_DEBUGGING #ifdef TLS_DEBUGGING
#define TLSCONN_DEBUG(fmt, ...) \ #define TLSCONN_DEBUG(fmt, ...) \
serverLog(LL_DEBUG, "TLSCONN: " fmt, __VA_ARGS__) serverLog(LL_DEBUG, "TLSCONN: " fmt, __VA_ARGS__)
......
...@@ -288,6 +288,116 @@ start_server {tags {"introspection"}} { ...@@ -288,6 +288,116 @@ start_server {tags {"introspection"}} {
assert_equal [r config get save] {save {}} assert_equal [r config get save] {save {}}
} }
} {} {external:skip} } {} {external:skip}
test {CONFIG SET with multiple args} {
set some_configs {maxmemory 10000001 repl-backlog-size 10000002 save {3000 5}}
# Backup
set backups {}
foreach c [dict keys $some_configs] {
lappend backups $c [lindex [r config get $c] 1]
}
# multi config set and veirfy
assert_equal [eval "r config set $some_configs"] "OK"
dict for {c val} $some_configs {
assert_equal [lindex [r config get $c] 1] $val
}
# Restore backup
assert_equal [eval "r config set $backups"] "OK"
}
test {CONFIG SET rollback on set error} {
# This test passes an invalid percent value to maxmemory-clients which should cause an
# input verification failure during the "set" phase before trying to apply the
# configuration. We want to make sure the correct failure happens and everything
# is rolled back.
# backup maxmemory config
set mm_backup [lindex [r config get maxmemory] 1]
set mmc_backup [lindex [r config get maxmemory-clients] 1]
set qbl_backup [lindex [r config get client-query-buffer-limit] 1]
# Set some value to maxmemory
assert_equal [r config set maxmemory 10000002] "OK"
# Set another value to maxmeory together with another invalid config
assert_error "ERR Config set failed - percentage argument must be less or equal to 100" {
r config set maxmemory 10000001 maxmemory-clients 200% client-query-buffer-limit invalid
}
# Validate we rolled back to original values
assert_equal [lindex [r config get maxmemory] 1] 10000002
assert_equal [lindex [r config get maxmemory-clients] 1] $mmc_backup
assert_equal [lindex [r config get client-query-buffer-limit] 1] $qbl_backup
# Make sure we revert back to the previous maxmemory
assert_equal [r config set maxmemory $mm_backup] "OK"
}
test {CONFIG SET rollback on apply error} {
# This test tries to configure a used port number in redis. This is expected
# to pass the `CONFIG SET` validity checking implementation but fail on
# actual "apply" of the setting. This will validate that after an "apply"
# failure we rollback to the previous values.
proc dummy_accept {chan addr port} {}
set some_configs {maxmemory 10000001 port 0 client-query-buffer-limit 10m}
# On Linux we also set the oom score adj which has an apply function. This is
# used to verify that even successful applies are rolled back if some other
# config's apply fails.
set oom_adj_avail [expr {!$::external && [exec uname] == "Linux"}]
if {$oom_adj_avail} {
proc get_oom_score_adj {} {
set pid [srv 0 pid]
set fd [open "/proc/$pid/oom_score_adj" "r"]
set val [gets $fd]
close $fd
return $val
}
set some_configs [linsert $some_configs 0 oom-score-adj yes oom-score-adj-values {1 1 1}]
set read_oom_adj [get_oom_score_adj]
}
# Backup
set backups {}
foreach c [dict keys $some_configs] {
lappend backups $c [lindex [r config get $c] 1]
}
set used_port [expr ([dict get $backups port]+1)%65536]
dict set some_configs port $used_port
# Run a dummy server on used_port so we know we can't configure redis to
# use it. It's ok for this to fail because that means used_port is invalid
# anyway
catch {socket -server dummy_accept $used_port}
# Try to listen on the used port, pass some more configs to make sure the
# returned failure message is for the first bad config and everything is rolled back.
assert_error "ERR Config set failed - Unable to listen on this port*" {
eval "r config set $some_configs"
}
# Make sure we reverted back to previous configs
dict for {conf val} $backups {
assert_equal [lindex [r config get $conf] 1] $val
}
if {$oom_adj_avail} {
assert_equal [get_oom_score_adj] $read_oom_adj
}
# Make sure we can still communicate with the server (on the original port)
set r1 [redis_client]
assert_equal [$r1 ping] "PONG"
$r1 close
}
test {CONFIG SET duplicate configs} {
assert_error "ERR*duplicate*" {r config set maxmemory 10000001 maxmemory 10000002}
}
test {CONFIG SET set immutable} {
assert_error "ERR*immutable*" {r config set daemonize yes}
}
# Config file at this point is at a weird state, and includes all # Config file at this point is at a weird state, and includes all
# known keywords. Might be a good idea to avoid adding tests here. # known keywords. Might be a good idea to avoid adding tests here.
......
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