Store the latest cloud provider node addresses #65226

ingvagabund · 2018-06-19T14:51:11Z

What this PR does / why we need it:
Buffer the recently retrieved node address so they can be used as soon as the next node status update is run.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #65814

Special notes for your reviewer:

Release note:

None

ingvagabund · 2018-06-19T14:51:30Z

@sjenning PTAL

derekwaynecarr · 2018-06-19T16:15:49Z

pkg/kubelet/kubelet.go

+	// Last list of node addresses retrieved from the cloud provider
+	cloudproviderLastNodeAddresses []v1.NodeAddress
+	// Last error retrieved from the cloud provider
+	cloudproviderLastError error


why is this needed here as member var and not just in the kubelet_node_status code block?

Both variables needs to be on the global level so the go routine does not store its values into a local variable that disappears. Plus, the cloud related code is going out at some point and the variables are not exported. So it's fine to keep it that way. We can change it and polish later.

derekwaynecarr · 2018-06-19T16:17:24Z

pkg/kubelet/kubelet_node_status.go

@@ -486,7 +486,7 @@ func (kl *Kubelet) setNodeAddress(node *v1.Node) error {
 			kl.cloudproviderRequestMux.Unlock()

 			go func() {
-				nodeAddresses, err = instances.NodeAddresses(context.TODO(), kl.nodeName)
+				kl.cloudproviderLastNodeAddresses, kl.cloudproviderLastError = instances.NodeAddresses(context.TODO(), kl.nodeName)


i may be missing something obvious, but seems like cloudProviderLastError can just move next to declaration of var err error above on line 476

The culprit is actually the case <-kl.cloudproviderRequestSync case. Each time the go routine asking for the node addresses finishes, it sends a value to the kl.cloudproviderRequestSync channel. If the happens after the kl.cloudproviderRequestTimeout, the kl.cloudproviderRequestSync is non-empty and in the next node status iteration the select picks the first case <-kl.cloudproviderRequestSync instead of waiting for the go routine to finish. Given the nodeAddresses and err are local variables and are both nil (cause the go routine responsible for setting them stores the values into previous local variables which no longer exist in the setNodeAddress scope), the nodeAddresses and err is always nil from the moment the timeout occurs.

Thus we need to store both (the list of node addresses and the error) into variables that are not local to the setNodeAddress method.

there must be a simpler way to do this. we've added 6 fields to the kubelet struct to support the timeout in this single area of the code. i'm not saying i see it yet. but there must be. i'm too tired to see it atm though.

this looks like it is writing to member variables asynchronously outside a lock... isn't that a crashing data race?

Unlikely to happen but it is. Just the chance is very low. I am addressing this in another PR.

Ok, I will update this PR with the suggested refactoring

derekwaynecarr · 2018-06-19T16:18:19Z

it may help give some clarity on the issue we are having to describe the reason for the change for posterity.

ingvagabund · 2018-06-19T20:39:31Z

/retest

sjenning · 2018-06-26T03:04:00Z

@ingvagabund what would you think about abstracting this to a cloudRequestManager that implements

type CloudRequestManager interface {
  Start(cloud cloudprovider.Interface) error
  NodeAddresses(ctx context.Context, name types.NodeName) ([]v1.NodeAddress, error)
}

and abstract all this internal state out of the kubelet and remove the 6 new fields between #62543 and this PR?

kl.cloudRequestManager.Start() could be called where we start the other kubelet managers and block starting the kubelet on the success of first instances.NodeAddresses(), ensuring that the call in setNodeAddress() never fails. After that, the manager can cache the result for future calls to kl.cloudRequestManager.NodeAddresses() and can update against the cloud provided at whatever interval we want (or the cloud provider will allow).

I know this PR has been proven to work, so might do this later. But I had the idea in my head; wanted to get it in writing for later.

ingvagabund · 2018-06-26T13:05:16Z

@sjenning what you are saying make sense. So far I am aware of two requests that are send:

request for node addresses
request for external ID

Do we want to have a cloud request manager for each cloud resource we ask for? Or group them based on some criteria? There are cloud resources we ask for once in a while (e.g. the node external ID) and cloud resources we ask for periodically (e.g. the node addresses). So maybe s/CloudRequestManager/CloudRequestSyncManager/? Effectively build it on top of the concept of an informer?

sjenning · 2018-06-26T14:02:06Z

Do we want to have a cloud request manager for each cloud resource we ask for? Or group them based on some criteria? There are cloud resources we ask for once in a while (e.g. the node external ID) and cloud resources we ask for periodically (e.g. the node addresses). So maybe s/CloudRequestManager/CloudRequestSyncManager/? Effectively build it on top of the concept of an informer?

Yes, I think we are on the same wavelength. CloudRequestSyncManager is good too. We can use it to basically buffer all cloudprovider requests the kubelet makes since, as we've found out, not all cloudprovider code is equal.

The real question is do you want to go ahead with this PR for now and do that as a follow-on or modify this PR. I'm on the fence about it.

sjenning · 2018-06-26T14:13:00Z

After some thought, I am in favor of going forward with this fix and doing the refactor in a follow-on PR.
/lgtm

liggitt · 2018-06-26T14:20:44Z

/hold

data race question in https://github.com/kubernetes/kubernetes/pull/65226/files#r198143459 is unresolved

ingvagabund · 2018-06-26T14:57:55Z

/lgtm cancel

ingvagabund · 2018-07-03T21:09:31Z

@dims thank you very much :)

ingvagabund · 2018-07-03T21:26:59Z

@vishh PTAL

vishh

It looks like this PR is decoupling updating node status from fetching updated node IPs.
However it is not clear why kubelet needs to keep watching for node addresses continually in the first place.
Also, if there were any production issues that this PR addresses, it would help describe them here in this PR or in a separate issue (preferred).

vishh · 2018-07-03T23:32:25Z

pkg/kubelet/cloud_request_manager.go

+}
+
+// NodeAddresses does not wait for cloud provider to return a node addresses.
+// It always returns node addresses or an error.


Is this comment true? The logic below is blocking on cloud provider API call to succeed at least once.

The first call to the NodeAddresses can be blocking but the remaining ones are not. The assumption is that a node needs to register first before it starts periodically invoking the method. So it is negligible the first call hangs for some time. I can rephrase the statement to be more clear about the fact.

ingvagabund · 2018-07-04T12:51:33Z

@vishh issue opened #65814

ingvagabund · 2018-07-05T17:00:17Z

@Random-Liu @dchen1107 @yujuhong PTAL

sjenning · 2018-07-05T17:20:53Z

@vishh I agree that it might be unnecessary to poll the cloudprovider for our addresses if we can guarantee that the information we can on the first NodeAddresses() call does not change for the life of the kubelet process (or the instance itself).

However, we didn't want to tackle that wider-ranging issue here. That is very cloudprovider specific and would require acks for all cloud providers saying "yes, our cloudprovider will not return changing return values for NodeAddresses() over the life of the kubelet".

This PR is to simply protect the kubelet from latency introduced by the cloudprovider on a NodeAddresses() call. In our case (Azure), this was a throttling mechanism which could cause the node status update loop to stall and the Node go NotReady.

ingvagabund · 2018-07-08T15:22:40Z

/retest

derekwaynecarr

/approve

this is an improvement over what exists prior.

i think its up to more debate if the cloud provider interface should be handling this for us by taking a timeout or something on each of its calls.

derekwaynecarr · 2018-07-09T14:50:37Z

pkg/kubelet/cloud_request_manager_test.go

+	case <-collected:
+		return nodeAddresses, err
+	case <-time.Tick(2 * nodeAddressesRetryPeriod):
+		return nil, fmt.Errorf("Timeout after %v waiting for address to appear", 2*nodeAddressesRetryPeriod)


nit: s/Timeout/timeout

see the original code had this casing issue as well, doesnt report back directly to end user, so not a huge deal.

actually, this is a test, so even less of a deal. got confused in my review.

derekwaynecarr · 2018-07-09T14:58:19Z

pkg/kubelet/kubelet.go

-	// Request timeout
-	cloudproviderRequestTimeout time.Duration
+	// Handles requests to cloud provider with timeout
+	cloudResourceSyncManager *cloudResourceSyncManager


it would be good to have a broader discussion on if we want all interaction with cloud to go through this interface in the future, or if we want to change the cloud provider interface to accept a context w/ timeout on operations in the future so each caller can decide how to handle it across the code base. for now, this cleans up the existing member vars on kubelet so is a nice incremental improvement.

derekwaynecarr · 2018-07-09T15:05:10Z

pkg/kubelet/kubelet_node_status_test.go

-		kubelet.cloudproviderRequestParallelism = make(chan int, 1)
-		kubelet.cloudproviderRequestSync = make(chan int)
-		kubelet.cloudproviderRequestTimeout = 10 * time.Second
+		kubelet.cloudResourceSyncManager = NewCloudResourceSyncManager(kubelet.cloud, kubelet.nodeName, kubelet.nodeStatusUpdateFrequency)


i think we can update this in the future from 10s to 1m, 5m, 10m, etc.

an issue or // todo to track it would be good.

k8s-ci-robot · 2018-07-09T15:06:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, dims, sjenning

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:

~~pkg/cloudprovider/OWNERS~~ [dims]
~~pkg/kubelet/OWNERS~~ [derekwaynecarr]

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

ingvagabund · 2018-07-09T15:32:18Z

/hold cancel

k8s-github-robot · 2018-07-09T16:30:28Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-07-09T17:47:06Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot · 2018-07-17T03:39:49Z

@ingvagabund: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-local-e2e-containerized	`9d9fb4d`	link	`/test pull-kubernetes-local-e2e-containerized`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

bashofmann · 2018-09-24T11:45:32Z

Would it make sense to backport this to 1.11 to fix #68270 ?

alena1108 · 2018-10-22T18:47:49Z

Having correct ip addresses set on the nodes is a plumbing for all subsequent operations on k8s. Would be really helpful to backport this fix to 1.11. We have a lot of users who run 1.11 who are currently facing this problem, and not planning to move to k8s 1.12 yet,

openstacker · 2018-10-23T21:47:08Z

I'm echoing @alena1108

This is a very critical fix and we have seen this on v1.11.2. Missing internal IP makes the API server can't talk to the worker node and it breaks everything. We also don't want to much v1.12 so fast because I'm afraid there will be new unknown issue.

So please consider to backport this to v1.11.x. Thank you.

2rs2ts · 2018-11-01T15:45:05Z

We ran into this problem too. Our solution was to just pass the --node-ip flag to kubelet.

Follow on to kubernetes#65226.

…#65226-upstream-release-1.11 Automated cherry pick of #65226: Put all the node address cloud provider retrival complex

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 19, 2018

ingvagabund requested a review from derekwaynecarr June 19, 2018 14:51

k8s-ci-robot requested a review from vishh June 19, 2018 14:51

derekwaynecarr suggested changes Jun 19, 2018

View reviewed changes

ingvagabund mentioned this pull request Jun 19, 2018

openshift: compute: origin-node fails to start Azure/acs-engine#3306

Closed

k8s-ci-robot assigned sjenning Jun 26, 2018

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

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

ingvagabund changed the title ~~Store the latest cloud provider node addresses~~ WIP: Store the latest cloud provider node addresses Jun 26, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2018

ingvagabund force-pushed the store-cloud-provider-latest-node-addresses branch 5 times, most recently from 452a7d2 to 3203fa6 Compare June 26, 2018 17:08

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

vishh reviewed Jul 3, 2018

View reviewed changes

derekwaynecarr approved these changes Jul 9, 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 Jul 9, 2018

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

k8s-github-robot merged commit f704109 into kubernetes:master Jul 9, 2018

ingvagabund deleted the store-cloud-provider-latest-node-addresses branch July 10, 2018 07:26

wanghaoran1988 mentioned this pull request Sep 12, 2018

One Node loose private and external ip address #68270

Closed

alexandrem mentioned this pull request Sep 28, 2018

Kubernetes nodes lose InternalIP and ExternalIP temporarily kubernetes/cloud-provider-openstack#280

Closed

lingxiankong mentioned this pull request Oct 24, 2018

Automated cherry pick of #65226: Put all the node address cloud provider retrival complex #70154

Merged

cheftako added a commit to cheftako/kubernetes that referenced this pull request Nov 22, 2018

Fixing address locking in CP fake.

a49a4b9

Follow on to kubernetes#65226.

cheftako mentioned this pull request Nov 22, 2018

Fixing address locking in CP fake. #71333

Merged

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

Merge pull request #70154 from lingxiankong/automated-cherry-pick-of-…

49a7911

…#65226-upstream-release-1.11 Automated cherry pick of #65226: Put all the node address cloud provider retrival complex

andrewsykim mentioned this pull request Jun 19, 2019

Nodes intermittently loose their node addresses kubernetes/cloud-provider#37

Closed

Store the latest cloud provider node addresses #65226

Store the latest cloud provider node addresses #65226

Conversation

ingvagabund commented Jun 19, 2018 • edited Loading

ingvagabund commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingvagabund Jun 19, 2018 • edited Loading

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

derekwaynecarr commented Jun 19, 2018

ingvagabund commented Jun 19, 2018

sjenning commented Jun 26, 2018

ingvagabund commented Jun 26, 2018 • edited Loading

sjenning commented Jun 26, 2018

sjenning commented Jun 26, 2018

liggitt commented Jun 26, 2018

ingvagabund commented Jun 26, 2018

ingvagabund commented Jul 3, 2018

ingvagabund commented Jul 3, 2018

vishh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingvagabund commented Jul 4, 2018

ingvagabund commented Jul 5, 2018

sjenning commented Jul 5, 2018

ingvagabund commented Jul 8, 2018

derekwaynecarr 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

k8s-ci-robot commented Jul 9, 2018

ingvagabund commented Jul 9, 2018

k8s-github-robot commented Jul 9, 2018

k8s-github-robot commented Jul 9, 2018

k8s-ci-robot commented Jul 17, 2018

bashofmann commented Sep 24, 2018

alena1108 commented Oct 22, 2018

openstacker commented Oct 23, 2018

2rs2ts commented Nov 1, 2018

ingvagabund commented Jun 19, 2018 •

edited

Loading

ingvagabund Jun 19, 2018 •

edited

Loading

ingvagabund commented Jun 26, 2018 •

edited

Loading