[RFC] Prepare for deprecating NodeLegacyHostIP #36095

caesarxuchao · 2016-11-02T21:52:01Z

What this PR does

Add comments saying "LegacyHostIP" will be deprecated in 1.7;
Add v1.NodeLegacyHostIP to be consistent with the internal API (useful for client-go migration [RFC] Migrating kubernetes components to use client-go #35159)
Let cloudproviders who used to only set LegacyHostIP set the IP as both InternalIP and ExternalIP
Master used to ssh tunnel to node's ExternalIP or LegacyHostIP to do healthz check. OTOH, if on-prem, kubelet only sets LegacyHostIP or InternalIP. In order to deprecate LegacyHostIP in 1.7, I let healthz check to use InternalIP if ExternalIP is not available. (The healthz check is the only consumer of LegacyHostIP in k8s.)

LegacyHostIP will be deprecated in 1.7.

This change is

k8s-ci-robot · 2016-11-02T22:26:33Z

Jenkins unit/integration failed for commit 73743d6. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

liggitt · 2016-11-03T15:01:49Z

pkg/master/master.go

-func findExternalAddress(node *api.Node) (string, error) {
-	var fallback string
+// findAddress returns address in this order: ExternalIP, InternalIP, LegacyHostIP.
+func findAddress(node *api.Node) (string, error) {


use GetPreferredNodeAddress(node, []{externalIP, internalIP, legacyHostIP})

Done. PTAL. Thanks.

liggitt · 2016-11-04T20:07:46Z

pkg/master/master.go

+	preferredAddressTypes := []api.NodeAddressType{
+		api.NodeExternalIP,
+		api.NodeInternalIP,
+		api.NodeLegacyHostIP,


would like someone familiar with the tunneling setup to comment... @mikedanese / @cjcullen? we weren't using NodeInternalIP for tunneling before... is that valid? I think the point of tunneling is that the master isn't on the same network, so internal IP is not likely to work

Agreed. We can't use an internal IP for tunneling.

caesarxuchao · 2016-11-04T20:56:53Z

pkg/master/master.go

@@ -354,14 +355,18 @@ type nodeAddressProvider struct {
 }

 func (n nodeAddressProvider) externalAddresses() (addresses []string, err error) {
+	preferredAddressTypes := []api.NodeAddressType{


I misunderstood the tunneling healthz. The tunneling shouldn't use the InternalIP.

Now the changes to this file do not change any logic. The only change is reusing the utility function in pkg/util/node.

caesarxuchao · 2016-11-04T20:57:15Z

@liggitt do you agree with the cloudprovider changes?

liggitt · 2016-11-04T21:00:28Z

pkg/cloudprovider/providers/rackspace/rackspace.go

@@ -408,7 +408,11 @@ func (i *Instances) NodeAddresses(nodeName types.NodeName) ([]api.NodeAddress, e
 	glog.V(2).Infof("NodeAddresses(%v) => %v", serverName, ip)

 	// net.ParseIP().String() is to maintain compatibility with the old code


parse to string once, assign multiple times

Done. Thanks.

liggitt · 2016-11-04T21:01:49Z

I think so. If a cloudprovider doesn't differentiate between internal/external, it makes sense to register it for both

liggitt · 2016-11-04T21:21:53Z

I1104 13:58:17.061] FAILED   hack/make-rules/../../hack/verify-bazel.sh 22s

liggitt · 2016-11-04T21:21:59Z

LGTM otherwise

Mark NodeLegacyHostIP will be deprecated in 1.7; Let cloudprovider that used to only set NodeLegacyHostIP set the IP as both InternalIP and ExternalIP, to allow dprecation in 1.7

caesarxuchao · 2016-11-04T21:30:21Z

Thanks! Applying the label per comment.

k8s-ci-robot · 2016-11-04T21:44:31Z

Jenkins verification failed for commit f3c096e2723ebe820661815dfa8f4024ae997db5. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-github-robot · 2016-11-07T04:47:00Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-11-07T05:21:15Z

Automatic merge from submit-queue

@deads2k

Automatic merge from submit-queue (batch tested with PRs 50537, 49699, 50160, 49025, 50205) When not using a CloudProvider, set both InternalIP and ExternalIP on Nodes #36095 changed all of the cloudproviders to set both InternalIP and ExternalIP on Nodes, but the non-cloudprovider fallback code now only sets InternalIP. This causes the test "should be able to create a functioning NodePort service" in test/e2e/service.go to fail on cloud-provider-less clusters, because (with LegacyHostIP gone), it now will only try to work with ExternalIPs, and will fail if the node has only an InternalIP. There isn't much other code that assumes that ExternalIP will always be set (there's something in pkg/master/master.go, but I don't know what it's doing, so maybe it's only useful in the case where InternalIP != ExternalIP anyway). But given that several of the cloudproviders (mesos, ovirt, rackspace) now explicitly set both InternalIP and ExternalIP to the same value always, it seemed right to do that in the fallback case too. @deads2k FYI **Release note**: ```release-note NONE ```

googlebot added the cla: yes label Nov 2, 2016

caesarxuchao changed the title ~~Prepare for deprecating NodeLegacyHostIP~~ [RFC] Prepare for deprecating NodeLegacyHostIP Nov 2, 2016

k8s-github-robot assigned erictune Nov 2, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Nov 2, 2016

liggitt reviewed Nov 3, 2016

View reviewed changes

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

caesarxuchao force-pushed the deprecate-LegacyHostIP branch from 73743d6 to 1fe68b2 Compare November 4, 2016 02:11

caesarxuchao added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Nov 4, 2016

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 4, 2016

caesarxuchao assigned liggitt and unassigned erictune Nov 4, 2016

liggitt reviewed Nov 4, 2016

View reviewed changes

caesarxuchao assigned mikedanese Nov 4, 2016

caesarxuchao force-pushed the deprecate-LegacyHostIP branch from 1fe68b2 to 42c03a8 Compare November 4, 2016 20:52

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

caesarxuchao force-pushed the deprecate-LegacyHostIP branch from 42c03a8 to 10b95b5 Compare November 4, 2016 20:54

caesarxuchao commented Nov 4, 2016

View reviewed changes

liggitt reviewed Nov 4, 2016

View reviewed changes

caesarxuchao force-pushed the deprecate-LegacyHostIP branch from 10b95b5 to f3c096e Compare November 4, 2016 21:16

Add v1.NodeLegacyHostIP to be consistent with the internal API;

783af94

Mark NodeLegacyHostIP will be deprecated in 1.7; Let cloudprovider that used to only set NodeLegacyHostIP set the IP as both InternalIP and ExternalIP, to allow dprecation in 1.7

caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2016

caesarxuchao added this to the v1.5 milestone Nov 4, 2016

caesarxuchao force-pushed the deprecate-LegacyHostIP branch from f3c096e to 783af94 Compare November 4, 2016 22:44

caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 4, 2016

k8s-github-robot merged commit 0068c30 into kubernetes:master Nov 7, 2016

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

caesarxuchao mentioned this pull request Apr 23, 2017

Deprecate NodeLegacyHostIP in 1.7 #44807

Closed

danwinship mentioned this pull request Jul 17, 2017

When not using a CloudProvider, set both InternalIP and ExternalIP on Nodes #49025

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Prepare for deprecating NodeLegacyHostIP #36095

[RFC] Prepare for deprecating NodeLegacyHostIP #36095

caesarxuchao commented Nov 2, 2016 •

edited by k8s-oncall

Loading

k8s-ci-robot commented Nov 2, 2016

liggitt Nov 3, 2016 •

edited

Loading

caesarxuchao Nov 4, 2016

liggitt Nov 4, 2016 •

edited

Loading

mikedanese Nov 4, 2016

caesarxuchao Nov 4, 2016

caesarxuchao commented Nov 4, 2016

liggitt Nov 4, 2016 •

edited

Loading

caesarxuchao Nov 4, 2016

liggitt commented Nov 4, 2016

liggitt commented Nov 4, 2016

liggitt commented Nov 4, 2016

caesarxuchao commented Nov 4, 2016

k8s-ci-robot commented Nov 4, 2016

k8s-github-robot commented Nov 7, 2016

k8s-github-robot commented Nov 7, 2016

		@@ -408,7 +408,11 @@ func (i *Instances) NodeAddresses(nodeName types.NodeName) ([]api.NodeAddress, e
		glog.V(2).Infof("NodeAddresses(%v) => %v", serverName, ip)

		// net.ParseIP().String() is to maintain compatibility with the old code

[RFC] Prepare for deprecating NodeLegacyHostIP #36095

[RFC] Prepare for deprecating NodeLegacyHostIP #36095

Conversation

caesarxuchao commented Nov 2, 2016 • edited by k8s-oncall Loading

k8s-ci-robot commented Nov 2, 2016

liggitt Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

caesarxuchao Nov 4, 2016

Choose a reason for hiding this comment

liggitt Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

mikedanese Nov 4, 2016

Choose a reason for hiding this comment

caesarxuchao Nov 4, 2016

Choose a reason for hiding this comment

caesarxuchao commented Nov 4, 2016

liggitt Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

caesarxuchao Nov 4, 2016

Choose a reason for hiding this comment

liggitt commented Nov 4, 2016

liggitt commented Nov 4, 2016

liggitt commented Nov 4, 2016

caesarxuchao commented Nov 4, 2016

k8s-ci-robot commented Nov 4, 2016

k8s-github-robot commented Nov 7, 2016

k8s-github-robot commented Nov 7, 2016

caesarxuchao commented Nov 2, 2016 •

edited by k8s-oncall

Loading

liggitt Nov 3, 2016 •

edited

Loading

liggitt Nov 4, 2016 •

edited

Loading

liggitt Nov 4, 2016 •

edited

Loading