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

Fix assertion when a key is lazy expired during cluster key migration (#11176)

Redis 7.0 has #9890 which added an assertion when the propagation queue
was not flushed and we got to beforeSleep.
But it turns out that when processCommands calls getNodeByQuery and
decides to reject the command, it can lead to a key that was lazy
expired and is deleted without later flushing the propagation queue.

This change prevents lazy expiry from deleting the key at this stage
(not as part of a command being processed in `call`)
parent 78259826
...@@ -6783,7 +6783,7 @@ clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, in ...@@ -6783,7 +6783,7 @@ clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, in
* slot migration, the channel will be served from the source * slot migration, the channel will be served from the source
* node until the migration completes with CLUSTER SETSLOT <slot> * node until the migration completes with CLUSTER SETSLOT <slot>
* NODE <node-id>. */ * NODE <node-id>. */
int flags = LOOKUP_NOTOUCH | LOOKUP_NOSTATS | LOOKUP_NONOTIFY; int flags = LOOKUP_NOTOUCH | LOOKUP_NOSTATS | LOOKUP_NONOTIFY | LOOKUP_NOEXPIRE;
if ((migrating_slot || importing_slot) && !is_pubsubshard) if ((migrating_slot || importing_slot) && !is_pubsubshard)
{ {
if (lookupKeyReadWithFlags(&server.db[0], thiskey, flags) == NULL) missing_keys++; if (lookupKeyReadWithFlags(&server.db[0], thiskey, flags) == NULL) missing_keys++;
......
...@@ -41,7 +41,11 @@ ...@@ -41,7 +41,11 @@
* C-level DB API * C-level DB API
*----------------------------------------------------------------------------*/ *----------------------------------------------------------------------------*/
int expireIfNeeded(redisDb *db, robj *key, int force_delete_expired); /* Flags for expireIfNeeded */
#define EXPIRE_FORCE_DELETE_EXPIRED 1
#define EXPIRE_AVOID_DELETE_EXPIRED 2
int expireIfNeeded(redisDb *db, robj *key, int flags);
int keyIsExpired(redisDb *db, robj *key); int keyIsExpired(redisDb *db, robj *key);
/* Update LFU when an object is accessed. /* Update LFU when an object is accessed.
...@@ -72,6 +76,8 @@ void updateLFU(robj *val) { ...@@ -72,6 +76,8 @@ void updateLFU(robj *val) {
* LOOKUP_NOSTATS: Don't increment key hits/misses counters. * LOOKUP_NOSTATS: Don't increment key hits/misses counters.
* LOOKUP_WRITE: Prepare the key for writing (delete expired keys even on * LOOKUP_WRITE: Prepare the key for writing (delete expired keys even on
* replicas, use separate keyspace stats and events (TODO)). * replicas, use separate keyspace stats and events (TODO)).
* LOOKUP_NOEXPIRE: Perform expiration check, but avoid deleting the key,
* so that we don't have to propagate the deletion.
* *
* Note: this function also returns NULL if the key is logically expired but * Note: this function also returns NULL if the key is logically expired but
* still existing, in case this is a replica and the LOOKUP_WRITE is not set. * still existing, in case this is a replica and the LOOKUP_WRITE is not set.
...@@ -92,8 +98,12 @@ robj *lookupKey(redisDb *db, robj *key, int flags) { ...@@ -92,8 +98,12 @@ robj *lookupKey(redisDb *db, robj *key, int flags) {
* command, since the command may trigger events that cause modules to * command, since the command may trigger events that cause modules to
* perform additional writes. */ * perform additional writes. */
int is_ro_replica = server.masterhost && server.repl_slave_ro; int is_ro_replica = server.masterhost && server.repl_slave_ro;
int force_delete_expired = flags & LOOKUP_WRITE && !is_ro_replica; int expire_flags = 0;
if (expireIfNeeded(db, key, force_delete_expired)) { if (flags & LOOKUP_WRITE && !is_ro_replica)
expire_flags |= EXPIRE_FORCE_DELETE_EXPIRED;
if (flags & LOOKUP_NOEXPIRE)
expire_flags |= EXPIRE_AVOID_DELETE_EXPIRED;
if (expireIfNeeded(db, key, expire_flags)) {
/* The key is no longer valid. */ /* The key is no longer valid. */
val = NULL; val = NULL;
} }
...@@ -1657,13 +1667,17 @@ int keyIsExpired(redisDb *db, robj *key) { ...@@ -1657,13 +1667,17 @@ int keyIsExpired(redisDb *db, robj *key) {
* *
* On replicas, this function does not delete expired keys by default, but * On replicas, this function does not delete expired keys by default, but
* it still returns 1 if the key is logically expired. To force deletion * it still returns 1 if the key is logically expired. To force deletion
* of logically expired keys even on replicas, set force_delete_expired to * of logically expired keys even on replicas, use the EXPIRE_FORCE_DELETE_EXPIRED
* a non-zero value. Note though that if the current client is executing * flag. Note though that if the current client is executing
* replicated commands from the master, keys are never considered expired. * replicated commands from the master, keys are never considered expired.
* *
* On the other hand, if you just want expiration check, but need to avoid
* the actual key deletion and propagation of the deletion, use the
* EXPIRE_AVOID_DELETE_EXPIRED flag.
*
* The return value of the function is 0 if the key is still valid, * The return value of the function is 0 if the key is still valid,
* otherwise the function returns 1 if the key is expired. */ * otherwise the function returns 1 if the key is expired. */
int expireIfNeeded(redisDb *db, robj *key, int force_delete_expired) { int expireIfNeeded(redisDb *db, robj *key, int flags) {
if (!keyIsExpired(db,key)) return 0; if (!keyIsExpired(db,key)) return 0;
/* If we are running in the context of a replica, instead of /* If we are running in the context of a replica, instead of
...@@ -1681,9 +1695,14 @@ int expireIfNeeded(redisDb *db, robj *key, int force_delete_expired) { ...@@ -1681,9 +1695,14 @@ int expireIfNeeded(redisDb *db, robj *key, int force_delete_expired) {
* expired. */ * expired. */
if (server.masterhost != NULL) { if (server.masterhost != NULL) {
if (server.current_client == server.master) return 0; if (server.current_client == server.master) return 0;
if (!force_delete_expired) return 1; if (!(flags & EXPIRE_FORCE_DELETE_EXPIRED)) return 1;
} }
/* In some cases we're explicitly instructed to return an indication of a
* missing key without actually deleting it, even on masters. */
if (flags & EXPIRE_AVOID_DELETE_EXPIRED)
return 1;
/* If clients are paused, we keep the current dataset constant, /* If clients are paused, we keep the current dataset constant,
* but return to the client what we believe is the right state. Typically, * but return to the client what we believe is the right state. Typically,
* at the end of the pause we will properly expire the key OR we will * at the end of the pause we will properly expire the key OR we will
......
...@@ -3087,6 +3087,7 @@ int objectSetLRUOrLFU(robj *val, long long lfu_freq, long long lru_idle, ...@@ -3087,6 +3087,7 @@ int objectSetLRUOrLFU(robj *val, long long lfu_freq, long long lru_idle,
#define LOOKUP_NONOTIFY (1<<1) /* Don't trigger keyspace event on key misses. */ #define LOOKUP_NONOTIFY (1<<1) /* Don't trigger keyspace event on key misses. */
#define LOOKUP_NOSTATS (1<<2) /* Don't update keyspace hits/misses counters. */ #define LOOKUP_NOSTATS (1<<2) /* Don't update keyspace hits/misses counters. */
#define LOOKUP_WRITE (1<<3) /* Delete expired keys even in replicas. */ #define LOOKUP_WRITE (1<<3) /* Delete expired keys even in replicas. */
#define LOOKUP_NOEXPIRE (1<<4) /* Avoid deleting lazy expired keys. */
void dbAdd(redisDb *db, robj *key, robj *val); void dbAdd(redisDb *db, robj *key, robj *val);
int dbAddRDBLoad(redisDb *db, sds key, robj *val); int dbAddRDBLoad(redisDb *db, sds key, robj *val);
......
...@@ -95,6 +95,7 @@ set ::all_tests { ...@@ -95,6 +95,7 @@ set ::all_tests {
unit/client-eviction unit/client-eviction
unit/violations unit/violations
unit/replybufsize unit/replybufsize
unit/cluster/misc
unit/cluster/cli unit/cluster/cli
unit/cluster/scripting unit/cluster/scripting
unit/cluster/hostnames unit/cluster/hostnames
......
start_cluster 2 2 {tags {external:skip cluster}} {
test {Key lazy expires during key migration} {
R 0 DEBUG SET-ACTIVE-EXPIRE 0
set key_slot [R 0 CLUSTER KEYSLOT FOO]
R 0 set FOO BAR PX 10
set src_id [R 0 CLUSTER MYID]
set trg_id [R 1 CLUSTER MYID]
R 0 CLUSTER SETSLOT $key_slot MIGRATING $trg_id
R 1 CLUSTER SETSLOT $key_slot IMPORTING $src_id
after 11
assert_error {ASK*} {R 0 GET FOO}
R 0 ping
} {PONG}
}
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