*: support raft learner in etcd - part 2 #10727

jingyih · 2019-05-15T21:26:12Z

Part 2 of #10645. Continuation of #10725.

This PR incldues the following 16 commits from #10645. The last 6 commits are just formatting and bug fixes for the earlier 10 commits.

(ordered by: latest commits on top)

f058b7fbfe9bb3a1ea82f262514110aa91b0c8d5 clientv3/integration: deflake TestKVForLearner
1a080384e20ca55f48c4effe448c87392600ba3e clientv3/integration: fix cluster tests
11e41685030846ba495a4263d8db352d0e9fc7d0 clientv3/integration: update MemberAdd call
5d1f6c73c1dcdd889a28138b0ba6e79f814a704a integration: remove unnecessary type conversion
4f9eb2f7c8b7175ce7eb690c72461e654bd0a0ff etcdserver: remove unnecessary bool comparison
ab030455051cc034704cc4fc9206786b12ee88f6 words: whitelist words to fix goword test.

b0be8067e40c068eb2180b192d792aec21a1e3a2 clientv3, etcdctl: MemberPromote for learner
f1d6e70e271342691c3f1b8cf7e0074f5804cc15 integration: add TestTransferLeadershipWithLearner
2db0ca918be883749c72ede61a38148550be3811 integration: add TestMoveLeaderToLearnerError
3d2d3718be4073e8c97a1e9eecee7b1d4b281619 etcdserver: exclude learner from leader transfer
cc8508d7a2469b944a1fec431f0c8346e4b7ae4b clientv3: add member promote
a1c0ea8636b6607baee91bc38b381aed22c17ae3 etcdserver: support MemberPromote for learner
01a7b3e8dcc29725e590dda2c03dbc83aa00bd38 integration: add TestKVForLearner
c8fbc4d50ef97bc2ba78f0e71f8b9b78de0494ad functional: fix MemberAdd call in tests.
2c19071219c8eeb6ef7bcc99c97a74a4d0d26757 etcdserver: filter rpc request to learner
c5001530d74b827c4949d490c7e523f3f91d112a *: add learner field in endpoint status

This PR is rebased to part 1 (#10725). Since we modified some function signatures in the end of part 1, I have to edit some of the commits in this PR so that they still make sense. - A little bit history rewrite of the learner feature branch. During this process, the following 2 commits (out of the total 16) are no longer needed, so they are dropped.

11e41685030846ba495a4263d8db352d0e9fc7d0 clientv3/integration: update MemberAdd call
c8fbc4d50ef97bc2ba78f0e71f8b9b78de0494ad functional: fix MemberAdd call in tests.

cc @xiang90

Added learner field to endpoint status API.

Hardcoded allowed rpc for learner node. Added filtering in grpc interceptor to check if rpc is allowed for learner node.

Adding TestKVForLearner. Also adding test utility functions for clientv3 integration tests.

1. Maintenance API MoveLeader() returns ErrBadLeaderTransferee if transferee does not exist or is raft learner. 2. etcdserver TransferLeadership() only choose voting member as transferee.

Adding integration test TestMoveLeaderToLearnerError, which ensures that leader transfer to learner member will fail.

Adding integration test TestTransferLeadershipWithLearner, which ensures that TransferLeadership does not timeout due to learner is automatically picked by leader as transferee.

Fixes 'gosimple' test.

Fixes go 'unconvert' test.

Fixes TestMemberAddForLearner and TestMemberPromoteForLearner.

Adding delay in the test for the newly started learner member to catch up applying config change entries in raft log.

xiang90 · 2019-05-15T21:33:57Z

etcdctl/ctlv3/command/member_command.go

+		Use:   "promote <memberID>",
+		Short: "Promotes a non-voting member in the cluster",
+		Long: `Promotes a non-voting learner member to a voting one in the cluster.
+`,


why a new line here?

I did not write this part of the code. But it seems consistent with the existing NewMemberListCommand.

xiang90 · 2019-05-15T21:36:20Z

etcdserver/api/membership/errors.go

+	ErrIDNotFound       = errors.New("membership: ID not found")
+	ErrPeerURLexists    = errors.New("membership: peerURL exists")
+	ErrMemberNotLearner = errors.New("membership: can only promote a learner member")
+	ErrLearnerNotReady  = errors.New("membership: can only promote a learner member which catches up with leader")


which is in sync with the leader

sounds good.

xiang90 · 2019-05-15T21:37:39Z

etcdserver/api/v3rpc/rpctypes/error.go

@@ -40,6 +40,8 @@ var (
 	ErrGRPCMemberNotEnoughStarted = status.New(codes.FailedPrecondition, "etcdserver: re-configuration failed due to not enough started members").Err()
 	ErrGRPCMemberBadURLs          = status.New(codes.InvalidArgument, "etcdserver: given member URLs are invalid").Err()
 	ErrGRPCMemberNotFound         = status.New(codes.NotFound, "etcdserver: member not found").Err()
+	ErrGRPCMemberNotLearner       = status.New(codes.FailedPrecondition, "etcdserver: can only promote a learner member").Err()
+	ErrGRPCLearnerNotReady        = status.New(codes.FailedPrecondition, "etcdserver: can only promote a learner member which catches up with leader").Err()


is in sync with leader

can we reuse the previously defined error message here? maybe define the error message as a const

can we reuse the previously defined error message here? maybe define the error message as a const

This is just following the exiting pattern. If we want to change it, it should be a separate PR for all existing errors.

is in sync with leader

sounds good.

for this one, a const is better since we repeat the same message twice. but it might also apply to other error messages. i am not sure.

xiang90 · 2019-05-15T21:39:10Z

etcdserver/api/v3rpc/util.go

@@ -116,3 +120,15 @@ func isClientCtxErr(ctxErr error, err error) bool {
 	}
 	return false
 }
+
+// in v3.4, learner is allowed to serve serializable read and endpoint status
+func isRPCEnabledForLearner(req interface{}) bool {


isRPCSupportedForLeaner ?

sounds good.

xiang90 · 2019-05-15T21:41:35Z

etcdserver/api/membership/cluster.go

+// ConfigChangeContext represents a context for confChange.
+type ConfigChangeContext struct {
+	Member
+	IsPromote bool `json:"isPromote"`


add a comment on this field?

xiang90 · 2019-05-15T21:42:04Z

etcdserver/api/membership/cluster.go

-			if urls[u] {
-				return ErrPeerURLexists
+		// A ConfChangeAddNode to a existing learner node promotes it to a voting member.
+		if confChangeContext.IsPromote {


move the validation to the top of this func?

we should do the validation first, then do the context encoding.

We need to encode confChangeContext first, which will tells us if the config change is for promoting a existing member, or for adding a new member. The validation logic is different for these two scenarios.

but then, can you move the decoding thing even before the urls construction?

Right, urls construction should be moved to later part of the logic. It is only needed for adding a new member case, not needed for promoting.

xiang90 · 2019-05-15T21:43:38Z

etcdserver/api/membership/cluster.go

@@ -693,3 +735,44 @@ func mustDetectDowngrade(lg *zap.Logger, cv *semver.Version) {
 		}
 	}
 }
+
+// IsLearner returns if the local member is raft learner
+func (c *RaftCluster) IsLearner() bool {


IsLocalMemberLearner

sounds good.

xiang90 · 2019-05-15T21:46:44Z

etcdserver/server.go

@@ -1435,20 +1440,20 @@ func (s *EtcdServer) TransferLeadership() error {
 		return nil
 	}

-	if !s.isMultiNode() {
+	if s.cluster == nil || len(s.cluster.VotingMemberIDs()) <= 1 {


probably still encapsulate this logic into a func.

xiang90 · 2019-05-15T21:47:44Z

etcdserver/server.go

+	// TODO add more checks whether the member can be promoted.
+	// like learner progress check or if cluster is ready to promote a learner
+	// this is an example to get progress
+	fmt.Printf("raftStatus, %#v\n", raftStatus())


we should not simply use fmt.printf as logging.

This part is unfinished. This line should be removed for now. The actual implementation is in later commit (not included in part 2 PR): dc79587

but checking this before the apply phase is ok.

but checking this before the apply phase is ok.

Not sure I understand. The member promote request should be rejected (if the reason is learner not ready) before it is appended to raft log. I think it is too late if we check learner progress in apply stage. In other words, applying a particular conf change raft entry should not depend on the progress of learner node in cluster. Some node may choose to apply this entry, whereas some node may not - depending on when they started applying this entry.

yes, that is what i mean. checking this at the apply phase will not work since the cluster might not have a consistent view on progress unless the leader keep on propagating this information.

This part is unfinished. This line should be removed for now. The actual implementation is in later commit (not included in part 2 PR):

can you just make this a noop or a TODO panic?

Currently it is noop - this checking function always return nil.

it will print something out, can we remove that.

Yes this print should be removed.

xiang90 · 2019-05-15T21:53:39Z

i did not give a look at all the tests changes. but the core path is good in general.

xiang90 · 2019-05-15T21:57:39Z

clientv3/integration/kv_test.go

+	defer cli.Close()
+
+	// waiting for learner member to catch up applying the config change entries in raft log.
+	time.Sleep(3 * time.Second)


it is not good to make this kind of timing assumption in general. we tried to get rid of all time.sleep in tests before.

This is replaced in later commits with server's ready notify channel. 90956f7 (not included in this part 2 PR)

add a todo?

xiang90 · 2019-05-15T22:00:59Z

tests look good too.

jingyih · 2019-05-15T22:17:50Z

I'll create an additional commit to address all the comments.

jingyih · 2019-05-15T23:05:30Z

@xiang90 Pushed an additional commit to address the comments. PTAL.

xiang90 · 2019-05-15T23:41:00Z

lgtm

jingyih and others added 14 commits May 15, 2019 13:13

*: add learner field in endpoint status

355d0ab

Added learner field to endpoint status API.

etcdserver: filter rpc request to learner

43ed94f

Hardcoded allowed rpc for learner node. Added filtering in grpc interceptor to check if rpc is allowed for learner node.

integration: add TestKVForLearner

57a11eb

Adding TestKVForLearner. Also adding test utility functions for clientv3 integration tests.

etcdserver: support MemberPromote for learner

ba9fd62

clientv3: add member promote

7f9479a

etcdserver: exclude learner from leader transfer

44d935e

1. Maintenance API MoveLeader() returns ErrBadLeaderTransferee if transferee does not exist or is raft learner. 2. etcdserver TransferLeadership() only choose voting member as transferee.

integration: add TestMoveLeaderToLearnerError

e8dc4c5

Adding integration test TestMoveLeaderToLearnerError, which ensures that leader transfer to learner member will fail.

integration: add TestTransferLeadershipWithLearner

bd7f428

Adding integration test TestTransferLeadershipWithLearner, which ensures that TransferLeadership does not timeout due to learner is automatically picked by leader as transferee.

clientv3, etcdctl: MemberPromote for learner

a039f2e

words: whitelist words to fix goword test.

c55519b

etcdserver: remove unnecessary bool comparison

c836e37

Fixes 'gosimple' test.

integration: remove unnecessary type conversion

ac05795

Fixes go 'unconvert' test.

clientv3/integration: fix cluster tests

b23c8f3

Fixes TestMemberAddForLearner and TestMemberPromoteForLearner.

clientv3/integration: deflake TestKVForLearner

90d28c0

Adding delay in the test for the newly started learner member to catch up applying config change entries in raft log.

xiang90 reviewed May 15, 2019

View reviewed changes

*: address comments

23f1d02

xiang90 merged commit d4cdbb1 into etcd-io:master May 15, 2019

jingyih mentioned this pull request May 16, 2019

*: support raft learner in etcd - part 3 #10730

Merged

jingyih mentioned this pull request Jun 1, 2019

*: support raft learner in etcd #10645

Closed

jingyih mentioned this pull request Jun 9, 2019

Support non-voting members in etcd server #9161

Closed

*: support raft learner in etcd - part 2 #10727

*: support raft learner in etcd - part 2 #10727

Conversation

jingyih commented May 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 commented May 15, 2019

Choose a reason for hiding this comment

jingyih May 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 commented May 15, 2019

jingyih commented May 15, 2019

jingyih commented May 15, 2019

xiang90 commented May 15, 2019

jingyih commented May 15, 2019 •

edited

Loading

jingyih May 15, 2019 •

edited

Loading