Repository navigation
fix: let a node that is one sequence behind rejoin the cluster - #700
Conversation
|
At our stand, the situation turned out to be that the nodes spread to different views and could not restore their functionality. |
Thanks for this. Before we look at the fix itself, here are some notes on How I checked: I copied
Only two of the six tests fail on
Keep, with fixes
For both: assert each group's view with Tests that don't exercise the fix
Why are these two needed in this PR? They pass both on
If they're meant to document current behaviour, please fix the comments and the recovery check. Otherwise they may not belong in this PR.
General
Flakiness:
I'll comment on the fix itself separately. |
Thank you very much. Don't comment on the correction yet. I need to finalize the split test first. |
961b575 to
9ac3e18
Compare
I've removed the extra tests. I have corrected the remaining ones, please take a look at them or there are more comments on them. |
Thanks, this is a big improvement. The shared setup, the delivery wait that checks for request 2 and stops its goroutines, How I checked: I ran the two tests on this branch, on
So the first test covers the
|
One more suggestion: could we take this one change at a time? I'd start with just the
For the first step, it would help to make Would you be open to splitting it this way? |
I'm willing to do anything to solve the main problem. I won't share the changes yet. I'm afraid that the main situation that I would like to overcome will go away. The nodes were stuck in views n, n + 1, n + 2. and the long waiting time did not lead to the fact that they restored their work and were able to continue calculating transactions. |
9ac3e18 to
b45bcf0
Compare
I changed the test according to your comments. |
Thanks for the update. The tests look good now. The first one builds the views 0/1/2 shape you described, the second runs with all timers ticking, and the setup checks stop the test at the step that went wrong. How I checked: you wanted to know whether each change on its own solves the main problem, so I ran both tests on four builds, 5 rounds each:
Both tests now fail without the fix, every time.
On your question whether each change alone solves the main problem:
So splitting shouldn't lose the main fix. The first PR, with the Two small points on the tests:
|
b45bcf0 to
da65fd3
Compare
no: in the prod, all nodes remained mutually accessible, the network was not completely cut; they got stuck precisely because of the out-of-sync sequences |
da65fd3 to
e7bbcf0
Compare
pfi79
changed the title
|
@HagarMeir On your advice, I will divide the pull request into 2. Now the first part will go, after we work it out and accept it, I will send the second one. |
e7bbcf0 to
6b6f10f
Compare
Thanks for splitting this. The change looks right to me, and the test now does what it says. How I checked: I ran
Why the change is correct:
One thing worth mentioning in the PR description: the value in Small nits in
Thanks for updating the I'll do one final pass before approving. |
6b6f10f to
911eb33
Compare
A node that fell behind the cluster by exactly the proposal that was in flight could never rejoin, and went silent forever. The numbers that tell a node how far the cluster got were taken from two different frames of reference. ViewSequences held the sequence that had just been decided, while the heartbeat a leader broadcasts, the sequence a node reports when its state is collected, and the sequence a follower compares its own against all refer to the sequence the view is about to propose. A node that syncs in the middle of a proposal is told the cluster is at a sequence it has already decided: it starts the new view at that sequence, waits for a proposal that was already sent once and is never sent again, and never learns that it is behind. Publish the next proposal sequence in ViewSequences as soon as the view moves on to it, so that the heartbeat a leader broadcasts, the sequence a node reports when its state is collected, and the sequence a follower compares its own against are all the same number. Decide() was the only place that stored the decided sequence; everywhere else already stored the next sequence to propose, at the start of a view and when it ends. Every reader of the value expects the next sequence: the check on the state transfer response, Seq == latestDecisionSeq+1; the heartbeat comparison, ourSeq+1 == hb.Seq; and the controller, which restarts the view at ViewSequences when a sync finds nothing new. That last path used to restart the view at a sequence that had already been decided, so this change fixes it as well. During a rolling upgrade the value carried by HeartBeat.Seq and StateTransferResponse.Sequence changes meaning: a new leader now reports the sequence it is about to propose rather than the one it just decided. An old follower sharing a view with a new leader therefore looks one sequence behind after every decision and syncs every NumOfTicksBehindBeforeSyncing ticks (10 by default) until it is upgraded. That is extra work, but it is safe, and it is the same work a follower that really is one sequence behind has to do. The scenario is covered end to end by TestNodeOneSequenceBehindRejoins in test/view_split_test.go. It builds the shape that was seen in production, a cluster of seven spread over three views, and it fails on main every time it is run: the two nodes that stayed behind never deliver the next request. The second half of the problem, a node that never starts a view change because it keeps hearing its leader, is handled by the announcement-triggered sync and by TestNodeBehindOnIdleViewRejoins, which follow in a separate pull request. Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
911eb33 to
385d17e
Compare
fix |
A node that falls behind a view change without ever asking to change view has no signal that tells it the cluster moved on. It sits in a view nobody is in any more, and it waits there: it is not the leader, so it has no reason to start a view change, and it keeps receiving the heartbeat of the leader it is stuck with, so it has no reason to time out either. A view change message for the view beyond the next one is what such a node's peers keep sending, but it was recorded and dropped: a view change only accepts messages for the view immediately after the one a node is in, and the view the cluster moved on to is no longer being announced, so there was no other way for the node to find out where the cluster went. A node that hears a quorum of distinct senders (f+1) announce a view beyond the one it is in now syncs, at most once per announced view, so that a faulty sender cannot make it sync over and over again. The announcement carries no signature, so it only triggers the sync: the view itself is still adopted through the authenticated new view message. The per-view sync bookkeeping is reset through a single helper on every path where the view changes, including a restore from the log. The scenario is covered end to end by TestNodeBehindOnIdleViewRejoins in test/view_split_test.go. The scope of it is the window in which the follower still hears its old leader: once the whole cluster is connected, the heartbeat responses and the timeouts rescue both nodes anyway, so this change makes a node recover on the announcement instead of waiting for its timeouts. Based on hyperledger#700, which fixes the other half of the problem, a node that is one sequence behind the proposal in flight. Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
A node that falls behind a view change without ever asking to change view has no signal that tells it the cluster moved on. It sits in a view nobody is in any more, and it waits there: it is not the leader, so it has no reason to start a view change, and it keeps receiving the heartbeat of the leader it is stuck with, so it has no reason to time out either. A view change message for the view beyond the next one is what such a node's peers keep sending, but it was recorded and dropped: a view change only accepts messages for the view immediately after the one a node is in, and the view the cluster moved on to is no longer being announced, so there was no other way for the node to find out where the cluster went. A node that hears a quorum of distinct senders (f+1) announce a view beyond the one it is in now syncs, at most once per announced view, so that a faulty sender cannot make it sync over and over again. The announcement carries no signature, so it only triggers the sync: the view itself is still adopted through the authenticated new view message. The per-view sync bookkeeping is reset through a single helper on every path where the view changes, including a restore from the log. The scenario is covered end to end by TestNodeBehindOnIdleViewRejoins in test/view_split_test.go. The scope of it is the window in which the follower still hears its old leader: once the whole cluster is connected, the heartbeat responses and the timeouts rescue both nodes anyway, so this change makes a node recover on the announcement instead of waiting for its timeouts. Based on hyperledger#700, which fixes the other half of the problem, a node that is one sequence behind the proposal in flight. Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
A node that falls behind a view change without ever asking to change view has no signal that tells it the cluster moved on. It sits in a view nobody is in any more, and it waits there: it is not the leader, so it has no reason to start a view change, and it keeps receiving the heartbeat of the leader it is stuck with, so it has no reason to time out either. A view change message for the view beyond the next one is what such a node's peers keep sending, but it was recorded and dropped: a view change only accepts messages for the view immediately after the one a node is in, and the view the cluster moved on to is no longer being announced, so there was no other way for the node to find out where the cluster went. A node that hears a quorum of distinct senders (f+1) announce a view beyond the one it is in now syncs, at most once per announced view, so that a faulty sender cannot make it sync over and over again. The announcement carries no signature, so it only triggers the sync: the view itself is still adopted through the authenticated new view message. The per-view sync bookkeeping is reset through a single helper on every path where the view changes, including a restore from the log. The scenario is covered end to end by TestNodeBehindOnIdleViewRejoins in test/view_split_test.go. The scope of it is the window in which the follower still hears its old leader: once the whole cluster is connected, the heartbeat responses and the timeouts rescue both nodes anyway, so this change makes a node recover on the announcement instead of waiting for its timeouts. Based on hyperledger#700, which fixes the other half of the problem, a node that is one sequence behind the proposal in flight. Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
2 participants
A node that fell behind the cluster by exactly the proposal that was in flight could never
rejoin, and went silent forever.
The numbers that tell a node how far the cluster got were taken from two different frames of
reference. ViewSequences held the sequence that had just been decided, while the heartbeat a
leader broadcasts, the sequence a node reports when its state is collected, and the sequence a
follower compares its own against all refer to the sequence the view is about to propose. A node
that syncs in the middle of a proposal is told the cluster is at a sequence it has already
decided: it starts the new view at that sequence, waits for a proposal that was already sent
once and is never sent again, and never learns that it is behind.
Publish the next proposal sequence in ViewSequences as soon as the view moves on to it, so that
the heartbeat a leader broadcasts, the sequence a node reports when its state is collected, and
the sequence a follower compares its own against are all the same number.
Decide() was the only place that stored the decided sequence; everywhere else already stored
the next sequence to propose, at the start of a view and when it ends. Every reader of the value
expects the next sequence: the check on the state transfer response, Seq == latestDecisionSeq+1;
the heartbeat comparison, ourSeq+1 == hb.Seq; and the controller, which restarts the view at
ViewSequences when a sync finds nothing new. That last path used to restart the view at a
sequence that had already been decided, so this change fixes it as well.
During a rolling upgrade the value carried by HeartBeat.Seq and StateTransferResponse.Sequence
changes meaning: a new leader now reports the sequence it is about to propose rather than the one
it just decided. An old follower sharing a view with a new leader therefore looks one sequence
behind after every decision and syncs every NumOfTicksBehindBeforeSyncing ticks (10 by default)
until it is upgraded. That is extra work, but it is safe, and it is the same work a follower that
really is one sequence behind has to do.
The scenario is covered end to end by TestNodeOneSequenceBehindRejoins in
test/view_split_test.go. It builds the shape that was seen in production, a cluster of seven
spread over three views, and it fails on main every time it is run: the two nodes that stayed
behind never deliver the next request.
The second half of the problem, a node that never starts a view change because it keeps hearing
its leader, is handled by the announcement-triggered sync and by
TestNodeBehindOnIdleViewRejoins, which follow in a separate pull request.