-
Notifications
You must be signed in to change notification settings - Fork 9.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
etcdserver: forbid removing started member if quorum cannot be preserved in strict reconfig mode #3543
Conversation
829e004
to
2ad2806
Compare
Tests seem to be failing, I'll fix it ASAP. |
2ad2806
to
bba099e
Compare
Fixed tests, they are incorrect because of my misunderstanding. |
@@ -451,6 +451,7 @@ func TestApplyConfChangeError(t *testing.T) { | |||
srv := &EtcdServer{ | |||
r: raftNode{Node: n}, | |||
cluster: cl, | |||
cfg: &ServerConfig{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we move it to the first line of EtcdServer struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the cfg be placed at first line? Seems that after r (raftNode) would be suitable for the member. How do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to follow the field order in the struct: https://github.com/coreos/etcd/blob/master/etcdserver/server.go#L144
So the best place should be the second line actually. (I remembered the wrong order when i left the comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I got it.
LGTM. defer to @xiang90 |
{ | ||
// 1/2 members ready, should fail | ||
[]*Member{ | ||
newTestMember(1, nil, "1", nil), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is the second member? the comment says 1/2 members are ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, it is clearly my mistake... I'll fix it soon.
LGTM after fixing the comment. |
…ved in strict reconfig mode Like the commit 6974fc6, this commit lets etcdserver forbid removing started member if quorum cannot be preserved after reconfiguration if the option -strict-reconfig-check is passed to etcd. The removal can cause deadlock if unstarted members have wrong peer URLs.
bba099e
to
f8859a9
Compare
Updated based on the comments. |
LGTM |
etcdserver: forbid removing started member if quorum cannot be preserved in strict reconfig mode
@mitake My thought is to keep the code as it today and not move it to raftlog apply side. Several reasons:
|
@mitake I think the next step is to treat an inactive member the same as a unstarted one. It makes the checking better. |
…nfig -strict-reconfig-check option ensures a quorum cannot be losed by membership reconfiguration even if unstarted nodes are misconfigured (e.g. typos in peerURLs). As suggested in [1], this commit lets isReadyToAddMember() not to count unreachable nodes as started nodes. [1] etcd-io#3543 (comment)
…nfig -strict-reconfig-check option ensures a quorum cannot be losed by membership reconfiguration even if unstarted nodes are misconfigured (e.g. typos in peerURLs). As suggested in [1], this commit lets isReadyToAddMember() not to count unreachable nodes as started nodes. [1] etcd-io#3543 (comment)
…nfig -strict-reconfig-check option ensures a quorum cannot be losed by membership reconfiguration even if unstarted nodes are misconfigured (e.g. typos in peerURLs). As suggested in [1], this commit lets isReadyToAddMember() not to count unreachable nodes as started nodes. [1] etcd-io#3543 (comment)
Like the commit 6974fc6, this commit lets etcdserver forbid
removing started member if quorum cannot be preserved after
reconfiguration if the option -strict-reconfig-check is passed to
etcd. The removal can cause deadlock if unstarted members have wrong
peer URLs.