cluster: get name from PeerURL if member unready #568
Conversation
c797e66
to
89a6d61
Compare
@@ -19,4 +19,6 @@ import "errors" | |||
var ( | |||
errNoBackupExist = errors.New("no backup exist for disaster recovery") | |||
errInvalidMemberName = errors.New("the format of member's name is invalid") | |||
|
|||
errSelfHostedUnreadyMember = errors.New("self hosted shouldn't have any unready member. It has loop waited until member 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.
unexpected unready member for selfhosted cluster.
@@ -19,4 +19,6 @@ import "errors" | |||
var ( | |||
errNoBackupExist = errors.New("no backup exist for disaster recovery") | |||
errInvalidMemberName = errors.New("the format of member's name is invalid") | |||
|
|||
errSelfHostedUnreadyMember = errors.New("self hosted shouldn't have any unready member. It has loop waited until member 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.
ErrUnexpectedUnreadyMember
return errMemberNotReady | ||
var name string | ||
if c.cluster.Spec.SelfHosted != nil { | ||
name = m.Name |
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.
remove this assignment.
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.
We need to assign name in order figure out self hosted membership.
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
@@ -54,3 +56,11 @@ func RemoveMember(clientURLs []string, id uint64) error { | |||
cancel() | |||
return err | |||
} | |||
|
|||
func NameFromPeerURL(pu string) (string, error) { |
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.
MemberNameFromPeerURL
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.
Add a test?
"github.com/coreos/etcd-operator/pkg/spec" | ||
"github.com/coreos/etcd-operator/pkg/util/etcdutil" | ||
|
||
"k8s.io/kubernetes/pkg/api" | ||
) | ||
|
||
var errMemberNotReady = errors.New("etcd member's Name is empty. It's not ready yet.") | ||
|
||
func (c *Cluster) updateMembers(known etcdutil.MemberSet) error { | ||
resp, err := etcdutil.ListMembers(known.ClientURLs()) | ||
if err != nil { | ||
return err | ||
} | ||
members := etcdutil.MemberSet{} |
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.
can we move line29 to line58 into etcdutil (from resp to memberSet)? and add a unit test for it?
1289338
to
84ae4cb
Compare
I figured the part in member.go line 29-59 isn't very easy to refactor out as a separate func into etcdutil due to some error handling. Additionally, we shouldn't bloat this PR more with error handling restructuring code. We should probably do it in another PR |
13227fb
to
53b0400
Compare
import "testing" | ||
|
||
func TestMemberNameFromPeerURL(t *testing.T) { | ||
n := "test-cluster" |
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.
test both valid and invalid case.
@xiang90 tests added |
continue | ||
} | ||
if err != nil { | ||
t.Fatal(err) |
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.
t.errorf: want err = nil, got %v
lgtm |
Problem statement: We could fail in MemberAdd() due to context deadline exceeded and thus not creating related pod/service. But we could have added the member. Then we would end up infinitely looping on updateMembers() on "not ready member" error. One idea is that we can get name from member's PeerURL. For self hosted case, it's more complicated because it's host IP. We currently loop wait until the member has been added successfully. In the future we could use pod IP to match.
Problem statement: We could fail in MemberAdd() due to context deadline
exceeded and thus not creating related pod/service. But we could have
added the member. Then we would end up infinitely looping on updateMembers()
on "not ready member" error.
One idea is that we can get name from member's PeerURL.
For self hosted case, it's more complicated because it's host IP.
We currently loop wait until the member has been added successfully.
In the future we could use pod IP to match.