Also see #6087.

I would argue that if we're unable to watch nodes reliably, we have many other problems. That seems like something we need to fix, rather than work around.

watch is watch, a separation of concerns between the kubelet (which is running on the node that has the hostip) and the apiserver is a different problem imo.

After discussion, we decided to just remove host IP from PodStatus. (It will still be available from "kubectl get nodes" if someone needs it.)

@bgrant0607 does that sound OK with you?

I'm working on making the nodes register themselves rather than having the node controller do it for them (see roberthbailey@dcec615 which should turn into a PR later this week). Will that assuage your concerns here?

I hadn't looked at your PR. We discussed having Kubelet pull ths information from the cloud provider but thought it might not be a good idea due to (1) requiring Kubelet to understand the details of all the cloud providers, and (2) making very hard to rate limit the calls into the cloud provider.

I could imagine an approach where we still use NodeController to get the node address, but the node still initiates the registration into the cluster. Not sure whether that's a good idea, though.

@lavalamp

Kubelet won't necessarily (and really shouldn't) have the credentials necessary to contact the cloud provider.

I don't remember why HostIP was added to PodStatus, and I would favor removing it, except that it would be a breaking change. You could ask on google-containers@ whether anyone is relying upon it.

HostIP is tricky, as hosts may have multiple addresses, in general, which is why NodeAddresses is now an array.

On GCE / AWS do you even need credentials to determine your external IP? I thought you got it from the metadata server running locally without needing to hit an Auth'd API endpoint.

HostIP wasn't the external IP AFAIK. It's not well defined what it is, which is one reason it should be eliminated.

Yes, one should be able to get the host IP from metadata, but the cloudprovider library doesn't currently do that.

I'm also in favor of removing HostIP from pods. But I don't agree with the reason in the OP (that sometimes it'll be blank when kubelet hasn't heard from apiserver)-- I think that's par for the course.

@roberthbailey Should we schedule a meeting to discuss whether it's reasonable for Kubelets to pull their IPs directly from the cloud provider? What is the alternative -- a new API endpoint on the master where unregistered nodes can query their IP address from NodeController? Do nodes really need their IP address when they register with the master?

That sounds like a good idea (I think we can likely clear this up with a quick meeting rather than going back and forth for days in github comments).

We all agreed that we should remove HostIP from pods.

Nodes do not need to provide their address(es) when they register. It's not part of the spec. They could provide whatever hostnames and/or addresses they can get through normal Linux mechanisms and/or could contact a local metadata server, but I don't think that's necessary, since the node controller will update the addresses once the node is registered, unless we want to support scenarios with no cloud provider and no external node database. In any case, we shouldn't discuss self-registration in this issue. There are other issues that cover that topic.

OK, so this issue just boils down to removing HostIP from PodStatus, and we'll discuss the rest in #6949 ?

Ping on this. e2e "Pods should get a host IP [Conformance]" is currently disabled everywhere. Bumping this to P1 and flake.

Today Kubelet fills PodStatus.HostIP in generatePodStatus() in kubelet.go
Master (e.g. NodeController) is not involved.

Currently the test waits two minutes for a pod to get a HostIP

Considering that we have proposed to remove this field completely, I think we should just bump up the timeout a bit and see if that fixes the test.

Any other thoughts?

I have also sent out email to the appropriate mailing lists to see if anyone is using PodStatus.HostIP, since we would like to get rid of it, as discussed upthread.

cc/ @dchen1107

BTW the title of this issue says "Kubelet shouldn't require apiserver to populate host ip of pods." But it wasn't clear to me where the dependency on the API server is (unfortunately @bprashanth 's original comment points to a very old revision to kubelet.go so I'm not sure which line he was referring to). The work of finding the HostIP seems to be done by Kubelet.GetHostIP() / Kubelet.GetNode() which assumes Kubelet.nodeInfo is filled in -- is that the dependency? I couldn't find where that gets filled in.

@dchen1107 would be great if you could shed some light.

I haven't looked at this code recently but nodinfo appears to still be retrieved from a cache populated via apiserver watch:

Ah yes, that looks right, thanks @bprashanth
(well, "right" in the sense of "that's where it gets populated," not in the sense of "it makes sense that kubelet is using code defined in the scheduler predicates for this"...)

@bprashanth while you're here, do you have a suggestion of the best course of action? Long-term we want to get rid of PodStatus.HostIP, but short-term we need to do something about this test. How can we make populating the cache more reliable? Or should we just increase the timeout on the test? Or something else?

It looks like the kubelet already has the node address: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet.go#L2663, so assuming no one else is going to change the ip, we could just store it in the kubelet and add it for every pod? Unsure if this is a safe assumption.

I also haven't debugged this new set of test failures, so I'm not sure if it's failing for the same reason

Unsure if this is a safe assumption.

... to make on all clouds

There isn't a new set of failures - Ike just noticed that the test is disabled, from when this issue was originally opened.

@dchen1107 @yujuhong could one of you comment on whether Prashanth's suggestion
#6558 (comment)
sounds reasonable?

I didn't parse through the entire issue since a lot of information is stale. Yes David is right, we moved to the logic of filling hostIP of Pod from apiserver to kubelet long time ago. Recording to the comment of Prashanth: #6558 (comment):

Some users might have node with multiple network interfaces, and want to hostIP with specific one
by updating Node object.

Fwiw I think we should turn the test on and handle things as they come up. I think the major source of flake was handled when we did: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet.go#L2663

With respect to the long-term future of HostIP: I'm in favor of documenting the field as deprecated and asking users to contact us with their use case if they need it (in the API field documentation).

As long as the field exists, I agree we should test that it's set correctly.

@ihmccreery in this comment #6558 (comment)
you wrote "Ping on this. e2e "Pods should get a host IP [Conformance]" is currently disabled everywhere. Bumping this to P1 and flake."

What made you say it is disabled? I looked at Jenkins and it seems to be running it:
http://kubekins.dls.corp.google.com/job/kubernetes-e2e-gce/10795/testReport/(root)/Kubernetes%20e2e%20suite/Pods_should_get_a_host_IP__Conformance_/history/

I looked in hack/jenkins/e2e.sh and didn't see any special handling of [Conformance]. There is an env var CONFORMANCE_TEST_SKIP_REGEX in hack/gingko-e2e.sh but it doesn't seem to be set anywhere. So I don't see anything that should be causing this test to not be run.

(As always there's a high likelihood I'm misunderstanding something.)

Sorry, I'm totally blind, the Jenkins history I linked to clearly shows it is being skipped.

But I'm still not sure what is causing it to be skipped.

Sorry I wasn't clear; it's being skipped because it was marked pending via PIt. Let's agree to use [Flaky] and [Feature:.+] labels in the future for clarity's sake.

Also see #15169 for use cases of "I need to know my local node" and possible fixes.

I'm going to remove the flake label, since #20510 has been merged and that was the suggestion here #6558 (comment). We can re-add the label if it flakes again. I'm not closing the issue, because we want to deprecate the field longer-term.