Skip to content
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.

Sign up for GitHub

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

Jump to bottom

kubeadm: fixed etcd sync endpoints #71945

Merged
merged 1 commit into from
Dec 14, 2018
Merged

kubeadm: fixed etcd sync endpoints #71945

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion cmd/kubeadm/app/util/etcd/etcd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,12 +190,13 @@ func NewFromCluster(client clientset.Interface, certificatesDir string) (*Client
if err != nil {
return nil, errors.Wrap(err, "error syncing endpoints with etc")
}
klog.V(1).Infof("update etcd endpoints: %s", strings.Join(etcdClient.Endpoints, ","))

return etcdClient, nil
}

// Sync synchronizes client's endpoints with the known endpoints from the etcd membership.
func (c Client) Sync() error {
func (c *Client) Sync() error {
cli, err := clientv3.New(clientv3.Config{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having a single method that accepts a pointer and the rest accepting non-pointers seems a bit odd.
we might want to make all of them accept pointer... see GetVersion(), GetClusterVersions() etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean that Sync() return the real etcd endpoints? such as:

func (c Client) Sync() ([]string, error) {}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the methods for Client have different signatures:
func (c *Client) Sync()
func (c Client) AddMember(name string, peerAddrs string) ([]Member, error) {
...

Sync() uses a pointer, but the rest of the methods use non-pointers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sync() only return error now, but the rest of the methods return multi values.
Can change this Sync() method return endpoints and error, like other methods?
The NewFromCluster invoke Sync() like below:

endpoints, err = etcdClient.Sync()
if err != nil {
	return nil, errors.Wrap(err, "error syncing endpoints with etc")
}
etcdClient.Endpoints = endpoints

In this case, all methods use non-pointers, and only change Sync() methods, but test case maybe need to update.

The other ways, the methods of Client all change non-pointers to pointer, the way need to change more codes.

I am not sure if i understand what you mean？If wrong, can you explain your comments again. Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pytimer my point is this:

func (c *Client) Sync(...) ...
func (c Client) AddMember(...) ...
...

^ see how c is a pointer in Sync but not a pointer in the rest of methods for the Client type.
we need to make all the methods consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pytimer please kindly fix as discussed above to address @neolit123 feedback and then I will leave hold

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabriziopandini @neolit123

Fix like:

func (c Client) Sync() ([]string, error) {}

Is it ok?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pytimer @fabriziopandini @neolit123 I am not OK with func (c Client) Sync() ([]string, error). This is no longer a Sync method. It's not even a GetEndpoints method (because we have Endpoints member in Client). Furthermore, this introduces a code smell (inappropriate intimacy), where NewFromCluster now updates field inside Client.

This is a lot worse solution to the problem it tries to solve. I am much more in favor of making all methods func (c *Client), than have this. I am also in favor of having only Sync() take a pointer to Client.

Copy link
Member

@neolit123 neolit123 Dec 12, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pytimer
OK, i missed the part that we want to modify the existing Client object and not a copy.
please bring back (c *Client) Sync() .

we will then cherry pick this PR for 1.13.
and we can have another PR that refactors all methods to use pointers.

Endpoints: c.Endpoints,
DialTimeout: 20 * time.Second,
Expand Down