-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
release-22.2: kvserver: fail stale ConfChange when rejected by raft #106150
Closed
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
Replace a code snippet with a drop-in solution that exists now.
ProposeConfChange just calls MarshalConfChange followed by Step, and by inlining this in our codebase we gain the ability to figure out if raft rejected our ConfChange. We will need this in a follow-up commit.
Because etcd/raft activates configuration changes when they are applied, but checks new proposed configs before they are considered for adding to the log (or forwarding to the leader), the following can happen: - conf change 1 gets evaluated on a leaseholder n1 - lease changes - new leaseholder evaluates and commits conf change 2 - n1 receives and applies conf change 2 - conf change 1 gets added to the proposal buffer and flushed; RawNode rejects it because conf change 1 is not compatible on top of conf change 2 Prior to this commit, because raft silently replaces the conf change with an empty entry, we would never see the proposal below raft (where it would be rejected due to the lease change). In effect, this caused replica unavailability because the proposal and the associated latch would stick around forever, and the replica circuit breaker would trip. This commit provides a targeted fix: when the proposal buffer flushes a conf change to raft, we check if it got replaced with an empty entry. If so, we properly finish the proposal. To be conservative, we signal it with an ambiguous result: it seems conceivable that the rejection would only occur on a reproposal, while the original proposal made it into raft before the lease change, and the local replica is in fact behind on conf changes rather than ahead (which can happen if it's a follower). The only "customer" here is the replicate queue (and scatter, etc) so this is acceptable; any choice here would necessarily be a "hard error" anyway. Epic: CRDB-25287 Release note (bug fix): under rare circumstances, a replication change could get stuck when proposed near lease/leadership changes (and likely under overload), and the replica circuit breakers could trip. This problem has been addressed. Fixes cockroachdb#105797. Closes cockroachdb#104709.
Thanks for opening a backport. Please check the backport criteria before merging:
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
Add a brief release justification to the body of your PR to justify this backport. Some other things to consider:
|
erikgrinaker
approved these changes
Jul 5, 2023
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.
Backport 3/3 commits from #106104.
/cc @cockroachdb/release
Release justification: fixes a bug that can result in replica unavailability.
Because etcd/raft activates configuration changes when they are applied, but checks new proposed configs before they are considered for adding to the log (or forwarding to the leader), the following can happen:
it because conf change 1 is not compatible on top of conf change 2
Prior to this commit, because raft silently replaces the conf change with an
empty entry, we would never see the proposal below raft (where it would be
rejected due to the lease change). In effect, this caused replica unavailability
because the proposal and the associated latch would stick around forever, and the
replica circuit breaker would trip.
This commit provides a targeted fix: when the proposal buffer flushes a conf
change to raft, we check if it got replaced with an empty entry. If so, we
properly finish the proposal. To be conservative, we signal it with an ambiguous
result: it seems conceivable that the rejection would only occur on a
reproposal, while the original proposal made it into raft before the lease
change, and the local replica is in fact behind on conf changes rather than
ahead (which can happen if it's a follower). The only "customer" here is the
replicate queue (and scatter, etc) so this is acceptable; any choice here would
necessarily be a "hard error" anyway.
Epic: CRDB-25287
Release note (bug fix): under rare circumstances, a replication change could get
stuck when proposed near lease/leadership changes (and likely under overload),
and the replica circuit breakers could trip. This problem has been addressed.
Fixes #105797.
Closes #104709.