From d09728833a1d8015be820fad565714f246ff5ba8 Mon Sep 17 00:00:00 2001
From: Tobias Schottdorf <tobias.schottdorf@gmail.com>
Date: Fri, 28 Jun 2019 12:00:15 +0200
Subject: [PATCH] raft: allow voter to become learner through snapshot

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 https://github.com/etcd-io/etcd/pull/8751#issuecomment-342028091.
---
 raft/raft.go      | 10 ----------
 raft/raft_test.go | 16 +++++++++++-----
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/raft/raft.go b/raft/raft.go
index 3cdc1f0ac6c2..196ed9313157 100644
--- a/raft/raft.go
+++ b/raft/raft.go
@@ -1334,16 +1334,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.logger.Infof("%x [commit: %d, lastindex: %d, lastterm: %d] starts to restore snapshot [index: %d, term: %d]",
 		r.id, r.raftLog.committed, r.raftLog.lastIndex(), r.raftLog.lastTerm(), s.Metadata.Index, s.Metadata.Term)
 
diff --git a/raft/raft_test.go b/raft/raft_test.go
index 805e6071f0e1..172a6ac22339 100644
--- a/raft/raft_test.go
+++ b/raft/raft_test.go
@@ -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
@@ -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")
 	}
 }