raft: add raft learner by siddontang · Pull Request #8751 · etcd-io/etcd

siddontang · 2017-10-24T02:56:50Z

This is a derived PR from #8605 which I think we can close now.

I just cache the voter count in this PR.

Using two processes may be easy, but I tried and found that many codes need to be changed, seem no big benefit now.

siddontang · 2017-10-24T03:07:41Z

raft/progress.go

 	// received entry.
 	ins *inflights
+
+	isLearner bool


do we need to export this, I guess maybe user may need it later

/cc @xiang90

seems fine to me.

xiang90 · 2017-10-25T21:56:02Z

raft/raft.go

 	step stepFunc

+	// voterCount caches the total voter which can vote.
+	voterCount int


i still want to try to separate the learners out of the prs. so we can keep the old logic without adding this "cache". i feel the code can be cleaner overall too.

can you give it a try at least?

xiang90 · 2017-10-25T21:56:53Z

it looks good to me overall. just want to another approach to keep track of learners.

siddontang · 2017-10-26T04:48:11Z

I have separated the learner progress, but the code seems not be simpler.

PTAL @xiang90

xiang90 · 2017-10-26T04:51:49Z

raft/raft.go

-	for id := range r.prs {
-		if id == r.id {
-			continue
+	f := func(prs map[uint64]*Progress) {


make this a method forEachProgress(prs [], prs[], f())? so we do not need to repeat this quite a few times.

xiang90 · 2017-10-26T04:54:20Z

thanks i will give this a closer look tomorrow.

xiang90 · 2017-10-27T00:28:43Z

raft/progress.go

 	// received entry.
 	ins *inflights
+
+	// IsLearner is true if the the follower is a learner.


if this progress is tracked for a learner.

xiang90 · 2017-10-29T05:26:32Z

raft/raft.go

 	// used for testing right now.
 	peers []uint64

+	// learners can not promote or vote.


// learners contains the IDs of all leaner nodes (including self if the local node is a leaner) in the raft cluster.
// learners only receives entries from the leader node. It does not vote or promote itself.

xiang90 · 2017-10-29T05:27:04Z

raft/raft.go


 	state StateType

+	isLearner bool


// ieLearner is true if the local raft node is a learner.

xiang90 · 2017-10-29T05:27:45Z

raft/raft.go

 	}
+	for _, p := range learners {
+		if _, ok := r.prs[p]; ok {
+			panic(fmt.Sprintf("cannot specify both Voter and Learner for node: %x", p))


node %x is in both learner and peer list.

xiang90 · 2017-10-29T05:30:08Z

raft/raft.go

 // r.bcastAppend).
 func (r *raft) maybeCommit() bool {
 	// TODO(bmizerany): optimize.. Currently naive
-	mis := make(uint64Slice, 0, len(r.prs))


is this change really needed in this PR?

xiang90 · 2017-10-29T05:31:36Z

raft/raft.go

 	// All other message types require a progress for m.From (pr).
-	pr, prOk := r.prs[m.From]
-	if !prOk {
+	pr := r.getProgress(m.From)


getProgress can return *p,ok, just like what the previous map struct does. it minimizes code changes. but not a big problem anyway.

If here returns *Progress, bool, I will check the ok boolean in the places which originally use getProgess().xxx to replace prs[id].xxx, and this may cause code complex.

Another way is to add a mustGetProgress function, but I think it is not necessary.

xiang90 · 2017-10-29T05:31:57Z

raft/raft.go

 		r.logger.Debugf("%x failed to send message to %x because it is unreachable [%s]", r.id, m.From, pr)
 	case pb.MsgTransferLeader:
+		if pr.IsLearner {
+			r.logger.Debugf("%x is Learner. Ignored transferring leadership", r.id)


xiang90 · 2017-10-29T05:34:51Z

raft/raft.go

+	if pr != nil {
+		if isLearner && !pr.IsLearner {
+			// can only change Learner to Voter
+			r.logger.Infof("%x ignore addLearner for existing node %x [%s]", r.id, id, pr)


r.id ignored addLeaner: do not support changing from raft peer to learner.

xiang90 · 2017-10-29T05:36:37Z

raft/raft.go

+		pr.IsLearner = false
+		r.prs[id] = pr
+	} else {
+		r.setProgress(id, 0, r.raftLog.lastIndex()+1, isLearner)


we should always put the short case first in if...else... statement for readability. so move this case first, then change the pr exist case.

xiang90 · 2017-10-29T05:37:34Z

raft/raft.go

+func (r *raft) setProgress(id, match, next uint64, isLearner bool) {
+	if isLearner {
+		if _, ok := r.prs[id]; ok {
+			panic(fmt.Sprintf("%x can't change voter to learner for %x", r.id, id))


unexpected changing from x to x.

xiang90 · 2017-10-29T05:38:56Z

raft/raft.go

+	}
+
+	delete(r.learnerPrs, id)
 	r.prs[id] = &Progress{Next: next, Match: match, ins: newInflights(r.maxInflight)}


i would handle not learner case first, since it is easier. so it improves readability.

siddontang · 2017-10-30T00:21:44Z

PTAL @xiang90

xiang90 · 2017-11-01T17:01:28Z

raft/raft_test.go

 	}
 }

+func TestLearnerElectionTimeout(t *testing.T) {


we need to add a description for this test.

xiang90 · 2017-11-01T17:02:52Z

raft/raft_test.go

+
+	nt := newNetwork(n1, n2)
+
+	// Learner can't start election


n2 is learner. Learner should not start election even when times out.

xiang90 · 2017-11-01T17:03:06Z

raft/raft_test.go

+	for i := 0; i < n2.electionTimeout; i++ {
+		n2.tick()
+	}
+	if n1.state != StateFollower {


no need to check the state of n1 here.

xiang90 · 2017-11-01T17:03:18Z

raft/raft_test.go

+		t.Errorf("peer 2 state: %s, want %s", n2.state, StateFollower)
+	}
+
+	// n1 should become leader


check n1 is not leader here.

xiang90 · 2017-11-01T17:04:20Z

raft/raft_test.go

+
+	nt.send(pb.Message{From: 1, To: 1, Type: pb.MsgBeat})
+
+	n1.addNode(2)


well... this tests that a learner can be promoted to a normal node. we should separate this into another test.

xiang90 · 2017-11-01T17:05:57Z

raft/raft_test.go

+	}
+
+	// n1 should become leader
+	nt.send(pb.Message{From: 1, To: 1, Type: pb.MsgHup})


why we do use tick to trigger timeout above, but use msghub here?

xiang90 · 2017-11-01T17:06:27Z

raft/raft_test.go

 	}
 }

+func TestRestoreWithLearner(t *testing.T) {


add test description.

xiang90 · 2017-11-01T17:07:21Z

raft/raft_test.go

+		t.Errorf("nodes = %v, want %v", nodes, wnodes)
+	}
+	if !r.learnerPrs[2].IsLearner {
+		t.Fatalf("node 2 has suffrage %t, want %t", r.prs[2].IsLearner, true)


use t.errorf. rename suffrage to learner.

xiang90 · 2017-11-01T17:10:10Z

raft/raft_test.go

+	}
+
+	if ok := sm.restore(s); ok {
+		t.Fatal("restore succeed, want fail")


use t.errorf here for checking purpose. also doc string about why do we expect this to fail.

xiang90 · 2017-11-01T17:17:01Z

xiang90 · 2017-11-10T01:36:02Z

@siddontang lgtm.

there are a couple of things left though, but we can do them in a follow up PR:

test status has learner info included
update documentation to include the learner feature

siddontang · 2017-11-10T02:32:47Z

@xiang90

Please file some issues so I can't forget it. I will try to use this feature in TiKV at first.

siddontang · 2017-11-10T02:35:51Z

Done @xiang90

siddontang · 2017-11-10T03:17:30Z

@xiang90

The go 1.8 and 1.9 have different format rule, how to fix it?

/cc @gyuho

gyuho · 2017-11-10T05:44:08Z

Yeah could you fix this line https://travis-ci.org/coreos/etcd/jobs/299973342#L841?
etcd CI only uses Go 1.9+.

siddontang · 2017-11-11T01:31:48Z

Hi @gyuho

I update the format but still find https://travis-ci.org/coreos/etcd/jobs/300033398 failed, but I don't know why.

gyuho · 2017-11-11T01:38:38Z

@siddontang Oh that was fixed just today. If you rebase from current master, it should pass.

Thanks!

siddontang · 2017-11-11T02:23:40Z

@gyuho

I rebase the master but the CI still failed.

gyuho · 2017-11-11T02:32:42Z

raft/raft_test.go

 			sm := newRaft(cfg)
 			npeers[id] = sm
 		case *raft:
+			learners := make(map[uint64]bool, 0)


learners := make(map[uint64]bool, len(v.learnerPrs))? Our static analysis tool complains about this :0

oh, got it.

siddontang · 2017-11-11T06:16:42Z

@gyuho

Jenkins failed but the failure seems irrelevant to Raft learner.

xiang90 · 2017-11-12T02:43:06Z

CI failures are not relevant. Merging.

absolute8511 · 2017-12-28T09:42:59Z

I reviewed some codes for this pr and found there may be an issue which can affect the
ApplyConfChange in the raft node.

	// ApplyConfChange applies config change to the local node.
	// Returns an opaque ConfState protobuf which must be recorded
	// in snapshots. Will never return nil; it returns a pointer only
	// to match MemoryStorage.Compact.
	ApplyConfChange(cc pb.ConfChange) *pb.ConfState

Since we add new Learners to pb.ConfState, we should return the pb.ConfState with Learners in the raft.

		case cc := <-n.confc:
			if cc.NodeID == None {
				r.resetPendingConf()
				select {
				case n.confstatec <- pb.ConfState{Nodes: r.nodes()}:
				case <-n.done:
				}
				break
			}
			switch cc.Type {
			case pb.ConfChangeAddNode:
				r.addNode(cc.NodeID)
			case pb.ConfChangeAddLearnerNode:
				r.addLearner(cc.NodeID)
			case pb.ConfChangeRemoveNode:
				// block incoming proposal when local node is
				// removed
				if cc.NodeID == r.id {
					propc = nil
				}
				r.removeNode(cc.NodeID)
			case pb.ConfChangeUpdateNode:
				r.resetPendingConf()
			default:
				panic("unexpected conf type")
			}
			select {
			case n.confstatec <- pb.ConfState{Nodes: r.nodes()}:
			case <-n.done:
			}

However, from the code above, we return all the nodes and learners written into the Nodes and leave the Learners empty. Is that by designed or a potential bug?

xiang90 · 2017-12-28T16:27:03Z

Can you please open an issue for this? Ideally with a test case to illustrate this problem. Thanks.

…

On Thu, Dec 28, 2017 at 4:43 AM Vincent Lee ***@***.***> wrote: I reviewed some codes for this pr and found there may be an issue which can affect the ApplyConfChange in the raft node. // ApplyConfChange applies config change to the local node. // Returns an opaque ConfState protobuf which must be recorded // in snapshots. Will never return nil; it returns a pointer only // to match MemoryStorage.Compact. ApplyConfChange(cc pb.ConfChange) *pb.ConfState Since we add new Learners to pb.ConfState, we should return the pb.ConfState with Learners in the raft. case cc := <-n.confc: if cc.NodeID == None { r.resetPendingConf() select { case n.confstatec <- pb.ConfState{Nodes: r.nodes()}: case <-n.done: } break } switch cc.Type { case pb.ConfChangeAddNode: r.addNode(cc.NodeID) case pb.ConfChangeAddLearnerNode: r.addLearner(cc.NodeID) case pb.ConfChangeRemoveNode: // block incoming proposal when local node is // removed if cc.NodeID == r.id { propc = nil } r.removeNode(cc.NodeID) case pb.ConfChangeUpdateNode: r.resetPendingConf() default: panic("unexpected conf type") } select { case n.confstatec <- pb.ConfState{Nodes: r.nodes()}: case <-n.done: } However, from the code above, we return all the nodes and learners written into the Nodes and leave the Learners empty. Is that by designed or a potential bug? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#8751 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AERbywOgweP8c2eRFR5qyBDmmguHTr0Cks5tE2K7gaJpZM4QDxdR> .

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).

siddontang commented Oct 24, 2017

View reviewed changes

xiang90 reviewed Oct 25, 2017

View reviewed changes

xiang90 reviewed Oct 26, 2017

View reviewed changes

xiang90 reviewed Oct 27, 2017

View reviewed changes

xiang90 reviewed Oct 29, 2017

View reviewed changes

raft/raft.go

state StateType

isLearner bool

Copy link

Contributor

xiang90 Oct 29, 2017

Choose a reason for hiding this comment

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

// ieLearner is true if the local raft node is a learner.

xiang90 reviewed Oct 29, 2017

View reviewed changes

xiang90 added the area/raft label Oct 29, 2017

xiang90 reviewed Nov 1, 2017

View reviewed changes

raft/raft_test.go

}

}

func TestLearnerElectionTimeout(t *testing.T) {

Copy link

Contributor

xiang90 Nov 1, 2017

Choose a reason for hiding this comment

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

we need to add a description for this test.

xiang90 reviewed Nov 1, 2017

View reviewed changes

raft/raft_test.go

}

}

func TestRestoreWithLearner(t *testing.T) {

Copy link

Contributor

xiang90 Nov 1, 2017

Choose a reason for hiding this comment

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

add test description.

xiang90 reviewed Nov 1, 2017

View reviewed changes

xiang90 mentioned this pull request Nov 1, 2017

raft: add learner #8605

Closed

siddontang force-pushed the siddontang/raft_learner branch from d20cbf6 to 35926cc Compare November 10, 2017 02:35

siddontang force-pushed the siddontang/raft_learner branch from 35926cc to 509c9bc Compare November 10, 2017 07:11

siddontang force-pushed the siddontang/raft_learner branch from 509c9bc to 63e5b66 Compare November 11, 2017 02:10

gyuho reviewed Nov 11, 2017

View reviewed changes

raft: support learner

c6f2db2

siddontang force-pushed the siddontang/raft_learner branch from 63e5b66 to c6f2db2 Compare November 11, 2017 02:38

xiang90 merged commit a8fde60 into etcd-io:master Nov 12, 2017

xiang90 mentioned this pull request Nov 12, 2017

raft: non-voting member for Catching up new servers #8568

Closed

This was referenced Nov 12, 2017

raft: support raft learner tikv/tikv#2467

Closed

raft: use raft learner tikv/tikv#2475

Closed

This was referenced Nov 16, 2017

Add ConfChange::AddLearnerNode command. pingcap/kvproto#212

Merged

raft: add learner and tests. tikv/tikv#2491

Merged

absolute8511 mentioned this pull request Jan 3, 2018

raft: raft learners should be returned after applyConfChange #9087

Closed

thaJeztah mentioned this pull request Jan 5, 2019

api: add BindOptions.NonRecursive moby/swarmkit#2780

Merged

tbg mentioned this pull request Jun 28, 2019

raft: allow voter to become learner through snapshot #10864

Merged


		nt.send(pb.Message{From: 1, To: 1, Type: pb.MsgBeat})

		n1.addNode(2)

Conversation

siddontang commented Oct 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiang90 commented Oct 25, 2017

Uh oh!

siddontang commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiang90 commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddontang commented Oct 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiang90 commented Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiang90 commented Nov 10, 2017

Uh oh!

siddontang commented Nov 10, 2017

Uh oh!

siddontang commented Nov 10, 2017

Uh oh!

siddontang commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gyuho commented Nov 10, 2017

Uh oh!

siddontang commented Nov 11, 2017

Uh oh!

gyuho commented Nov 11, 2017

Uh oh!

siddontang commented Nov 11, 2017

xiang90 commented Nov 1, 2017 •

edited

Loading

siddontang commented Nov 10, 2017 •

edited

Loading

gyuho Nov 11, 2017 •

edited

Loading