From 6f009d211f17ce2c93d024beffc0242608b1363c Mon Sep 17 00:00:00 2001
From: Tobias Schottdorf <tobias.schottdorf@gmail.com>
Date: Mon, 8 Jul 2019 09:32:24 +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 a42bb4e63db..846ff496ffc 100644
--- a/raft/raft.go
+++ b/raft/raft.go
@@ -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.
diff --git a/raft/raft_test.go b/raft/raft_test.go
index 8fcc2f5c76d..6d9a26efd89 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")
 	}
 }