From af5167b7f35972fd21cd8f8566da7339ee07809a Mon Sep 17 00:00:00 2001 From: WuYunlong Date: Wed, 18 Mar 2020 16:17:46 +0800 Subject: [PATCH 1/2] Add 14-consistency-check.tcl to prove there is a data consistency issue. --- tests/cluster/tests/14-consistency-check.tcl | 87 ++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 tests/cluster/tests/14-consistency-check.tcl diff --git a/tests/cluster/tests/14-consistency-check.tcl b/tests/cluster/tests/14-consistency-check.tcl new file mode 100644 index 000000000..a43725ebc --- /dev/null +++ b/tests/cluster/tests/14-consistency-check.tcl @@ -0,0 +1,87 @@ +source "../tests/includes/init-tests.tcl" + +test "Create a 5 nodes cluster" { + create_cluster 5 5 +} + +test "Cluster should start ok" { + assert_cluster_state ok +} + +test "Cluster is writable" { + cluster_write_test 0 +} + +proc find_non_empty_master {} { + set master_id_no {} + foreach_redis_id id { + if {[RI $id role] eq {master} && [R $id dbsize] > 0} { + set master_id_no $id + } + } + return $master_id_no +} + +proc get_one_of_my_replica {id} { + set replica_port [lindex [lindex [lindex [R $id role] 2] 0] 1] + set replica_id_num [get_instance_id_by_port redis $replica_port] + return $replica_id_num +} + +proc cluster_write_keys_with_expire {id ttl} { + set prefix [randstring 20 20 alpha] + set port [get_instance_attrib redis $id port] + set cluster [redis_cluster 127.0.0.1:$port] + for {set j 100} {$j < 200} {incr j} { + $cluster setex key_expire.$j $ttl $prefix.$j + } + $cluster close +} + +proc test_slave_load_expired_keys {aof} { + test "Slave expired keys is loaded when restarted: appendonly=$aof" { + set master_id [find_non_empty_master] + set replica_id [get_one_of_my_replica $master_id] + + set master_dbsize [R $master_id dbsize] + set slave_dbsize [R $replica_id dbsize] + assert_equal $master_dbsize $slave_dbsize + + set data_ttl 5 + cluster_write_keys_with_expire $master_id $data_ttl + after 100 + set replica_dbsize_1 [R $replica_id dbsize] + assert {$replica_dbsize_1 > $slave_dbsize} + + R $replica_id config set appendonly $aof + R $replica_id config rewrite + + set start_time [clock seconds] + set end_time [expr $start_time+$data_ttl+2] + R $replica_id save + set replica_dbsize_2 [R $replica_id dbsize] + assert {$replica_dbsize_2 > $slave_dbsize} + kill_instance redis $replica_id + + set master_port [get_instance_attrib redis $master_id port] + exec ../../../src/redis-cli -h 127.0.0.1 -p $master_port debug sleep [expr $data_ttl+3] > /dev/null & + + while {[clock seconds] <= $end_time} { + #wait for $data_ttl seconds + } + restart_instance redis $replica_id + + wait_for_condition 200 50 { + [R $replica_id ping] eq {PONG} + } else { + fail "replica #$replica_id not started" + } + + set replica_dbsize_3 [R $replica_id dbsize] + assert {$replica_dbsize_3 > $slave_dbsize} + } +} + +test_slave_load_expired_keys no +after 5000 +test_slave_load_expired_keys yes From f6029fb9256ee1ffa1491f6fa57bb56a9558e952 Mon Sep 17 00:00:00 2001 From: WuYunlong Date: Wed, 18 Mar 2020 16:20:10 +0800 Subject: [PATCH 2/2] Fix master replica inconsistency for upgrading scenario. Before this commit, when upgrading a replica, expired keys will not be loaded, thus causing replica having less keys in db. To this point, master and replica's keys is logically consistent. However, before the keys in master and replica are physically consistent, that is, they have the same dbsize, if master got a problem and the replica got promoted and becomes new master of that partition, and master updates a key which does not exist on master, but physically exists on the old master(new replica), the old master would refuse to update the key, thus causing master and replica data inconsistent. How could this happen? That's all because of the wrong judgement of roles while starting up the server. We can not use server.masterhost to judge if the server is master or replica, since it fails in cluster mode. When we start the server, we load rdb and do want to load expired keys, and do not want to have the ability to active expire keys, if it is a replica. --- src/rdb.c | 2 +- src/server.c | 7 ++++++- src/server.h | 2 ++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index cbcea96c6..5d34f5a32 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -2231,7 +2231,7 @@ int rdbLoadRio(rio *rdb, int rdbflags, rdbSaveInfo *rsi) { * received from the master. In the latter case, the master is * responsible for key expiry. If we would expire keys here, the * snapshot taken by the master may not be reflected on the slave. */ - if (server.masterhost == NULL && !(rdbflags&RDBFLAGS_AOF_PREAMBLE) && expiretime != -1 && expiretime < now) { + if (iAmMaster() && !(rdbflags&RDBFLAGS_AOF_PREAMBLE) && expiretime != -1 && expiretime < now) { decrRefCount(key); decrRefCount(val); } else { diff --git a/src/server.c b/src/server.c index f702da94a..467d09b67 100644 --- a/src/server.c +++ b/src/server.c @@ -1691,7 +1691,7 @@ void databasesCron(void) { /* Expire keys by random sampling. Not required for slaves * as master will synthesize DELs for us. */ if (server.active_expire_enabled) { - if (server.masterhost == NULL) { + if (iAmMaster()) { activeExpireCycle(ACTIVE_EXPIRE_CYCLE_SLOW); } else { expireSlaveKeys(); @@ -4863,6 +4863,11 @@ int redisIsSupervised(int mode) { return 0; } +int iAmMaster(void) { + return ((!server.cluster_enabled && server.masterhost == NULL) || + (server.cluster_enabled && nodeIsMaster(server.cluster->myself))); +} + int main(int argc, char **argv) { struct timeval tv; diff --git a/src/server.h b/src/server.h index fa6770dfa..fdfe5b8ea 100644 --- a/src/server.h +++ b/src/server.h @@ -2393,4 +2393,6 @@ int tlsConfigure(redisTLSContextConfig *ctx_config); #define redisDebugMark() \ printf("-- MARK %s:%d --\n", __FILE__, __LINE__) +int iAmMaster(void); + #endif