Skip to content

Commit

Permalink
raft: allow voter to become learner through snapshot
Browse files Browse the repository at this point in the history
At the time of writing, we don't allow configuration changes to change
voters to learners directly, but note that a snapshot may compress
multiple changes to the configuration into one: the voter could have
been removed, then readded as a learner and the snapshot reflects both
changes. In that case, a voter receives a snapshot telling it that it is
now a learner. In fact, the node has to accept that snapshot, or it is
permanently cut off from the Raft log.

I think this just wasn't realized in the original work, but this is just
my guess since there generally is very little rationale on the various
decisions made. I also generally haven't been able to figure out whether
the decision to prevent voters from becoming learners without first
having been removed was motivated by some particular concern, or if it
just wasn't deemed necessary. I suspect it is the latter because
demoting a voter seems perfectly safe.

See etcd-io#8751 (comment).
  • Loading branch information
tbg committed Jul 8, 2019
1 parent 48f5bb6 commit 6f009d2
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 15 deletions.
10 changes: 0 additions & 10 deletions raft/raft.go
Original file line number Diff line number Diff line change
Expand Up @@ -1374,16 +1374,6 @@ func (r *raft) restore(s pb.Snapshot) bool {
return false
}

// The normal peer can't become learner.
if !r.isLearner {
for _, id := range s.Metadata.ConfState.Learners {
if id == r.id {
r.logger.Errorf("%x can't become learner when restores snapshot [index: %d, term: %d]", r.id, s.Metadata.Index, s.Metadata.Term)
return false
}
}
}

r.raftLog.restore(s)

// Reset the configuration and add the (potentially updated) peers in anew.
Expand Down
16 changes: 11 additions & 5 deletions raft/raft_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2777,9 +2777,15 @@ func TestRestoreWithLearner(t *testing.T) {
}
}

// TestRestoreInvalidLearner verfies that a normal peer can't become learner again
// when restores snapshot.
func TestRestoreInvalidLearner(t *testing.T) {
// TestRestoreVoterToLearner verifies that a normal peer can be downgraded to a
// learner through a snapshot. At the time of writing, we don't allow
// configuration changes to do this directly, but note that the snapshot may
// compress multiple changes to the configuration into one: the voter could have
// been removed, then readded as a learner and the snapshot reflects both
// changes. In that case, a voter receives a snapshot telling it that it is now
// a learner. In fact, the node has to accept that snapshot, or it is
// permanently cut off from the Raft log.
func TestRestoreVoterToLearner(t *testing.T) {
s := pb.Snapshot{
Metadata: pb.SnapshotMetadata{
Index: 11, // magic number
Expand All @@ -2794,8 +2800,8 @@ func TestRestoreInvalidLearner(t *testing.T) {
if sm.isLearner {
t.Errorf("%x is learner, want not", sm.id)
}
if ok := sm.restore(s); ok {
t.Error("restore succeed, want fail")
if ok := sm.restore(s); !ok {
t.Error("restore failed unexpectedly")
}
}

Expand Down

0 comments on commit 6f009d2

Please sign in to comment.