kubeadm: fixed etcd sync endpoints #71945

pytimer · 2018-12-11T08:57:57Z

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind bug

What this PR does / why we need it:

This PR fix kubeadm sync etcd endpoints problem.

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#1300

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kubeadm: fix a bug when syncing etcd endpoints

k8s-ci-robot · 2018-12-11T08:58:04Z

Hi @pytimer. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rosti

Well spotted @pytimer ! Thank you!
Can you also create cherry-pick PR for 1.13 once this one merges?

/lgtm
/ok-to-test
/kind bug
/assign @fabriziopandini
/assign @neolit123

rosti · 2018-12-11T09:20:58Z

/priority important-soon

rosti · 2018-12-11T10:36:24Z

/test pull-kubernetes-e2e-gce-100-performance

neolit123

@pytimer
thanks, added a comment about pointers vs non-pointers.

neolit123 · 2018-12-11T12:19:23Z

cmd/kubeadm/app/util/etcd/etcd.go


 	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{


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.

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

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

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.

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.

@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.

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

@fabriziopandini @neolit123

Fix like:

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

Is it ok?

@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.

@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.

k8s-ci-robot · 2018-12-11T14:51:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, pytimer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kubeadm/OWNERS~~ [neolit123]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pytimer · 2018-12-12T11:28:56Z

Thanks for you reply. I understand your said. If my tone looks a little impatient, sorry.I am willing to patiently wait for the way of the better solution.

If have the best solution about it, I will implement it. But maybe time zone different from yours, comments can not reply immediately.

neolit123 · 2018-12-12T13:19:06Z

@pytimer
added a comment here: #71945 (comment)

please also make the release note in the OP:
kubeadm: fix a bug when syncing etcd endpoints
instead of NONE.

thanks.

pytimer · 2018-12-13T01:43:09Z

@neolit123 @fabriziopandini @rosti

I bring back (c *Client) Sync(), and edit comment according to @neolit123 feedback. Can you check it out?

Please let me know if i need to do other things. thanks.

neolit123 · 2018-12-13T01:56:02Z

@pytimer
thanks for the update.

pytimer · 2018-12-13T02:48:45Z

/test pull-kubernetes-e2e-gce-100-performance

fabriziopandini · 2018-12-13T08:56:27Z

@pytimer thanks!
/hold cancel

rosti · 2018-12-13T09:45:51Z

@pytimer looks superb again! Thank you for this!
We have to do a cherry-pick of this change for 1.13. If you can't handle this yourself, I can do it. Just let me know.

/lgtm

pytimer · 2018-12-13T10:20:17Z

@rosti I have no experience in using cherry-pick on the kubernetes repository, so can you cherry-pick this change? Thanks.

Can you tell me how to cherry-pick on the kubernetes repository if you have time? I want to study it and if there is a similar operation in the future, i can do it. Thanks.

rosti · 2018-12-13T11:55:32Z

@pytimer please, follow this guide. It's really simple. If you have issues you can ping me to do it. Thanks!
https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md

rosti · 2018-12-13T14:31:41Z

/test pull-kubernetes-godeps

fejta-bot · 2018-12-13T16:43:05Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-12-13T18:49:05Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-12-13T20:55:06Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

pytimer · 2018-12-14T01:30:25Z

/test pull-kubernetes-e2e-kops-aws

pytimer · 2018-12-14T03:00:01Z

/test pull-kubernetes-e2e-kops-aws

pytimer · 2018-12-14T04:48:48Z

/test pull-kubernetes-integration

pytimer · 2018-12-14T04:51:02Z

/test pull-kubernetes-e2e-kops-aws

pytimer · 2018-12-14T06:05:40Z

@rosti Thanks for you help about cherry-pick. I already cherry-pick this PR.

Cherry pick is #72042.

…5-upstream-release-1.13 Automated cherry pick of #71945: kubeadm: fixed etcd sync endpoints

kubeadm: fixed etcd sync endpoints

48d757b

k8s-ci-robot requested review from kad and liztio December 11, 2018 08:58

pytimer mentioned this pull request Dec 11, 2018

kubeadm reset success but this node ip still in kubeadm-config configmap kubernetes/kubeadm#1300

Closed

k8s-ci-robot assigned rosti, fabriziopandini and neolit123 Dec 11, 2018

rosti approved these changes Dec 11, 2018

View reviewed changes

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 11, 2018

neolit123 reviewed Dec 11, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2018

pytimer force-pushed the kubeadm-etcd-fix branch from ca1fdbc to 48d757b Compare December 13, 2018 01:35

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 13, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 13, 2018

k8s-ci-robot merged commit 1c04caa into kubernetes:master Dec 14, 2018

pytimer mentioned this pull request Dec 14, 2018

Automated cherry pick of #71945: kubeadm: fixed etcd sync endpoints #72042

Merged

tossmilestone mentioned this pull request Dec 20, 2018

kubeadm: fixed etcd sync endpoints alauda/kubernetes#38

Merged

k8s-ci-robot added a commit that referenced this pull request Dec 28, 2018

Merge pull request #72042 from pytimer/automated-cherry-pick-of-#7194…

dc5b1c5

…5-upstream-release-1.13 Automated cherry pick of #71945: kubeadm: fixed etcd sync endpoints

pytimer deleted the kubeadm-etcd-fix branch February 22, 2019 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: fixed etcd sync endpoints #71945

kubeadm: fixed etcd sync endpoints #71945

pytimer commented Dec 11, 2018 •

edited

k8s-ci-robot commented Dec 11, 2018

rosti left a comment

rosti commented Dec 11, 2018

rosti commented Dec 11, 2018

neolit123 left a comment

neolit123 Dec 11, 2018

pytimer Dec 11, 2018

neolit123 Dec 11, 2018

pytimer Dec 11, 2018

neolit123 Dec 11, 2018

fabriziopandini Dec 11, 2018

fabriziopandini Dec 11, 2018 •

edited

pytimer Dec 11, 2018

rosti Dec 12, 2018

neolit123 Dec 12, 2018 •

edited

k8s-ci-robot commented Dec 11, 2018

pytimer commented Dec 12, 2018

neolit123 commented Dec 12, 2018

pytimer commented Dec 13, 2018

neolit123 commented Dec 13, 2018

pytimer commented Dec 13, 2018

fabriziopandini commented Dec 13, 2018

rosti commented Dec 13, 2018

pytimer commented Dec 13, 2018

rosti commented Dec 13, 2018

rosti commented Dec 13, 2018

fejta-bot commented Dec 13, 2018

fejta-bot commented Dec 13, 2018

fejta-bot commented Dec 13, 2018

pytimer commented Dec 14, 2018

pytimer commented Dec 14, 2018

pytimer commented Dec 14, 2018

pytimer commented Dec 14, 2018

pytimer commented Dec 14, 2018

kubeadm: fixed etcd sync endpoints #71945

kubeadm: fixed etcd sync endpoints #71945

Conversation

pytimer commented Dec 11, 2018 • edited

k8s-ci-robot commented Dec 11, 2018

rosti left a comment

Choose a reason for hiding this comment

rosti commented Dec 11, 2018

rosti commented Dec 11, 2018

neolit123 left a 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

fabriziopandini Dec 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Dec 12, 2018 • edited

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 11, 2018

pytimer commented Dec 12, 2018

neolit123 commented Dec 12, 2018

pytimer commented Dec 13, 2018

neolit123 commented Dec 13, 2018

pytimer commented Dec 13, 2018

fabriziopandini commented Dec 13, 2018

rosti commented Dec 13, 2018

pytimer commented Dec 13, 2018

rosti commented Dec 13, 2018

rosti commented Dec 13, 2018

fejta-bot commented Dec 13, 2018

fejta-bot commented Dec 13, 2018

fejta-bot commented Dec 13, 2018

pytimer commented Dec 14, 2018

pytimer commented Dec 14, 2018

pytimer commented Dec 14, 2018

pytimer commented Dec 14, 2018

pytimer commented Dec 14, 2018

pytimer commented Dec 11, 2018 •

edited

fabriziopandini Dec 11, 2018 •

edited

neolit123 Dec 12, 2018 •

edited