raft: make follower commit index advancement safe #122100
Comments
pav-kv
added
C-enhancement
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
cc @cockroachdb/replication |
We need the ability to safely ship backward incompatible changes in the raft module. This commit plumbs version handle in to raft package to enable version gate infrastructure in raft. Touches: cockroachdb#122100 Release note: None
|
Will this potentially also help with raft regression panics such as:
Related discussion in https://cockroachlabs.slack.com/archives/C06UNJ6PE3U/p1713827710088409?thread_ts=1713794326.949309&cid=C06UNJ6PE3U |
|
@sumeerbhola Yes, it will make the follower commit index update to a safe value, instead of panicking. The loss of durability could manifest in some other place though. |
|
@sumeerbhola After some discussion here and internally, there is a concern that, despite removing this Is the short term, it seems safer to still panic if the follower's log regresses. There is a way to do this without taking a dependency on the commit index: #124225. In the long term, we should improve raft's resilience to durability regressions. One systematic way of doing this is described in #124278. |
122690: raft: advance commit index safely r=lyang24 a=lyang24 This change makes the commit index advancement in handleHeartbeat safe. Previously, a follower would attempt to update the commit index to whichever was sent in the MsgHeartbeat message. Out-of-bound indices would crash the node. It is always safe to advance a commit index if the follower's log is "in sync" with the leader, i.e. when its log is guaranteed to be a prefix of the leader's log. This is always true if the term of last entry in the log matches the leader's term, otherwise this guarantee is established when the first MsgApp append message from the leader succeeds. At the moment, the leader will never send a commit index that exceeds the follower's log size. However, this may change in future. This change is a defence-in-depth. The newly added accTerm field will be used for other safety checks in the future, for example to establish that overriding a suffix of entries in raftLog is safe. Part of #122100 Co-authored-by: lyang24 <[email protected]>
nvanbenschoten
added
the
A-leader-leases
A follower panics if commit index advances above its local log last entry index.
For this reason, the leader caps the commit index in heartbeat messages at
Matchindex of the follower log. This may lead to delays in commit index advancement on the follower: etcd-io/raft#138.We should make the follower's commit index advancement more flexible, and safer, and allow the leader to attach the latest
commitindex without capping it. The follower would then cap it locally.When a leader@term sends a commit index, it means all entries <= this index are committed. Generally, the follower's log shares some prefix with the leader's log, and can commit up to the index at which it "forks" from the leader's log. A simple way for the follower to check that its log is fully consistent with the leader's log is to ensure that it has already accepted at least one append from this leader. After that, the follower can safely advance the commit index to
min(commit, lastIndex).Benefits of doing this:
Matchindex), and possibly from heartbeats.Issue etcd-io/raft#138 describes steps to achieve this. It is probably necessary to hide this change behind a version gate, so that in mixed-version clusters followers running the old code don't get a high commit index and panic.
Epic: CRDB-37516
Jira issue: CRDB-39619
The text was updated successfully, but these errors were encountered: