Skip to content
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

kvserver: fail stale ConfChange when rejected by raft #106104

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

tbg
Copy link
Member

@tbg tbg commented Jul 4, 2023

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 #105797.
Closes #104709.

Replace a code snippet with a drop-in solution that exists now.
@tbg tbg force-pushed the stuck-repl-change branch from 28174a4 to 671a40b Compare July 4, 2023 11:03
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg force-pushed the stuck-repl-change branch from 6c44d94 to ebc2a95 Compare July 4, 2023 11:57
@tbg tbg marked this pull request as ready for review July 4, 2023 12:11
@tbg tbg requested a review from a team July 4, 2023 12:11
@erikgrinaker erikgrinaker self-requested a review July 5, 2023 08:06
@erikgrinaker erikgrinaker added backport-22.2.x backport-23.1.x Flags PRs that need to be backported to 23.1 labels Jul 5, 2023
Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great with the targeted fix!

pkg/kv/kvserver/replica_proposal_buf.go Show resolved Hide resolved
pkg/kv/kvserver/client_raft_test.go Outdated Show resolved Hide resolved
tbg added 2 commits July 5, 2023 10:33
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.
@tbg tbg force-pushed the stuck-repl-change branch from b5ab78e to 93117db Compare July 5, 2023 08:34
@tbg
Copy link
Member Author

tbg commented Jul 5, 2023

TFTR!

bors r=erikgrinaker

@craig
Copy link
Contributor

craig bot commented Jul 5, 2023

Build succeeded:

@craig craig bot merged commit 31e7a29 into cockroachdb:master Jul 5, 2023
@blathers-crl
Copy link

blathers-crl bot commented Jul 5, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 3d1760f to blathers/backport-release-22.2-106104: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.


error creating merge commit from 3d1760f to blathers/backport-release-23.1-106104: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@nvanbenschoten
Copy link
Member

I just saw kvnemesis run into this new config change rejected by raft; please retry error when trying to reproduce #101721. Do we want to add retry handing for this error?

@tbg
Copy link
Member Author

tbg commented Jul 5, 2023

Yep let me look at it, thanks for the heads up!

@tbg
Copy link
Member Author

tbg commented Jul 10, 2023

Reopening, since we had to revert the fix: #106267

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kvserver: config change proposals can hang forever roachtest: failover/chaos/read-write failed
4 participants