bugfix:del keys in slot replicate to replica, and trigger other invalidations

[weim0000]

Bugfix:
with the scenario if we force assigned a slot to other master, old master will lose the slot ownership, then old master will call the function delKeysInSlot() to delete all keys which in the slot. These delete operations should replicate to replicas, avoid the data divergence issue in master and replicas.

Additionally, in this case, we now call:

• signalModifiedKey (to invalidate WATCH)
• moduleNotifyKeyspaceEvent (key space notification for modules)
• dirty++ (to signal that the persistence file may be outdated)

Fixes #10967

Release notes: Fix a bug where nodes in a cluster may not replicate or handle internal events for keys deleted when another node in the cluster claimed a slot.

[weim0000]

Close #10967

[madolson]

I have one concern about this change. Which is that the replica may incorrectly report an empty dataset since it has received the deletes over replication but hasn't yet processed the clusterbus information about the slot ownership yet. On the master these two events are happening at the same time, so we don't need to worry about it. This is related to my position that we should be replicating slot ownership transitions so they are properly sequenced with the replicated data. I think a better fix would be to replicate some type of "slot flush + DEL" command.

It's also worth noting that this is really only possible during misuse or failure modes. The reproduction in the issue was forcing a cluster to take ownership of a slot instead of migrating it using the importing/migrating functionality.

[oranagra]

So the master in this case will replicate a single CLUSTER SLOTFLUSH command (rather than a ton of DEL commands) to the replica, but what about an AOF file? i suppose that's currently also broken, and will be solved by the same new command?

besides that, all the other missing DEL related actions are still needed (on both the master and replica).
i.e. notifyKeyspaceEvent, signalModifiedKey, dirty++.

[weim0000]

thanks for your advise @judeng @madolson @oranagra @MeirShpilraien
I have already update this pr with 'signalModifiedKey, dirty++'

@oranagra clusterCommand is “CMD_ADMIN” command , so i think "CLUSTER SLOTFLUSH" maybe not better replicate to the replica

[oranagra]

@weim0000 i think we're still missing notifyKeyspaceEvent.

Regarding CLUSTER being an admin command, i don't see a problem with that.
i do think it's better to send one command down the line rather than a flood of DEL commands.

But more importantly, i agree with @madolson that it's better that the slot ownership changes are better be properly sequenced on the data stream rather than relying on the cluster bus alone.
@madolson i imagine such a thing is a much bigger task with some chance for issues from race conditions.
how do you suggest proceeding? fix the local bugs (keyspace notifications, dirty, watch... which happen on the master too) in one PR while we're planning the bigger change elsewhere? or also propagate deletions in some way here and live with the odd state (arguably an improvement over what we have now)?

[judeng]

i do think it's better to send one command down the line rather than a flood of DEL commands.

@oranagra There is a scenario where the master node of the cluster has a replica in standalone mode,and it is strange to execute the cluster slotflush command on this standalone replica. DEL command is a more general solution IMO

[judeng]

i think we're still missing notifyKeyspaceEvent.

I doubt if there really is a need to notify the keyspace events to users here, these keys already belong to dirty data that the user cannot access

[oranagra]

@oranagra There is a scenario where the master node of the cluster has a replica in standalone mode,and it is strange to execute the cluster slotflush command on this standalone replica. DEL command is a more general solution IMO

i don't think it's a viable configuration, for a cluster node to have a replica in standalone more.
maybe there is a scenario where some tool is used to replicate the data from that node where the replica is not a redis-server.
e.g. it could be something like redis-cli --slave or a redis-rdb-cli, and you're right that such a command could break it, but arguably DELs are much worse.
i.e. imagine someone with 3 cluster nodes, and a tool that replicates or migrates the data from these nodes to a target that's a standalone redis, getting these DELs from one node would delete the keys from the target.
so a logical command that says these keys moved would be better than a DEL that's indistinguishable from a normal DEL.
i'm not sure we wanna consider this case, just stating it since you raised it.

I doubt if there really is a need to notify the keyspace events to users here, these keys already belong to dirty data that the user cannot access

I agree, sending keyspace notification about deletions will cause a similar damage as i mentioned above (the keys were not really deleted from the database, just moved), but i do think modules need to know these keys are no longer locally available, so i think we should send some keyspace notification just to modules.

[madolson]

@madolson i imagine such a thing is a much bigger task with some chance for issues from race conditions.

Yes, I don't think we should address the wider issue here, there is another issue #10517 which aims to address a bunch of these items, I would prefer us address it there. Any type of complex change would also be unwise to backport. Therefor, I think the current outlined approach is acceptable for now, since it's solving the data divergence issue. We can safely backport it to 7.0 and earlier, and we can continue on the long term approach.

[weim0000]

thanks for your advise @judeng @madolson @oranagra
I have already update this pr with 'moduleNotifyKeyspaceEvent'

Therefor, I think the current outlined approach is acceptable for now, since it's solving the data divergence issue. We can safely backport it to 7.0 and earlier, and we can continue on the long term approach.

i agree

[madolson]

@weim0000 Cool, change looks gtm. Would you be comfortable adding a test around this case? Want to avoid future regressions here, since we clearly weren't testing for it.

[weim0000]

thanks @madolson i have added a test for this case, and update cluster.c with 'call propagatePendingCommands();'.
please review, thanks

[weim0000]

thanks @enjoy-binbin i have already update the test

[oranagra]

sorry for the delay (been busy).
generally LGTM with few minor comments resulting from another PR that just got merged.
didn't really review the test code in detail.

[weim0000]

thanks @oranagra
i have already resolve all the conversations

[madolson]

@weim0000 I moved the test to the new framework, so now the tests will run as part of CI. Also kicked off a full test run here: https://github.com/redis/redis/actions/runs/2897153567.

[oranagra]

@weim0000 thank you.
this is ready for merge IMO, can you please update the top comment with the scenario in which it happens, and the implications of the bug / changes.
this will serve as a commit command and for the purpose of release notes.

[weim0000]

@oranagra OK, thanks.