*: support raft learner in etcd #10645

jingyih · 2019-04-15T03:53:28Z

Edit: This PR is replaced by #10725, #10727, #10730.

This PR adds support for learner (non-voting) member in etcd. Currently the basic functionality is done. Feels like this might be a good time to get reviews before the changes grow bigger. Please refer to the pull requests against learner branch for incremental changes and their context.

Background

Design doc for raft learner (non-voting member)
Original issue Support non-voting members in etcd server #9161

Status
The implementation status is tracked in task list #10537.

Main contributors to learner branch:
@jingyih @WIZARD-CXY @gyuho @jpbetz

codecov-io · 2019-04-15T04:40:22Z

Codecov Report

Merging #10645 into master will decrease coverage by 0.14%.
The diff coverage is 68.28%.

@@            Coverage Diff             @@
##           master   #10645      +/-   ##
==========================================
- Coverage    71.8%   71.66%   -0.15%     
==========================================
  Files         393      393              
  Lines       36628    36875     +247     
==========================================
+ Hits        26302    26425     +123     
- Misses       8502     8599      +97     
- Partials     1824     1851      +27

Impacted Files	Coverage Δ
etcdserver/api/v3rpc/rpctypes/error.go	`90.47% <ø> (ø)`	⬆️
etcdserver/errors.go	`0% <ø> (ø)`	⬆️
etcdserver/api/membership/errors.go	`100% <ø> (ø)`	⬆️
etcdctl/ctlv3/command/printer_fields.go	`0% <0%> (ø)`	⬆️
etcdserver/api/v2v3/server.go	`0% <0%> (ø)`	⬆️
proxy/grpcproxy/adapter/cluster_client_adapter.go	`16.66% <0%> (-3.34%)`	⬇️
etcdctl/ctlv3/command/printer_simple.go	`72.84% <0%> (+0.36%)`	⬆️
clientv3/retry.go	`87.5% <100%> (+0.22%)`	⬆️
etcdctl/ctlv3/command/ep_command.go	`63.57% <100%> (ø)`	⬆️
etcdserver/api/v3rpc/maintenance.go	`76.92% <100%> (+1.92%)`	⬆️
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f29b1ad...f852a94. Read the comment docs.

gyuho · 2019-04-18T23:00:18Z

clientv3/cluster.go

 )

 type Cluster interface {
 	// MemberList lists the current cluster membership.
 	MemberList(ctx context.Context) (*MemberListResponse, error)

 	// MemberAdd adds a new member into the cluster.
-	MemberAdd(ctx context.Context, peerAddrs []string) (*MemberAddResponse, error)
+	MemberAdd(ctx context.Context, peerAddrs []string, isLearner bool) (*MemberAddResponse, error)


@xiang90 @jpbetz Sorry for late response. Should we create a new RPC, rather than breaking API? I am fine with changing the function signature, and just release notes.

jingyih · 2019-05-07T20:54:15Z

Removing WIP label, as most of the items in task list are done.

xiang90 · 2019-05-09T22:52:56Z

@jingyih Can you improve the commit history a little bit? Right now they are 60 commits. For example separate them into 3-5 commits that can be reviewed independently? Thanks!

jingyih · 2019-05-10T01:40:05Z

@jingyih Can you improve the commit history a little bit? Right now they are 60 commits. For example separate them into 3-5 commits that can be reviewed independently? Thanks!

@xiang90

I agree there are a lot of commits. Half of these commits are merge commits, which can be ignored. What happened was we first created a feature branch, and all the code changes were first merged into the feature branch. There are about 30 PRs opened against feature branch, they closely match the items in task list #10537.

Reviewing 30 commits is not easy, rebasing and grouping them into 3 to 5 commits is not easy neither. I can pick the most important commits (maybe 15 to 20 commits) and list them in groups. Is this acceptable?

xiang90 · 2019-05-10T06:12:45Z

@jingyih OK. Let us get rid of the merge commits first.

- Added isLearner flag to MemberAddRequest in Cluster API. - Added isLearner field to StatusResponse in Maintenance API. - Added MemberPromote rpc to Cluster API.

Fixed compilation erros after API change for learner.

Added IsLearner field to etcdserver internal Member type. Routed learner MemberAdd request from server API to raft. Apply learner MemberAdd result to server after the request is passed through Raft.

Added IsLearner flag to clientv3 MemberAdd API.

Added support for "etcdctl member add --learner".

Added an e2e test to exercise "etcdctl member add --learner".

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.

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.

Adjust StrictReconfigCheck logic to accommodate learner members in the cluster.

Make learner return code.Unavailable when the request is not supported by learner. Client balancer will retry a different endpoint.

If member does not exist in cluster, IsLearner will panic.

Hard-coded the maximum number of learners to 1.

Use ReadyNotify instead of time.Sleep to wait for server ready.

If learner is not ready to be promoted, use etcdserver.ErrLearnerNotReady instead of using membership.ErrLearnerNotReady.

Check http StatusCode. Only Unmarshal body if StatusCode is statusOK.

Update TestMemberPromote to include both learner not-ready and learner ready test cases. Removed unit test TestPromoteMember, it requires underlying raft node to be started and running. The member promote is covered by the integration test.

jingyih · 2019-05-11T00:34:22Z

@jingyih OK. Let us get rid of the merge commits first.

@xiang90 Done.

xiang90 · 2019-05-14T17:25:22Z

etcdserver/api/membership/member.go

@@ -51,10 +53,13 @@ type Member struct {

 // NewMember creates a Member without an ID and generates one based on the
 // cluster name, peer URLs, and time. This is used for bootstrapping/adding new member.
-func NewMember(name string, peerURLs types.URLs, clusterName string, now *time.Time) *Member {
+func NewMember(name string, peerURLs types.URLs, clusterName string, now *time.Time, isLearner bool) *Member {


probably add a func called NewLearnerMember. Boolean arg on public method is not good in general.

xiang90 · 2019-05-14T17:27:20Z

@jingyih I reviewed the first 7 commits (up to etcdctl: add learner field in member list output). Can you make a separate PR for these 7 commits? I think they can be merged separately first.

jingyih · 2019-05-14T20:14:34Z

@jingyih I reviewed the first 7 commits (up to etcdctl: add learner field in member list output). Can you make a separate PR for these 7 commits? I think they can be merged separately first.

@xiang90
Thanks for reviewing! A separate PR for the first 7 commits is created #10725.

But do we want to merge only part of the learner commits into master? I though it might cause confusion?

xiang90 · 2019-05-14T20:21:08Z

clientv3/cluster.go

@@ -17,10 +17,11 @@ package clientv3
 import (
 	"context"



the import format needs to be fixed.

Sure. FYI, it is not in the final version. "error" is imported here temporarily because the MemberPromote API returns error "not implemented". The "error" import is removed when the actual implementation is done.

xiang90 · 2019-05-14T20:22:32Z

etcdserver/api/membership/cluster.go

@@ -252,6 +252,16 @@ func (c *RaftCluster) Recover(onSet func(*zap.Logger, *semver.Version)) {
 	}
 }

+// IsPromoteChange checks if m is a promoteChange
+func (c *RaftCluster) IsPromoteChange(m *Member) bool {


IsMemberPromoted

This function checks if the config change is for promoting a member (as opposed to adding a member). Maybe use IsMemberPromoteConfChange or IsConfChangeForMemberPromote?

xiang90 · 2019-05-14T20:23:46Z

etcdserver/api/membership/cluster.go

@@ -252,6 +252,16 @@ func (c *RaftCluster) Recover(onSet func(*zap.Logger, *semver.Version)) {
 	}
 }

+// IsPromoteChange checks if m is a promoteChange
+func (c *RaftCluster) IsPromoteChange(m *Member) bool {
+	members, _ := membersFromStore(c.lg, c.v2store)


why do we need to load member from c.v2store, not from c.members directly?

Members info from store is more "accurate" then c.members. In most cases they are the same, except during server bootstrapping.

During server bootstrapping, we construct members information from "--init-cluster". The resulting members info does not have any information on member's learner flag. Because we do not provide a learner flag when starting etcd binary.

Also check here, in current code we load members from store when validating the conf change in apply stage, it is for very similar reasons.

etcd/etcdserver/api/membership/cluster.go

Lines 255 to 256 in a44a281

func (c *RaftCluster) ValidateConfigurationChange(cc raftpb.ConfChange) error {

members, removed := membersFromStore(c.lg, c.v2store)

Maybe I should be more specific on this topic. Consider the case where we add a new member to an existing cluster.

Call MemberAdd API to add new member to existing members in cluster.
1.1 Each existing member append confchange raft entry of adding the new member.
1.2 Each existing member apply the raft entry which adds the new member to its c.members and backend storage.

When starting the new member (during bootstrapping), new member's c.members is initially constructed from the input parameter "--init-cluster", whereas initally the backend storage has no members information. At this point, new member's c.members is almost accurate except that it does not have info on learner.

As the new member getting raft entries from leader and applying them, it will eventually apply all the confchange raft entries, including the entry that adds itself to the cluster. At this point, its c.members is accurate.

During bootstrapping, apply stage should always refer to member info form backend storage. Otherwise server gets confused when applying the confchange raft entries: adding a member which already exists in its c.members.

BTW, just realized this function is not part of the final version. We had to come up with a better way of deciding (in apply stage) if a confchange is for adding new member, or for promoting an existing member.
https://github.com/jingyih/etcd/blob/b433162c0b9ea4d13e9a0dd49b1f51e20896cbcc/etcdserver/server.go#L2232

ok. then probably replace this part of the PR with the final version :)

It will be removed in later commits. You will not see this function in part2 PR.

xiang90 · 2019-05-15T05:40:05Z

@jingyih Can you create a part2 PR? I feel the next 10 commits are good candidates.

jingyih · 2019-05-15T05:57:19Z

@jingyih Can you create a part2 PR? I feel the next 10 commits are good candidates.

I can do it tomorrow.

xiang90 · 2019-05-15T23:56:34Z

@jingyih Probably the remaining commits can be easily reviewed in one PR?

jingyih · 2019-05-16T00:08:37Z

@jingyih Probably the remaining commits can be easily reviewed in one PR?

Yeah I am creating the last part - part 3.

gyuho requested review from gyuho, jpbetz and xiang90 April 15, 2019 17:55

gyuho added Release-Note type/feature labels Apr 15, 2019

maoling mentioned this pull request Apr 16, 2019

Add an observer role #10647

Closed

jingyih mentioned this pull request Apr 17, 2019

Task list: support raft learner in etcd. #10537

Closed

33 tasks

gyuho reviewed Apr 18, 2019

View reviewed changes

jingyih changed the title ~~[WIP] *: support raft learner in etcd~~ *: support raft learner in etcd May 7, 2019

jingyih added this to the etcd-v3.4 milestone May 7, 2019

jingyih and others added 15 commits May 10, 2019 16:42

*: Change etcdserver API to support raft learner

087f98c

- Added isLearner flag to MemberAddRequest in Cluster API. - Added isLearner field to StatusResponse in Maintenance API. - Added MemberPromote rpc to Cluster API.

*: fix compilation after API change

229391c

Fixed compilation erros after API change for learner.

etcdserver: support MemberAdd for learner

d29f066

Added IsLearner field to etcdserver internal Member type. Routed learner MemberAdd request from server API to raft. Apply learner MemberAdd result to server after the request is passed through Raft.

clientv3: support MemberAdd for learner

699a7d8

Added IsLearner flag to clientv3 MemberAdd API.

etcdctl: support MemberAdd for learner

e9c923a

Added support for "etcdctl member add --learner".

tests/e2e: Add test for learner member add

a4af2f9

Added an e2e test to exercise "etcdctl member add --learner".

etcdctl: add learner field in member list output

3108914

*: add learner field in endpoint status

c500153

Added learner field to endpoint status API.

etcdserver: filter rpc request to learner

2c19071

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

functional: fix MemberAdd call in tests.

c8fbc4d

integration: add TestKVForLearner

01a7b3e

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

etcdserver: support MemberPromote for learner

a1c0ea8

clientv3: add member promote

cc8508d

etcdserver: exclude learner from leader transfer

3d2d371

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

2db0ca9

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

jingyih and others added 14 commits May 10, 2019 16:42

clientv3/integration: fix cluster tests

1a08038

Fixes TestMemberAddForLearner and TestMemberPromoteForLearner.

clientv3/integration: deflake TestKVForLearner

f058b7f

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

etcdserver: adjust StrictReconfigCheck

d93fecc

Adjust StrictReconfigCheck logic to accommodate learner members in the cluster.

etcdserver: learner return Unavailable for unsupported RPC

6d8abba

Make learner return code.Unavailable when the request is not supported by learner. Client balancer will retry a different endpoint.

etcdserver: check IsMemberExist before IsLearner

678d86e

If member does not exist in cluster, IsLearner will panic.

etcdserver: allow 1 learner in cluster

2e87a9a

Hard-coded the maximum number of learners to 1.

clientv3/integration: better way to deflake test

90956f7

Use ReadyNotify instead of time.Sleep to wait for server ready.

Doc: add learner in runtime-configuration.md

c629884

etcdserver: add mayPromote check

dc79587

etcdserver: forward member promote to leader

a8f87b8

etcdserver: use etcdserver ErrLearnerNotReady

ec1928e

If learner is not ready to be promoted, use etcdserver.ErrLearnerNotReady instead of using membership.ErrLearnerNotReady.

etcdserver: check http StatusCode before unmarshal

3f937cc

Check http StatusCode. Only Unmarshal body if StatusCode is statusOK.

etcdserver: update raftStatus

ae08d06

integration: update TestMemberPromote test

b433162

Update TestMemberPromote to include both learner not-ready and learner ready test cases. Removed unit test TestPromoteMember, it requires underlying raft node to be started and running. The member promote is covered by the integration test.

jingyih force-pushed the learner branch from f852a94 to b433162 Compare May 11, 2019 00:32

xiang90 reviewed May 14, 2019

View reviewed changes

jingyih mentioned this pull request May 14, 2019

*: support raft learner in etcd - part 1 #10725

Merged

xiang90 reviewed May 14, 2019

View reviewed changes

jingyih mentioned this pull request May 15, 2019

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

Merged

jingyih mentioned this pull request May 16, 2019

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

Merged

jingyih closed this May 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: support raft learner in etcd #10645

*: support raft learner in etcd #10645

jingyih commented Apr 15, 2019 •

edited

Loading

codecov-io commented Apr 15, 2019 •

edited

Loading

gyuho Apr 18, 2019

jingyih commented May 7, 2019

xiang90 commented May 9, 2019

jingyih commented May 10, 2019 •

edited

Loading

xiang90 commented May 10, 2019

jingyih commented May 11, 2019 •

edited

Loading

xiang90 May 14, 2019

xiang90 commented May 14, 2019

jingyih commented May 14, 2019

xiang90 May 14, 2019

jingyih May 14, 2019

xiang90 May 14, 2019

jingyih May 14, 2019

xiang90 May 14, 2019

jingyih May 14, 2019

jingyih May 15, 2019

jingyih May 15, 2019

xiang90 May 15, 2019

jingyih May 15, 2019

xiang90 May 15, 2019

xiang90 commented May 15, 2019

jingyih commented May 15, 2019

xiang90 commented May 15, 2019

jingyih commented May 16, 2019

	func (c *RaftCluster) ValidateConfigurationChange(cc raftpb.ConfChange) error {
	members, removed := membersFromStore(c.lg, c.v2store)

*: support raft learner in etcd #10645

*: support raft learner in etcd #10645

Conversation

jingyih commented Apr 15, 2019 • edited Loading

codecov-io commented Apr 15, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

jingyih commented May 7, 2019

xiang90 commented May 9, 2019

jingyih commented May 10, 2019 • edited Loading

xiang90 commented May 10, 2019

jingyih commented May 11, 2019 • edited Loading

Choose a reason for hiding this comment

xiang90 commented May 14, 2019

jingyih commented May 14, 2019

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

jingyih commented May 15, 2019

xiang90 commented May 15, 2019

jingyih commented May 16, 2019

jingyih commented Apr 15, 2019 •

edited

Loading

codecov-io commented Apr 15, 2019 •

edited

Loading

jingyih commented May 10, 2019 •

edited

Loading

jingyih commented May 11, 2019 •

edited

Loading