Fix data loss when the old primary takes over the slots after online #974
Conversation
There is a race in clusterHandleConfigEpochCollision, which may cause the old primary node to take over the slots again after coming online and cause data loss. It happens when the old primary and the new primary have the same config epoch, and the old primary has a smaller node id and win the collision. In this case, the old primary and the new primary are in the same shard, we are not sure which is strictly the latest. To prevent data loss, now in clusterHandleConfigEpochCollision we will let the node with the larger offset win the conflict. In addition to this change, when a node increments the config epoch throught conflicts, or CLUSTER FAILOVER TAKEOVER, or CLUSTER BUMPEPOCH, we will send PONGs to all ndoes to allow the cluster to reach consensus on the new config epoch more quickly. This also can closes valkey-io#969. Signed-off-by: Binbin <[email protected]> Signed-off-by: Binbin <[email protected]>
|
Here is the logs: The old primary: The new primary (using a failover takeover, and became a replica at the last) |
| @@ -22,8 +22,12 @@ set paused_pid [srv 0 pid] | |||
| set paused_pid1 [srv -1 pid] | |||
| set paused_pid2 [srv -2 pid] | |||
| test "Killing majority of master nodes" { | |||
| # Bumping the epochs to increase the chance of conflicts. | |||
There was a problem hiding this comment.
without the fix, this can lead to increased conflicts, which can cause tests to fail
There was a problem hiding this comment.
I think the original issue is less about the non-deterministic epoch resolution behavior we have today but more about the race between the primary election and the forced takeover. I think that would be addressed if we disable the replica failover before pausing the primaries.
The change of bumping the epoch before pausing the primaries changes the intent of the test IMO to validate your "more deterministic conflict resolution" proposal, which does require increasing the conflict probably.
Can we make a surgical fix to the original test issue but create a new file to add the new test? It is a bit confusing to include it in a test file called manual-takeover as conflicts can happen outside manual-takeover too. maybe "cluster-epoch-conflict.tcl" or just "epoch-conflict.tcl"?
There was a problem hiding this comment.
i am fail to see why disbale the replica failover will work. Do you mean disable the it on the replica side? The code already doing it. Or do you mean disable it on the primary side?
# For this test, disable replica failover until
# all of the primaries are confirmed killed. Otherwise
# there might be enough time to elect a replica.
set replica_ids { 5 6 7 }
foreach id $replica_ids {
R $id config set cluster-replica-no-failover yes
}
set paused_pid [srv 0 pid]
set paused_pid1 [srv -1 pid]
set paused_pid2 [srv -2 pid]
test "Killing majority of master nodes" {
pause_process $paused_pid
pause_process $paused_pid1
pause_process $paused_pid2
}
foreach id $replica_ids {
R $id config set cluster-replica-no-failover no
}
yes, it is a good idea to split it, i will find some ways to do it.
There was a problem hiding this comment.
i am fail to see why disbale the replica failover will work.
I think there is a race condition between turning off failover on the replicas and stopping the primary. I meant to suggest the following changes (reverting the order R $id config set cluster-replica-no-failover no and pause_process $paused_pid). Can you give it a try (without the server code change)?
| # Bumping the epochs to increase the chance of conflicts. | |
| foreach id $replica_ids { | |
| R $id config set cluster-replica-no-failover no | |
| } | |
| test "Killing majority of master nodes" { | |
| pause_process $paused_pid | |
| pause_process $paused_pid1 | |
| pause_process $paused_pid2 | |
| } |
There was a problem hiding this comment.
What is the specific race? look like you are suggested to remove this one? e81bd15
… and we lost the context Signed-off-by: Binbin <[email protected]>
| serverLog(LL_NOTICE, "configEpoch collision with node %.40s (%s). configEpoch set to %llu", sender->name, | ||
| sender->human_nodename, (unsigned long long)myself->configEpoch); | ||
| clusterSaveConfigOrDie(1); |
There was a problem hiding this comment.
print the logs first and then save the config, in case the save fails and we lost the context
There was a problem hiding this comment.
I think this is a good context to keep in the code. add it as code comment?
There was a problem hiding this comment.
SGTM, i will add this in the next commit.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #974 +/- ##
============================================
- Coverage 70.50% 70.47% -0.04%
============================================
Files 114 114
Lines 61742 61750 +8
============================================
- Hits 43532 43519 -13
- Misses 18210 18231 +21
|
There was a problem hiding this comment.
I hope this solves the problem! I'm not sure about the theoretical stuff. @PingXie what do you think?
| * with the conflicting epoch (the 'sender' node), it will assign itself | ||
| * the greatest configuration epoch currently detected among nodes plus 1. | ||
| * | ||
| * The above is an optimistic scenario. It this node and the sender node | ||
| * are in the same shard, their conflict in configEpoch indicates that a | ||
| * node has experienced a partition. Or for example, the old primary node | ||
| * was down then up again, and the new primary node won the election. In | ||
| * this case, we need to take the replication offset into consideration, | ||
| * otherwise, if the old primary wins the collision, we will lose some of | ||
| * the new primary's data. | ||
| * | ||
| * This means that even if there are multiple nodes colliding, the node | ||
| * with the greatest Node ID never moves forward, so eventually all the nodes | ||
| * end with a different configuration epoch. |
There was a problem hiding this comment.
It's better to describe this as if same shard ... otherwise ... like you did in another code comment below:
/* If sender and myself are in the same shard, the one with the
* bigger offset will win. Otherwise if sender and myself are not
* in the same shard, the one will the lexicographically small
* Node ID will win.*/
And the text below the added text describes the guarantee "the node with the greatest Node ID never moves forward". We need to change this text.
Do we have another guarantees in the same-shard scenario? Can it happen that sender and myself don't have the same view on the replication offsets and both try to bump the epoch?
And what if one node thinks that it is in the same shard but the other one has a different idea and tries to compare by node-id? Can it happen that both try to bump the epoch?
There was a problem hiding this comment.
some thoughts:
- shard-id is mostly stable except in two situations:
case 1. The delayed propagation of shard-id, as captured in #778 and #573. I am still trying to wrap my head around all the possible permutations.
case 2. cluster-allow-replica-migration when enabled would allow a replica or primary to join another shard if their shard lost their last slot to the other shard. BTW, I don't get the use case of this setting and even more why it is enabled by default. This setting created quite a few surprises IMO. We disable it on GCP Memorystore.
In both cases, we could have a split "are_in_the_same_shard" view on the nodes involved.
case 1. The split view could happen such that node A correctly concludes that it is in the same shard as B but B, because it hasn't received A real shard_id, thinks otherwise and relies on the node-id only to resolve the conflict. When this happens, the conflict continues. Eventually, B will receive A's real shard_id and should arrive at the same conclusion as A. Data written to the losing node during this transitional period will get lost when the conflict is eventually resolved.
case 2. The split could happen in both the source and target shards but I think the end result is innocuous in the source shard since it will be empty by then. The impact on the target shard should be mitigated by #885
- We will always lose data (except for the trivial case of no user writes) even if we pick the larger offset of the two. The replication history diverges the moment the "split brain" occurs even the two still share the same repl_id, which is what leads to the epoch conflict. By favoring the larger offset, we are saying the one with more user writes wins. I think this is a reasonable decision but just wanted to clarify that the two no longer share the same replication history despite having the same repl_id.
There was a problem hiding this comment.
btw, @madolson, this is one of the many places where I fully agree with you a new clustering solution that is designed holistically would fare better.
There was a problem hiding this comment.
overall I think this is a good fix but I feel that we can address #969 by disabling replica votes before pausing the primaries. @enjoy-binbin can you give that a shot? If it works, I would suggest moving this fix out of 8.0.
| serverLog(LL_NOTICE, "configEpoch collision with node %.40s (%s). configEpoch set to %llu", sender->name, | ||
| sender->human_nodename, (unsigned long long)myself->configEpoch); | ||
| clusterSaveConfigOrDie(1); |
There was a problem hiding this comment.
I think this is a good context to keep in the code. add it as code comment?
| @@ -22,8 +22,12 @@ set paused_pid [srv 0 pid] | |||
| set paused_pid1 [srv -1 pid] | |||
| set paused_pid2 [srv -2 pid] | |||
| test "Killing majority of master nodes" { | |||
| # Bumping the epochs to increase the chance of conflicts. | |||
There was a problem hiding this comment.
I think the original issue is less about the non-deterministic epoch resolution behavior we have today but more about the race between the primary election and the forced takeover. I think that would be addressed if we disable the replica failover before pausing the primaries.
The change of bumping the epoch before pausing the primaries changes the intent of the test IMO to validate your "more deterministic conflict resolution" proposal, which does require increasing the conflict probably.
Can we make a surgical fix to the original test issue but create a new file to add the new test? It is a bit confusing to include it in a test file called manual-takeover as conflicts can happen outside manual-takeover too. maybe "cluster-epoch-conflict.tcl" or just "epoch-conflict.tcl"?
| * with the conflicting epoch (the 'sender' node), it will assign itself | ||
| * the greatest configuration epoch currently detected among nodes plus 1. | ||
| * | ||
| * The above is an optimistic scenario. It this node and the sender node | ||
| * are in the same shard, their conflict in configEpoch indicates that a | ||
| * node has experienced a partition. Or for example, the old primary node | ||
| * was down then up again, and the new primary node won the election. In | ||
| * this case, we need to take the replication offset into consideration, | ||
| * otherwise, if the old primary wins the collision, we will lose some of | ||
| * the new primary's data. | ||
| * | ||
| * This means that even if there are multiple nodes colliding, the node | ||
| * with the greatest Node ID never moves forward, so eventually all the nodes | ||
| * end with a different configuration epoch. |
There was a problem hiding this comment.
some thoughts:
- shard-id is mostly stable except in two situations:
case 1. The delayed propagation of shard-id, as captured in #778 and #573. I am still trying to wrap my head around all the possible permutations.
case 2. cluster-allow-replica-migration when enabled would allow a replica or primary to join another shard if their shard lost their last slot to the other shard. BTW, I don't get the use case of this setting and even more why it is enabled by default. This setting created quite a few surprises IMO. We disable it on GCP Memorystore.
In both cases, we could have a split "are_in_the_same_shard" view on the nodes involved.
case 1. The split view could happen such that node A correctly concludes that it is in the same shard as B but B, because it hasn't received A real shard_id, thinks otherwise and relies on the node-id only to resolve the conflict. When this happens, the conflict continues. Eventually, B will receive A's real shard_id and should arrive at the same conclusion as A. Data written to the losing node during this transitional period will get lost when the conflict is eventually resolved.
case 2. The split could happen in both the source and target shards but I think the end result is innocuous in the source shard since it will be empty by then. The impact on the target shard should be mitigated by #885
- We will always lose data (except for the trivial case of no user writes) even if we pick the larger offset of the two. The replication history diverges the moment the "split brain" occurs even the two still share the same repl_id, which is what leads to the epoch conflict. By favoring the larger offset, we are saying the one with more user writes wins. I think this is a reasonable decision but just wanted to clarify that the two no longer share the same replication history despite having the same repl_id.
| * with the conflicting epoch (the 'sender' node), it will assign itself | ||
| * the greatest configuration epoch currently detected among nodes plus 1. | ||
| * | ||
| * The above is an optimistic scenario. It this node and the sender node | ||
| * are in the same shard, their conflict in configEpoch indicates that a | ||
| * node has experienced a partition. Or for example, the old primary node | ||
| * was down then up again, and the new primary node won the election. In | ||
| * this case, we need to take the replication offset into consideration, | ||
| * otherwise, if the old primary wins the collision, we will lose some of | ||
| * the new primary's data. | ||
| * | ||
| * This means that even if there are multiple nodes colliding, the node | ||
| * with the greatest Node ID never moves forward, so eventually all the nodes | ||
| * end with a different configuration epoch. |
There was a problem hiding this comment.
btw, @madolson, this is one of the many places where I fully agree with you a new clustering solution that is designed holistically would fare better.
| /* Get the next ID available at the best of this node knowledge. */ | ||
| server.cluster->currentEpoch++; | ||
| myself->configEpoch = server.cluster->currentEpoch; | ||
| clusterSaveConfigOrDie(1); | ||
| clusterBroadcastPong(CLUSTER_BROADCAST_ALL); |
There was a problem hiding this comment.
Can we document why there is a broadcast here as well. If there is any type of significant conflicts we'll send out a lot of messages unnecessary.
There was a problem hiding this comment.
yes, we can document it. i guess in normal cases, conflicts should be rare.
I think i am doing this here is also because, if the primary has a larger epoch, and its replicas don't know it in time, the replica may not be able to win the election (it has an old epoch and will get rejected in this case, but rare just my guess)
| * bigger offset will win. Otherwise if sender and myself are not | ||
| * in the same shard, the one will the lexicographically small | ||
| * Node ID will win.*/ | ||
| if (areInSameShard(sender, myself)) { |
There was a problem hiding this comment.
I suppose I don't get why this works. If we both nodes think they are primaries, won't both of our offsets be the replication offsets we are now using could still be the same, and both nodes would both bump. The previous logic was consistent between nodes, whereas this logic may not and they may fight? It feels like this is just a race between the manual failover and the automated failover, and I'm not sure we necessarily should automatically be picking a winner.
There was a problem hiding this comment.
if the offset is the same, yes, it will still have the same issue, in this case, if the offset is the same, we will use the origin node id logic to pick a winner, in this case, the logic is the same as before.
i guess in here the only problem i want to solve is: the old primary paused or block (offset is small), and the new primary was elected and did some writes (offset is large). They encountered a conflict and the old primary win, got a new config epoch and forced the new primary to become a replica again.
There is a race in clusterHandleConfigEpochCollision, which may cause
the old primary node to take over the slots again after coming online
and cause data loss. It happens when the old primary and the new primary
have the same config epoch, and the old primary has a smaller node id
and win the collision.
In this case, the old primary and the new primary are in the same shard,
we are not sure which is strictly the latest. To prevent data loss,
now in clusterHandleConfigEpochCollision we will let the node with the
larger offset win the conflict.
In addition to this change, when a node increments the config epoch
throught conflicts, or CLUSTER FAILOVER TAKEOVER, or CLUSTER BUMPEPOCH,
we will send PONGs to all ndoes to allow the cluster to reach consensus
on the new config epoch more quickly.
This also can closes #969.