raft: fix panic on MsgApp after log truncation #31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pav-kv
changed the title
pav-kv
force-pushed
the
panic-on-truncation
branch
10 times, most recently
from
00532c3
to
2da1d51
Compare
pav-kv
changed the title
|
@nvanbenschoten @tbg PTAL Commit 1: the minimal fix. |
|
@ahrtr PTAL |
This commit fixes a panic in the following scenario: 1. The flow of MsgApp from the leader to a follower is throttled (i.e. Inflights is full). 2. The leader doesn't fetch entries from storage and only periodically sends MsgApps with empty Entries. 3. A log compaction/truncation happens in the background, and cuts out a portion of the log beyond what's still in-flight towards the slow follower (i.e. Progress.Match < log cutoff) 4. Some messages to the slow follower get dropped, and as a result it replies with a rejection MsgAppResp. 5. The leader resets Progress.Next = Progress.Match+1, and is about to retry sending entries from this point. 6. In raft.maybeSendAppend it calls raftLog.term and gets 0 for the missing entry (instead of some indication/error that the log was truncated at this index). It also skips fetching entries (as in step 2), and goes ahead sending an empty MsgApp (with LogTerm = 0 and a fresh Commit index). 7. When the follower gets this MsgApp, in raftLog.maybeAppend it a) wrongly passes the matchTerm check because the 0 index matches the 0 corresponding to a missing entry in the local log, b) tries to bump the Commit index and panics because this index is beyond its local log's lastIndex(). This bug was introduced in 42419da. Specifically, the steps (2) and (6) previously used to unconditionally fetch raftLog.entries(), which would return ErrCompacted in the above scenario, and prevent sending the problematic MsgApp. The commit above inroduced a condition under which the ErrCompacted would be unnoticed. This commit makes maybeSendAppend more aware of this compaction scenario, and prevents sending the problematic MsgApp. Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
This commit makes raftLog.term() call return ErrCompacted and ErrUnavailable errors if the requested log index is out of bounds. Previously it would return 0 which was an error-prone behaviour. Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
pav-kv
force-pushed
the
panic-on-truncation
branch
from
2da1d51
to
d0fb0cd
Compare
nvanbenschoten
approved these changes
Mar 8, 2023
nvanbenschoten
approved these changes
Mar 8, 2023
|
Github isn't liking my approval. It may be because I'm not a maintainer. |
ahrtr
reviewed
Mar 9, 2023
This commit makes the testing loggers print Panic and Fatal agruments before
redirecting them to the panic() call. Previously they would be displayed in a
non-human-readable way, as something like:
panic: ([]interface {}) 0x1400000eb88
Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a panic and adds a regression test for the following scenario:
MsgAppfrom the leader to a follower is throttled (i.e.Inflightsis full).MsgApps with emptyEntries.Progress.Match< log cutoff)MsgAppResp.Progress.Next = Progress.Match+1, and is about to retry sending entries from this point.raft.maybeSendAppendit callsraftLog.termand gets 0 for the missing entry (instead of some indication/error that the log was truncated at this index). It also skips fetching entries (as in step 2), and goes ahead sending an emptyMsgApp(withLogTerm = 0and a freshCommitindex).MsgApp, inraftLog.maybeAppendit a) wrongly passes thematchTermcheck because the 0 index matches the 0 corresponding to a missing entry in the local log, b) tries to bump theCommitindex and panics because this index is beyond its local log'slastIndex().This bug was introduced in 42419da. Specifically, the steps (2) and (6) previously used to unconditionally fetch
raftLog.entries(), which would returnErrCompactedin the above scenario, and prevent sending the problematicMsgApp. The commit above introduced a condition under which theErrCompactedwould be unnoticed.This PR makes
maybeSendAppendmore aware of this compaction scenario, and prevents sending the problematicMsgApp.