Use KubeletServer.NodeIP instead of KubeletServer.HostnameOverride to set node IP #6310

pravisankar · 2015-12-14T22:31:00Z

No description provided.

pravisankar · 2015-12-14T22:31:08Z

liggitt · 2015-12-15T05:22:12Z

Changing back could trigger more attempts by the node to delete and recreate its API object (which prevents the node from starting)

liggitt · 2015-12-15T17:51:09Z

Need to resolve kubernetes/kubernetes#17731 before this can merge

pravisankar · 2016-01-12T23:46:01Z

[test]

pravisankar · 2016-01-13T06:07:41Z

@liggitt @DirectXMan12 @ncdc @eparis PTAL

liggitt · 2016-01-13T06:09:15Z

change UPSTREAM: <drop>: ... to UPSTREAM: 18541: ... if there's an existing PR

liggitt · 2016-01-13T14:09:07Z

can you outline the ways you tested this? Off the top of my head, I'd expect:

upgrade path with nodeIP set
- launch 1.0, with nodeIP set in config
- relaunch 1.1, with nodeIP set in config, against the same etcd (this may actually prevent the node from coming up)
- relaunch with this PR, with nodeIP set in config, against the same etcd
- expect node to come up, with warning, and pods to be able to be deployed
upgrade path without nodeIP set
- launch 1.0, without nodeIP set in config
- relaunch 1.1, without nodeIP set in config, against the same etcd
- relaunch with this PR, without nodeIP set in config, against the same etcd
- expect node to come up, with no warning
- relaunch with this PR, with nodeIP set in config, against the same etcd
- expect node to come up, with no warning, and pods to be able to be deployed

DirectXMan12 · 2016-01-13T15:01:31Z

For the part that I wrote:

Before my patch:

launch node with nodeIP set (giving externalID == nodeIP)
relaunch node without nodeIP set (giving externalID == hostname)
Fail

After my patch:

Try to launch node again
It works, gives warning about externalID mismatch
Delete node
Launch node with same config (externalID == hostname)
Relaunch node with nodeIP set (externalID == nodeIP)
It works, gives warning about externalID mismatch

…belet

…vider Previously, if the kubelet tried to register itself with the API server, and was rejected due to the external ID changing, it would delete the node object and recreate it. This commit causes it to tolerate a change in ExternalID when the ExternalID is not being provided by a cloud provider, assuming the new ExternalID is either the node's (metadata) name, or one of node's addresses.

… set node IP

pravisankar · 2016-01-13T21:18:59Z

Updated the PR:
Changed commit message: UPSTREAM: : ... to UPSTREAM: 18541: ..

For tolerating node externalID to have hostname or node IP in case of non-cloud provider, we just need to look at the currentNode.Spec.ExternalID (instead of node.Spec.ExternalID).
@DirectXMan12 please review?

Tested various combinations from v1.0.8 to v1.1.1 without fix and with fix. I think, I covered all the test cases mentioned by @liggitt and @DirectXMan12

@liggitt @ncdc @eparis please review/merge

DirectXMan12 · 2016-01-13T21:27:12Z

For tolerating node externalID to have hostname or node IP in case of non-cloud provider, we just need to look at the currentNode.Spec.ExternalID (instead of node.Spec.ExternalID).

@pravisankar LGTM. The logic you removed was for the "hostname -> nodeIP" case, but that should be solved by your other changes, correct?

liggitt · 2016-01-13T21:31:27Z

LGTM

eparis · 2016-01-13T21:33:27Z

LGTM who is going to click the button?

ncdc · 2016-01-13T21:39:20Z

Godeps/_workspace/src/k8s.io/kubernetes/pkg/kubelet/kubelet.go

+
+			if kl.cloud == nil {
+				// In the case where we're not using a cloud provider, the ExternalID could either have
+				// been the NodeIP, or the actual hostname.  If we switch between the two, consider it


Do we want a link back to the PR that introduced the nodeIP issue?

not in the code, fine to link from this PR, which is reachable from the commit breadcrumb

eparis · 2016-01-13T21:42:26Z

[merge]

ncdc · 2016-01-13T21:42:48Z

LGTM

smarterclayton · 2016-01-13T21:44:23Z

LGTM

ncdc · 2016-01-13T22:25:29Z

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8529/

1)  unauthenticated user should be able to log in
  - UnknownError: unknown error: [$injector:modulerr] Failed to instantiate module openshiftConsole due to:
ReferenceError: URI is not defined

liggitt · 2016-01-13T22:29:34Z

@jwforres ^

liggitt · 2016-01-13T22:30:53Z

[test]

jwforres · 2016-01-14T00:01:21Z

Opened an issue for the test problem
#6651
On Jan 13, 2016 6:30 PM, "OpenShift Bot" [email protected] wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8536/
)

—
Reply to this email directly or view it on GitHub
#6310 (comment).

openshift-bot · 2016-01-14T02:20:24Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4628/) (Image: devenv-rhel7_3144)

liggitt · 2016-01-14T04:24:19Z

URI.js issue resolved in https://ci.openshift.redhat.com/jenkins/job/devenv_ami/3143/
[test][merge]

openshift-bot · 2016-01-14T04:25:24Z

Evaluated for origin test up to 7da6989

openshift-bot · 2016-01-14T04:25:31Z

Evaluated for origin merge up to 7da6989

openshift-bot · 2016-01-14T05:21:36Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8543/)

Merged by openshift-bot

smarterclayton · 2016-09-01T01:54:56Z

Did this ever get moved upstream?

liggitt · 2016-09-04T03:40:36Z

the toleration to avoid deleting/recreating? no, though I doubt upstream cares... the node just deletes itself there

liggitt added the area/compatibility label Dec 15, 2015

pravisankar force-pushed the fix-nodeip-config branch from f705544 to 1b3c2d9 Compare January 12, 2016 23:45

pravisankar force-pushed the fix-nodeip-config branch from 51167af to 2b425d8 Compare January 13, 2016 00:11

pravisankar changed the title ~~[DO_NOT_MERGE] Use KubeletServer.NodeIP instead of KubeletServer.HostnameOverride to set node IP~~ Use KubeletServer.NodeIP instead of KubeletServer.HostnameOverride to set node IP Jan 13, 2016

Ravi Sankar Penta and others added 4 commits January 13, 2016 07:57

UPSTREAM: 18541: Allow node IP to be passed as optional config for ku…

44d5579

…belet

Use KubeletServer.NodeIP instead of KubeletServer.HostnameOverride to…

815eafb

… set node IP

Auto generated bash completions for node-ip kubelet config option

7da6989

pravisankar force-pushed the fix-nodeip-config branch from 2b425d8 to 7da6989 Compare January 13, 2016 21:10

ncdc reviewed Jan 13, 2016
View reviewed changes

openshift-bot pushed a commit that referenced this pull request Jan 14, 2016

Merge pull request #6310 from pravisankar/fix-nodeip-config

ad0da3a

Merged by openshift-bot

openshift-bot merged commit ad0da3a into openshift:master Jan 14, 2016

pmorie mentioned this pull request Aug 31, 2016

Fixes for attach-detach controller enablement on existing nodes #10748

Merged

DirectXMan12 mentioned this pull request Jun 14, 2018

Rebase 3.11 beta2 openshift/kubernetes#59

Merged

7 tasks

Use KubeletServer.NodeIP instead of KubeletServer.HostnameOverride to set node IP #6310

Use KubeletServer.NodeIP instead of KubeletServer.HostnameOverride to set node IP #6310

Uh oh!

Conversation

pravisankar commented Dec 14, 2015

Uh oh!

pravisankar commented Dec 14, 2015

Uh oh!

liggitt commented Dec 15, 2015

Uh oh!

liggitt commented Dec 15, 2015

Uh oh!

pravisankar commented Jan 12, 2016

Uh oh!

pravisankar commented Jan 13, 2016

Uh oh!

liggitt commented Jan 13, 2016

Uh oh!

liggitt commented Jan 13, 2016

Uh oh!

DirectXMan12 commented Jan 13, 2016

Uh oh!

pravisankar commented Jan 13, 2016

Uh oh!

DirectXMan12 commented Jan 13, 2016

Uh oh!

liggitt commented Jan 13, 2016

Uh oh!

eparis commented Jan 13, 2016

Uh oh!

ncdc Jan 13, 2016

Choose a reason for hiding this comment

Uh oh!

liggitt Jan 13, 2016

Choose a reason for hiding this comment

Uh oh!

eparis commented Jan 13, 2016

Uh oh!

ncdc commented Jan 13, 2016

Uh oh!

smarterclayton commented Jan 13, 2016

Uh oh!

ncdc commented Jan 13, 2016

Uh oh!

liggitt commented Jan 13, 2016

Uh oh!

liggitt commented Jan 13, 2016

Uh oh!

jwforres commented Jan 14, 2016

Uh oh!

openshift-bot commented Jan 14, 2016

Uh oh!

liggitt commented Jan 14, 2016

Uh oh!

openshift-bot commented Jan 14, 2016

Uh oh!

openshift-bot commented Jan 14, 2016

Uh oh!

openshift-bot commented Jan 14, 2016

Uh oh!

smarterclayton commented Sep 1, 2016

Uh oh!

liggitt commented Sep 4, 2016

Uh oh!

Uh oh!