kubelet waits for node lister to sync at least once #94087

derekwaynecarr · 2020-08-18T19:30:46Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
If node has a kube client, it will wait to ensure the node lister has synced at least once before trusting its data.

Which issue(s) this PR fixes:
Fixes #92067

Special notes for your reviewer:
still under discussion where to best wait (in getter, or in main kubelet startup)

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2020-08-18T19:31:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr

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/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

derekwaynecarr · 2020-08-18T19:31:17Z

@sjenning idea is to do similar as seen here, just need to work out pro/con for where we wait for node lister to sync at least once.

derekwaynecarr · 2020-08-18T19:39:24Z

after following up with @deads2k seems like it would be a good time to move away from list/watch and to a node informer with filter, then we can just use regular HasSynced function. will iterate, just trying to determine the best location to wait for the node informer to sync at least once and not cause issues (do not want to do it in the kubelet_getters if possible)

sjenning · 2020-08-18T20:13:21Z

this might also fix #93338

derekwaynecarr · 2020-08-18T21:09:47Z

@sjenning take a look, the idea is to move the predicate function to call GetNode, which if we have a kubeclient ensures we synced the node lister at least once (otherwise, it errors). I am still trying to think through an edge case for the scheduler scenario that we may miss, but this would ensure that we do not fall back to the kubelet "initialNode" and stop tripping up scheduler.

sjenning · 2020-08-18T21:14:02Z

pkg/kubelet/kubelet_getters.go

@@ -245,7 +256,7 @@ func (kl *Kubelet) GetNode() (*v1.Node, error) {
 // zero capacity, and the default labels.
 func (kl *Kubelet) getNodeAnyWay() (*v1.Node, error) {
 	if kl.kubeClient != nil {
-		if n, err := kl.nodeLister.Get(string(kl.nodeName)); err == nil {
+		if n, err := kl.GetNode(); err == nil {


pretty sure this whole function collapses to just return kl.GetNode() since GetNode() contains the standalone logic

GetNode does not guarantee a response in case where kubeclient is not nil, whereas getNodeAnyway does (it falls back to initial node).

deads2k · 2020-08-18T21:16:21Z

pkg/kubelet/kubelet_getters.go

@@ -235,6 +237,15 @@ func (kl *Kubelet) GetNode() (*v1.Node, error) {
 	if kl.kubeClient == nil {
 		return kl.initialNode(context.TODO())
 	}
+	// if we have a valid kube client, we wait up to 5s for initial lister to sync
+	if !kl.nodeHasSynced() {
+		err := wait.PollImmediate(time.Second, 5*time.Second, func() (bool, error) {


choose a weird number so we can find this later if it comes up. How about 8 seconds

derekwaynecarr · 2020-08-31T20:10:44Z

/retest

derekwaynecarr · 2021-01-12T22:09:02Z

i am fine removing my own hold on this as its possible what i saw was related to another unreliable aspect of CI at that time.

/hold cancel

…m-release-1.18 Cherry pick of #94087 upstream release 1.18

…87-upstream-release-1.19 Automated cherry pick of #94087: node sync at least once

…87-upstream-release-1.20 Automated cherry pick of #94087: node sync at least once

neolit123 · 2021-02-21T02:32:26Z

pkg/kubelet/kubelet.go

+				klog.Infof("kubelet nodes sync")
+				return true
+			}
+			klog.Infof("kubelet nodes not sync")


this message is posted a lot in the logs, i think it can be omitted.

Line 447 klog.Infof("kubelet nodes sync") is removed in #98137

it's still present at HEAD:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet.go#L452

neolit123 · 2021-02-21T02:44:02Z

pkg/kubelet/kubelet_getters.go

+	// if we have a valid kube client, we wait for initial lister to sync
+	if !kl.nodeHasSynced() {
+		err := wait.PollImmediate(time.Second, maxWaitForAPIServerSync, func() (bool, error) {
+			return kl.nodeHasSynced(), nil
+		})
+		if err != nil {
+			return nil, fmt.Errorf("nodes have not yet been read at least once, cannot construct node object")
+		}
+	}


this poll is problematic.
GetNode is called in a hot loop, which means that the poll is present on each call.

the sequence looks like:

GetNode()

Poll for nodeHasSynced()

Poll ....

GetNode()

Poll for nodeHasSynced()

Poll ....

if there is a valid client, arguably the informer sync check should be outside of this loop.

Wouldn't it be enough to check HasSynced once before creating the Kubelet object?

it would only sync after the api server is up.

if the API server is down, there is nothing to sync. Am I missing something?

BTW we are discussion how to improve this in:
#99336

if the kubelet is managing the first server instance in the cluster as a static pod and if the HasSynced check is before the kubelet object creation this means for such a kubelet instance the single HasSynced check will always be false.

maybe that's what we want, given this is the first node in the cluster and there is no need to sync it on the first kubelet run (ever).

for subsequent runs from the same kubelet or additional kubelet the check should pass if there is an api server.

neolit123 · 2021-02-21T02:54:09Z

the Minikube maintainers (cc @medyagh) found that this change introduced a performance regression.
when client credentials are fed to a primary kubelet in a cluster (--kubeconfig), but there are no other nodes or an API server running yet (because this primary kubelet will boot it from a static pod), this blocks for extra ~40 seconds.

i can see the informer wait as something desired but i think the logic here can be improved.
cc @derekwaynecarr @deads2k @liggitt

we also backported this change in the support skew and since we don't have direct performance tests, nobody saw it until now.

snowplayfire · 2022-05-13T11:40:42Z

If these pods are re-scheduled automatically, can the problem be solved? @derekwaynecarr

k8s-ci-robot requested review from dchen1107 and mtaufen August 18, 2020 19:31

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 18, 2020

derekwaynecarr mentioned this pull request Aug 18, 2020

Kubernetes scheduler spams cluster with pods in NodeAffinity status #92067

Closed

derekwaynecarr force-pushed the node-sync-once branch 2 times, most recently from b510d22 to 5a39f16 Compare August 18, 2020 19:49

derekwaynecarr force-pushed the node-sync-once branch from 5a39f16 to bdd6a2f Compare August 18, 2020 20:46

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 18, 2020

derekwaynecarr force-pushed the node-sync-once branch 2 times, most recently from 3de6020 to 7847919 Compare August 18, 2020 21:08

sjenning reviewed Aug 18, 2020

View reviewed changes

deads2k reviewed Aug 18, 2020

View reviewed changes

derekwaynecarr force-pushed the node-sync-once branch 2 times, most recently from 524527b to ee3f26d Compare August 20, 2020 17:57

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

k8s-ci-robot merged commit 4e93dbc into kubernetes:master Jan 12, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 12, 2021

ehashman mentioned this pull request Jan 28, 2021

NodeAffinity issue for preemptible nodes #98534

Closed

cwdsuzhou mentioned this pull request Feb 7, 2021

Automated cherry pick of #94087: node sync at least once #98848

Merged

ehashman mentioned this pull request Feb 12, 2021

Cherry pick of #94087 upstream release 1.18 #99034

Merged

k8s-ci-robot added a commit that referenced this pull request Feb 12, 2021

Merge pull request #99034 from ehashman/cherry-pick-of-#94087-upstrea…

51cf368

…m-release-1.18 Cherry pick of #94087 upstream release 1.18

k8s-ci-robot added a commit that referenced this pull request Feb 12, 2021

Merge pull request #97996 from ehashman/automated-cherry-pick-of-#940…

c337677

…87-upstream-release-1.19 Automated cherry pick of #94087: node sync at least once

k8s-ci-robot added a commit that referenced this pull request Feb 12, 2021

Merge pull request #97995 from ehashman/automated-cherry-pick-of-#940…

9337b6c

…87-upstream-release-1.20 Automated cherry pick of #94087: node sync at least once

openshift-ci-robot mentioned this pull request Feb 19, 2021

Bug 1930960: UPSTREAM: 94087: kubelet: node sync at least once openshift/kubernetes#582

Merged

neolit123 mentioned this pull request Feb 21, 2021

1 minute slower since kubernets version 1.20.3 - [kubelet-check] Initial timeout of 40s passed. kubernetes/kubeadm#2395

Closed

neolit123 reviewed Feb 21, 2021

View reviewed changes

medyagh mentioned this pull request Feb 22, 2021

Proposal: test kubernetes pre-releases against minikube #99322

Closed

liggitt mentioned this pull request Feb 23, 2021

pkg/kubelet: improve the node informer sync check #99336

Merged

Bevisy mentioned this pull request Feb 24, 2021

Podstatus becomes MatchNodeSelector after restart kubelet #52902

Closed

ehashman mentioned this pull request Feb 24, 2021

kubelet logs print 'kubelet nodes sync' frequently #98137

Merged

This was referenced Feb 25, 2021

Kubelet rejects pod scheduled based on newly added node labels which have not been observed by the kubelet yet #93338

Closed

Pod status becomes MatchNodeSelector after restart kubelet #99708

Closed

ruiwen-zhao mentioned this pull request Mar 23, 2021

Pods fail with "NodeAffinity failed" after kubelet restarts #100467

Closed

leoryu mentioned this pull request Jun 3, 2021

tkestack pod state in NodeAffinity after reboot host tkestack/tke#1330

Closed

pacoxu mentioned this pull request Sep 13, 2022

pod stuck with NodeAffinity status // using spot VMs under K8s 1.22.x and 1.23.x #112333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet waits for node lister to sync at least once #94087

kubelet waits for node lister to sync at least once #94087

derekwaynecarr commented Aug 18, 2020

k8s-ci-robot commented Aug 18, 2020

derekwaynecarr commented Aug 18, 2020

derekwaynecarr commented Aug 18, 2020

sjenning commented Aug 18, 2020

derekwaynecarr commented Aug 18, 2020

sjenning Aug 18, 2020

derekwaynecarr Aug 18, 2020

deads2k Aug 18, 2020

derekwaynecarr commented Aug 31, 2020

derekwaynecarr commented Jan 12, 2021

neolit123 Feb 21, 2021

pacoxu Feb 23, 2021

neolit123 Feb 24, 2021

neolit123 Feb 21, 2021 •

edited

alculquicondor Feb 23, 2021

neolit123 Feb 23, 2021

alculquicondor Feb 23, 2021

neolit123 Feb 23, 2021 •

edited

neolit123 commented Feb 21, 2021

snowplayfire commented May 13, 2022

kubelet waits for node lister to sync at least once #94087

kubelet waits for node lister to sync at least once #94087

Conversation

derekwaynecarr commented Aug 18, 2020

k8s-ci-robot commented Aug 18, 2020

derekwaynecarr commented Aug 18, 2020

derekwaynecarr commented Aug 18, 2020

sjenning commented Aug 18, 2020

derekwaynecarr commented Aug 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Aug 31, 2020

derekwaynecarr commented Jan 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Feb 21, 2021 • edited

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

neolit123 Feb 23, 2021 • edited

Choose a reason for hiding this comment

neolit123 commented Feb 21, 2021

snowplayfire commented May 13, 2022

neolit123 Feb 21, 2021 •

edited

neolit123 Feb 23, 2021 •

edited