Fix setNodeAddress when a node IP and a cloud provider are set #49202

cbonte · 2017-07-19T13:37:07Z

What this PR does / why we need it:
When a node IP is set and a cloud provider returns the same address with
several types, only the first address was accepted. With the changes made
in PR #45201, the vSphere cloud provider returned the ExternalIP first,
which led to a node without any InternalIP.

The behaviour is modified to return all the address types for the
specified node IP.

Which issue this PR fixes: fixes #48760

Special notes for your reviewer:

I'm not a golang expert, is it possible to mock kubelet.validateNodeIP() to avoid the need of real host interface addresses in the test ?
It would be great to have it backported for a next 1.6.8 release.

Release note:

Fix for Nodes in vSphere lacking an InternalIP. (#48760)

k8s-ci-robot · 2017-07-19T13:37:15Z

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

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. I understand the commands that are listed here.

spxtr · 2017-07-19T19:54:35Z

/lint

k8s-ci-robot

@spxtr: no lint warnings.

In response to this:

/lint

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.

dims · 2017-08-09T10:48:07Z

/ok-to-test

cbonte · 2017-08-10T12:21:44Z

/retest

cbonte · 2017-08-10T13:16:00Z

/assign @Random-Liu

cbonte · 2017-08-28T11:33:23Z

/retest

cbonte · 2017-08-28T12:52:56Z

PR has been rebased according to current master branch.

jhorwit2 · 2017-08-28T14:35:42Z

Does this issue also occur in the cloud controller manager?

jhorwit2 · 2017-08-28T14:37:38Z

cc @wlan0 @luxas

wlan0 · 2017-08-28T21:33:08Z

@cbonte Thanks for the PR.

It LGTM, except for the part where host IP addresses are used.

I'm not a golang expert, is it possible to mock kubelet.validateNodeIP() to avoid the need of real host interface addresses in the test ?

I don't think there is a simple answer to this. You could "inject" a interface here - https://github.com/cbonte/kubernetes/blob/25767ae38906ca7dc00c6c6a2935de583c12eaa5/pkg/kubelet/kubelet.go#L846

and in the test, while creating the kubelet object (newTestKubelet method), pass a mock struct to that interface. This is quite a bit of refactoring!

Tbh, I'm not entirely sure that relying on the host IP addresses is a bad idea. If that is ok, then we don't need any of this refactoring, and the current tests will suffice.

@luxas what would you suggest?

cbonte · 2017-08-29T14:12:44Z

Tbh, I'm not entirely sure that relying on the host IP addresses is a bad idea. If that is ok, then we don't need any of this refactoring, and the current tests will suffice.

I tend to agree.

Note : I'm currently rebasing the PR again, as some changes since yesterday introduce new conflicts.

calebamiles · 2017-09-05T15:40:40Z

@luxas, sorry this was a bulk remove from the milestone since the PR was unmergable at the time of code freeze (missing either, lgtm, or approve, or had test failures). Added the PR back to the milestone but the PR itself is still unmergable

calebamiles · 2017-09-05T15:40:46Z

/retest

liggitt · 2017-09-05T19:56:44Z

flake on #50695
/test pull-kubernetes-e2e-gce-etcd3

dims · 2017-09-06T01:25:53Z

/test pull-kubernetes-e2e-gce-etcd3

liggitt · 2017-09-06T04:09:06Z

/retest

dims · 2017-09-06T11:14:40Z

@Random-Liu @vishh @tallclair @dchen1107 - Can one of you please approve?

cheftako

/lgtm

yujuhong · 2017-09-07T01:50:00Z

pkg/kubelet/kubelet_node_status_test.go

+		Spec:       v1.NodeSpec{},
+	}
+
+	// TODO : is it possible to mock kubelet.validateNodeIP() to avoid relying on the host interface addresses ?


I looked at validateNodeIP() and there is nothing to prevent it from being a standalone function. I think that'd make testing much easier.

yujuhong · 2017-09-07T01:56:02Z

/approve

k8s-github-robot · 2017-09-07T01:56:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cbonte, cheftako, liggitt, yujuhong

Associated issue: 45201

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubelet/OWNERS~~ [yujuhong]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-09-07T03:00:58Z

Automatic merge from submit-queue (batch tested with PRs 51728, 49202)

cbonte · 2017-09-07T11:22:40Z

Hi @yujuhong, is it possible to make this PR as a cherry-pick candidate for release-1.6 ?
Thanks !

yujuhong · 2017-09-07T17:28:59Z

@cbonte you can submit a cherrypick PR.

…-upstream-release-1.6 Automatic merge from submit-queue Automated cherry pick of #49202 Cherry pick of #49202 on release-1.6. #49202: Fix setNodeAddress when a node IP and a cloud provider are

…-upstream-release-1.7 Automatic merge from submit-queue. Automated cherry pick of #49202 Cherry pick of #49202 on release-1.7. #49202: Fix setNodeAddress when a node IP and a cloud provider are

k8s-cherrypick-bot · 2017-10-03T08:26:13Z

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-github-robot assigned Random-Liu and euank Jul 19, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 19, 2017

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jul 19, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 19, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2017

k8s-ci-robot reviewed Jul 19, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 9, 2017

cbonte force-pushed the node-addresses branch from 5c78a02 to ca97c2e Compare August 10, 2017 11:14

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2017

cbonte force-pushed the node-addresses branch 2 times, most recently from b7e7da4 to 9749d4b Compare August 28, 2017 09:56

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 28, 2017

cbonte force-pushed the node-addresses branch from 9749d4b to 25767ae Compare August 28, 2017 10:38

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 28, 2017

cbonte force-pushed the node-addresses branch from 25767ae to 5fd0962 Compare August 29, 2017 14:25

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2017

calebamiles added this to the v1.8 milestone Sep 5, 2017

cheftako approved these changes Sep 6, 2017

View reviewed changes

yujuhong reviewed Sep 7, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 7, 2017

k8s-github-robot merged commit a51eb2a into kubernetes:master Sep 7, 2017

This was referenced Sep 8, 2017

Automated cherry pick of #49202 #52156

Merged

Automated cherry pick of #49202 #52157

Merged

enisoc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 8, 2017

wojtek-t modified the milestones: v1.8, v1.7 Oct 3, 2017

wojtek-t added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cherrypick-candidate labels Oct 3, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Oct 3, 2017

danwinship mentioned this pull request Nov 8, 2018

Fix a CloudProvider-vs-nodeIP edge case #70805

Merged

liggitt mentioned this pull request Jul 11, 2019

Fix cloud reported hostname being overridden if nodeIP set #80003

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix setNodeAddress when a node IP and a cloud provider are set #49202

Fix setNodeAddress when a node IP and a cloud provider are set #49202

cbonte commented Jul 19, 2017 •

edited by enisoc

Loading

k8s-ci-robot commented Jul 19, 2017

spxtr commented Jul 19, 2017

k8s-ci-robot left a comment

dims commented Aug 9, 2017

cbonte commented Aug 10, 2017

cbonte commented Aug 10, 2017

cbonte commented Aug 28, 2017

cbonte commented Aug 28, 2017

jhorwit2 commented Aug 28, 2017

jhorwit2 commented Aug 28, 2017

wlan0 commented Aug 28, 2017

cbonte commented Aug 29, 2017

calebamiles commented Sep 5, 2017

calebamiles commented Sep 5, 2017

liggitt commented Sep 5, 2017

dims commented Sep 6, 2017

liggitt commented Sep 6, 2017

dims commented Sep 6, 2017

cheftako left a comment

yujuhong Sep 7, 2017

yujuhong commented Sep 7, 2017

k8s-github-robot commented Sep 7, 2017

k8s-github-robot commented Sep 7, 2017

cbonte commented Sep 7, 2017

yujuhong commented Sep 7, 2017

k8s-cherrypick-bot commented Oct 3, 2017

Fix setNodeAddress when a node IP and a cloud provider are set #49202

Fix setNodeAddress when a node IP and a cloud provider are set #49202

Conversation

cbonte commented Jul 19, 2017 • edited by enisoc Loading

k8s-ci-robot commented Jul 19, 2017

spxtr commented Jul 19, 2017

k8s-ci-robot left a comment

Choose a reason for hiding this comment

dims commented Aug 9, 2017

cbonte commented Aug 10, 2017

cbonte commented Aug 10, 2017

cbonte commented Aug 28, 2017

cbonte commented Aug 28, 2017

jhorwit2 commented Aug 28, 2017

jhorwit2 commented Aug 28, 2017

wlan0 commented Aug 28, 2017

cbonte commented Aug 29, 2017

calebamiles commented Sep 5, 2017

calebamiles commented Sep 5, 2017

liggitt commented Sep 5, 2017

dims commented Sep 6, 2017

liggitt commented Sep 6, 2017

dims commented Sep 6, 2017

cheftako left a comment

Choose a reason for hiding this comment

yujuhong Sep 7, 2017

Choose a reason for hiding this comment

yujuhong commented Sep 7, 2017

k8s-github-robot commented Sep 7, 2017

k8s-github-robot commented Sep 7, 2017

cbonte commented Sep 7, 2017

yujuhong commented Sep 7, 2017

k8s-cherrypick-bot commented Oct 3, 2017

cbonte commented Jul 19, 2017 •

edited by enisoc

Loading