From cf61ad14cc45787e57d9af3f28f41462ac0f2aa2 Mon Sep 17 00:00:00 2001 From: Huang Zhw Date: Mon, 2 Aug 2021 19:59:08 +0800 Subject: [PATCH] When redis-cli received ASK, it didn't handle it (#8930) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When redis-cli received ASK, it used string matching wrong and didn't handle it. When we access a slot which is in migrating state, it maybe return ASK. After redirect to the new node, we need send ASKING command before retry the command. In this PR after redis-cli receives ASK, we send a ASKING command before send the origin command after reconnecting. Other changes: * Make redis-cli -u and -c (unix socket and cluster mode) incompatible with one another. * When send command fails, we avoid the 2nd reconnect retry and just print the error info. Users will decide how to do next. See #9277. * Add a test faking two redis nodes in TCL to just send ASK and OK in redis protocol to test ASK behavior. Co-authored-by: Viktor Söderqvist Co-authored-by: Oran Agra --- src/redis-cli.c | 63 +++++++++++++++++++++++++------ tests/helpers/fake_redis_node.tcl | 58 ++++++++++++++++++++++++++++ tests/integration/redis-cli.tcl | 31 ++++++++++++--- 3 files changed, 135 insertions(+), 17 deletions(-) create mode 100644 tests/helpers/fake_redis_node.tcl diff --git a/src/redis-cli.c b/src/redis-cli.c index 0ca1809ab..edf695ec4 100644 --- a/src/redis-cli.c +++ b/src/redis-cli.c @@ -220,6 +220,7 @@ static struct config { long long lru_test_sample_size; int cluster_mode; int cluster_reissue_command; + int cluster_send_asking; int slave_mode; int pipe_mode; int pipe_timeout; @@ -931,6 +932,29 @@ static int cliConnect(int flags) { return REDIS_OK; } +/* In cluster, if server replies ASK, we will redirect to a different node. + * Before sending the real command, we need to send ASKING command first. */ +static int cliSendAsking() { + redisReply *reply; + + config.cluster_send_asking = 0; + if (context == NULL) { + return REDIS_ERR; + } + reply = redisCommand(context,"ASKING"); + if (reply == NULL) { + fprintf(stderr, "\nI/O error\n"); + return REDIS_ERR; + } + int result = REDIS_OK; + if (reply->type == REDIS_REPLY_ERROR) { + result = REDIS_ERR; + fprintf(stderr,"ASKING failed: %s\n",reply->str); + } + freeReplyObject(reply); + return result; +} + static void cliPrintContextError(void) { if (context == NULL) return; fprintf(stderr,"Error: %s\n",context->errstr); @@ -1306,7 +1330,7 @@ static int cliReadReply(int output_raw_strings) { /* Check if we need to connect to a different node and reissue the * request. */ if (config.cluster_mode && reply->type == REDIS_REPLY_ERROR && - (!strncmp(reply->str,"MOVED",5) || !strcmp(reply->str,"ASK"))) + (!strncmp(reply->str,"MOVED ",6) || !strncmp(reply->str,"ASK ",4))) { char *p = reply->str, *s; int slot; @@ -1330,6 +1354,9 @@ static int cliReadReply(int output_raw_strings) { printf("-> Redirected to slot [%d] located at %s:%d\n", slot, config.hostip, config.hostport); config.cluster_reissue_command = 1; + if (!strncmp(reply->str,"ASK ",4)) { + config.cluster_send_asking = 1; + } cliRefreshPrompt(); } else if (!config.interactive && config.set_errcode && reply->type == REDIS_REPLY_ERROR) @@ -1819,6 +1846,11 @@ static int parseOptions(int argc, char **argv) { } } + if (config.hostsocket && config.cluster_mode) { + fprintf(stderr,"Options -c and -s are mutually exclusive.\n"); + exit(1); + } + /* --ldb requires --eval. */ if (config.eval_ldb && config.eval == NULL) { fprintf(stderr,"Options --ldb and --ldb-sync-mode require --eval.\n"); @@ -2037,26 +2069,32 @@ static sds *getSdsArrayFromArgv(int argc, char **argv, int quoted) { static int issueCommandRepeat(int argc, char **argv, long repeat) { while (1) { + if (config.cluster_reissue_command || context == NULL || + context->err == REDIS_ERR_IO || context->err == REDIS_ERR_EOF) + { + if (cliConnect(CC_FORCE) != REDIS_OK) { + cliPrintContextError(); + config.cluster_reissue_command = 0; + return REDIS_ERR; + } + } config.cluster_reissue_command = 0; - if (cliSendCommand(argc,argv,repeat) != REDIS_OK) { - cliConnect(CC_FORCE); - - /* If we still cannot send the command print error. - * We'll try to reconnect the next time. */ - if (cliSendCommand(argc,argv,repeat) != REDIS_OK) { + if (config.cluster_send_asking) { + if (cliSendAsking() != REDIS_OK) { cliPrintContextError(); return REDIS_ERR; } } + if (cliSendCommand(argc,argv,repeat) != REDIS_OK) { + cliPrintContextError(); + return REDIS_ERR; + } /* Issue the command again if we got redirected in cluster mode */ if (config.cluster_mode && config.cluster_reissue_command) { - /* If cliConnect fails, sleep for a while and try again. */ - if (cliConnect(CC_FORCE) != REDIS_OK) - sleep(1); - } else { - break; + continue; } + break; } return REDIS_OK; } @@ -8301,6 +8339,7 @@ int main(int argc, char **argv) { config.lru_test_mode = 0; config.lru_test_sample_size = 0; config.cluster_mode = 0; + config.cluster_send_asking = 0; config.slave_mode = 0; config.getrdb_mode = 0; config.stat_mode = 0; diff --git a/tests/helpers/fake_redis_node.tcl b/tests/helpers/fake_redis_node.tcl new file mode 100644 index 000000000..a12d87fed --- /dev/null +++ b/tests/helpers/fake_redis_node.tcl @@ -0,0 +1,58 @@ +# A fake Redis node for replaying predefined/expected traffic with a client. +# +# Usage: tclsh fake_redis_node.tcl PORT COMMAND REPLY [ COMMAND REPLY [ ... ] ] +# +# Commands are given as space-separated strings, e.g. "GET foo", and replies as +# RESP-encoded replies minus the trailing \r\n, e.g. "+OK". + +set port [lindex $argv 0]; +set expected_traffic [lrange $argv 1 end]; + +# Reads and parses a command from a socket and returns it as a space-separated +# string, e.g. "set foo bar". +proc read_command {sock} { + set char [read $sock 1] + switch $char { + * { + set numargs [gets $sock] + set result {} + for {set i 0} {$i<$numargs} {incr i} { + read $sock 1; # dollar sign + set len [gets $sock] + set str [read $sock $len] + gets $sock; # trailing \r\n + lappend result $str + } + return $result + } + {} { + # EOF + return {} + } + default { + # Non-RESP command + set rest [gets $sock] + return "$char$rest" + } + } +} + +proc accept {sock host port} { + global expected_traffic + foreach {expect_cmd reply} $expected_traffic { + if {[eof $sock]} {break} + set cmd [read_command $sock] + if {[string equal -nocase $cmd $expect_cmd]} { + puts $sock $reply + flush $sock + } else { + puts $sock "-ERR unexpected command $cmd" + break + } + } + close $sock +} + +socket -server accept $port +after 5000 set done timeout +vwait done diff --git a/tests/integration/redis-cli.tcl b/tests/integration/redis-cli.tcl index db525e405..8ffa154a7 100644 --- a/tests/integration/redis-cli.tcl +++ b/tests/integration/redis-cli.tcl @@ -85,8 +85,8 @@ start_server {tags {"cli"}} { set _ $tmp } - proc _run_cli {opts args} { - set cmd [rediscli [srv host] [srv port] [list -n $::dbnum {*}$args]] + proc _run_cli {host port db opts args} { + set cmd [rediscli $host $port [list -n $db {*}$args]] foreach {key value} $opts { if {$key eq "pipe"} { set cmd "sh -c \"$value | $cmd\"" @@ -105,15 +105,19 @@ start_server {tags {"cli"}} { } proc run_cli {args} { - _run_cli {} {*}$args + _run_cli [srv host] [srv port] $::dbnum {} {*}$args } proc run_cli_with_input_pipe {cmd args} { - _run_cli [list pipe $cmd] -x {*}$args + _run_cli [srv host] [srv port] $::dbnum [list pipe $cmd] -x {*}$args } proc run_cli_with_input_file {path args} { - _run_cli [list path $path] -x {*}$args + _run_cli [srv host] [srv port] $::dbnum [list path $path] -x {*}$args + } + + proc run_cli_host_port_db {host port db args} { + _run_cli $host $port $db {} {*}$args } proc test_nontty_cli {name code} { @@ -230,6 +234,23 @@ start_server {tags {"cli"}} { assert_equal "foo\nbar" [run_cli lrange list 0 -1] } + test_nontty_cli "ASK redirect test" { + # Set up two fake Redis nodes. + set tclsh [info nameofexecutable] + set script "tests/helpers/fake_redis_node.tcl" + set port1 [find_available_port $::baseport $::portcount] + set port2 [find_available_port $::baseport $::portcount] + set p1 [exec $tclsh $script $port1 \ + "SET foo bar" "-ASK 12182 127.0.0.1:$port2" &] + set p2 [exec $tclsh $script $port2 \ + "ASKING" "+OK" \ + "SET foo bar" "+OK" &] + # Sleep to make sure both fake nodes have started listening + after 100 + # Run the cli + assert_equal "OK" [run_cli_host_port_db "127.0.0.1" $port1 0 -c SET foo bar] + } + test_nontty_cli "Quoted input arguments" { r set "\x00\x00" "value" assert_equal "value" [run_cli --quoted-input get {"\x00\x00"}]