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

DEPS: upgrade Raft to 72d62a1 #105578

Closed
wants to merge 3 commits into from

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Jun 26, 2023

Makes Raft leaders step down when removing themselves from the range.

72d62a1 2023-06-13: step down when leader removes itself [Erik Grinaker]

Epic: none
Release note: None

@erikgrinaker erikgrinaker requested a review from a team June 26, 2023 20:46
@erikgrinaker erikgrinaker requested a review from a team as a code owner June 26, 2023 20:46
@erikgrinaker erikgrinaker self-assigned this Jun 26, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Jun 26, 2023

For some reason, etcd-io/raft#77 breaks leader removals. Should've known better than to make opportunistic cleanups, I suppose. Not sure why it breaks yet, will dig into it tomorrow.

@erikgrinaker erikgrinaker marked this pull request as draft June 27, 2023 07:03
@erikgrinaker
Copy link
Contributor Author

I think the problem is that the leader steps down before we complete the joint quorum change:

I230627 07:36:53.597613 796 go.etcd.io/raft/v3/raft.go:1886  [T1,n1,s1,r64/1:/{Table/Max-Max}] 2883  1 switched to configuration voters=(2 3) learners=(1)
I230627 07:36:53.597678 796 go.etcd.io/raft/v3/raft.go:835  [T1,n1,s1,r64/1:/{Table/Max-Max}] 2884  1 became follower at term 6
I230627 07:36:53.597749 796 kv/kvserver/replica_raft.go:842  [T1,n1,s1,r64/1:/{Table/Max-Max},raft] 2885  raft leader changed: 1 -> 0
I230627 07:36:53.597739 2901 go.etcd.io/raft/v3/raft.go:1886  [T1,n2,s2,r64/2:/{Table/Max-Max}] 2886  2 switched to configuration voters=(2 3) learners=(1)
I230627 07:36:53.597811 3809 go.etcd.io/raft/v3/raft.go:1886  [T1,n3,s3,r64/3:/{Table/Max-Max}] 2887  3 switched to configuration voters=(2 3) learners=(1)

The leader is now a learner, but it needs to propose and commit the conf change that actually removes it from the range, otherwise noone else will campaign for leadership.

@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Jun 27, 2023

The problem is a mismatch between Raft and CRDB. In Raft, the leader steps down once it become a learner in a joint config. In CRDB, we don't elect a new leader until the old leader is fully removed from the range -- so when the leader becomes a learner and steps down, the range remains leaderless. This patch fixes it:

--- a/pkg/kv/kvserver/client_raft_test.go
+++ b/pkg/kv/kvserver/client_raft_test.go
diff --git a/pkg/kv/kvserver/replica_raft.go b/pkg/kv/kvserver/replica_raft.go
index d33eece0989..48b1aed4727 100644
--- a/pkg/kv/kvserver/replica_raft.go
+++ b/pkg/kv/kvserver/replica_raft.go
@@ -2570,7 +2570,7 @@ func shouldCampaignAfterConfChange(
                // don't expect to hit this, but let's be defensive.
                return false
        }
-       if _, ok := desc.GetReplicaDescriptorByID(roachpb.ReplicaID(raftStatus.Lead)); ok {
+       if r, ok := desc.GetReplicaDescriptorByID(roachpb.ReplicaID(raftStatus.Lead)); ok && r.IsAnyVoter() {
                // The leader is still in the descriptor.
                return false
        }

Unfortunately, this will break backwards compatibility with 23.1 nodes. We have a few options:

  • Roll back the step-down change in etcd/raft.
  • Make etcd/raft step-down opt-out or opt-in.
  • Modify etcd/raft to step down on removal, not when becoming learner.

The Raft thesis describes leader removal in section 4.2.2, we should probably just do what's specified there, with an option to enable/disable it, for backwards compatibility and migrations.

@erikgrinaker erikgrinaker changed the title DEPS: upgrade Raft DEPS: upgrade Raft to 72d62a1 Jun 27, 2023
@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Jun 27, 2023

Making it opt-in in etcd-io/raft#79

@erikgrinaker erikgrinaker force-pushed the raft-bump branch 2 times, most recently from 115bd01 to 362fa76 Compare June 27, 2023 18:54
Epic: none
Release note: None
This patch campaigns when the leader demotes itself to a learner during
an atomic conf change, instead of waiting until it is completely removed
from the range. Relying on the old leader to commit the final conf
change while it's a learner appears unfortunate, and is not compatible
with an upcoming etcd/raft change that makes the learner step down in
this case.

This is backwards compatible with 23.1, because the same follower
replica is responsible for campaigning, and it does not matter if it
campaigns during the demotion or removal -- in particular because it
forces an immediate election via `forceCampaignLocked()` which
immediately bumps the term.

Epic: none
Release note: None
This causes the Raft leader to step down when it removes itself from the
range or demotes itself to a learner. This doesn't make any difference
functionally, since we're careful to no longer tick the replica or
otherwise use it, but in principle it could cause a leader that was no
longer part of the range to continue to assert leadership, stalling the
range since it wouldn't be able to propose anything. Stepping down
appears safer and cleaner.

Epic: none
Release note: None
@erikgrinaker
Copy link
Contributor Author

Closing in favor of #107203. Raft has already been bumped.

@erikgrinaker erikgrinaker deleted the raft-bump branch January 10, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants