-
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: allow to update attributes of removed member #2939
Conversation
@@ -319,7 +319,13 @@ func (c *cluster) RemoveMember(id types.ID) { | |||
func (c *cluster) UpdateAttributes(id types.ID, attr Attributes) { | |||
c.Lock() | |||
defer c.Unlock() | |||
c.members[id].Attributes = attr | |||
if m, ok := c.members[id]; ok { |
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.
if m, ok := c.members[id]; ok {
m.Attributes = attr
return
}
_, ok := c.removed[id]
if ok {
...
} else {
...
}
LGTM |
4ab3cb0
to
46ebcff
Compare
It would be great if we can add a test case for it in integration test. |
7484df5
to
31494a5
Compare
unit test and integration test added. |
@@ -270,6 +270,33 @@ func TestIssue2746(t *testing.T) { | |||
clusterMustProgress(t, c.Members) | |||
} | |||
|
|||
// Ensure etcd can update attribute of removed member |
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.
Ensure etcd will not panic when removing a just started member.
31494a5
to
c84e378
Compare
rebased and improved comment in integration test. :) |
ma := client.NewMembersAPI(cc) | ||
ctx, cancel := context.WithTimeout(context.Background(), requestTimeout) | ||
// the proposal is not committed because member 1 is stopped, but the | ||
// proposal is written to leader's raft log. |
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.
is appended to
LGTM |
c84e378
to
8725e69
Compare
There exist the possiblity to update attributes of removed member in reasonable workflow: 1. start member A 2. leader receives the proposal to remove member A 2. member A sends the proposal of update its attribute to the leader 3. leader commits the two proposals So etcdserver should allow to update attributes of removed member.
etcdserver: allow to update attributes of removed member
I believe I am seeing this issue on 2.0.13. Will this fix be back-ported ? |
There exist the possiblity to update attributes of removed member in
reasonable workflow:
So etcdserver should allow to update attributes of removed member.
fixes case 1 and 3 of #2904
I have manually tested that the patch works.
@spacejam Could you double-check that it works?