Ensure that kubelet updates state on restart by lentzi90 · Pull Request #130305 · kubernetes/kubernetes

lentzi90 · 2025-02-20T07:18:22Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

If the last status report time is zero, we consider the update status period expired to ensure that the status is properly updated on restarts. Otherwise, setLastObservedNodeAddresses is never called until there is a change in the node's status. The --node-status-update-frequency flag is also not respected, since the status will not be updated until there is an actual change.

After the first update to the nodes status (when lastStatusReportTime is updated to a non-zero value), the updates continue as expected, respecting the --node-status-update-frequency. So this is only about the very first update after a kubelet (re)start.

The behavior changed in #128640, from

shouldPatchNodeStatus := changed || kl.clock.Since(kl.lastStatusReportTime) >= kl.nodeStatusReportFrequency

to

kubernetes/pkg/kubelet/kubelet_node_status.go

Lines 605 to 608 in ad4d9b2

    
           func (kl *Kubelet) isUpdateStatusPeriodExperid() bool { 
        
           	if kl.lastStatusReportTime.IsZero() { 
        
           		return false 
        
           	}

Note that the zero case changed. The status used to be forcefully updated if the lastStatusReportTime was zero. Now it is not touched in that case.

The specific thing that this change breaks for us is described in more detail in #130001. The gist is that the kubelet server certificate CSR cannot be created until getLastObservedNodeAddresses returns an actual address. This function relies on the kubelets internal state, NOT the node status, and this internal state is not set until patchNodeStatus has been called because that is the only place setLastObservedNodeAddresses is called.

This all means that after a kubelet (re)start, as long as the node status hasn't changed, the kubelet cannot create the CSR for the serving certificate. And the --node-status-update-frequency flag isn't respected.

Which issue(s) this PR fixes:

Fixes #130001

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Ensure Kubelet state is initialized on start even if the node hasn't changed

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2025-02-20T07:18:32Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Details

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-sigs/prow repository.

aojea · 2025-02-20T08:58:47Z

 		expectExpired              bool
 	}{
 		{
+			// Ensure that everything is properly initialized if the node status is not updated before


this is why we have the detection of changes

kubernetes/pkg/kubelet/kubelet_node_status.go

Line 485 in ad4d9b2

node, changed := kl.updateNode(ctx, originalNode)

we are trying to be conservative with updates, can't we detect that we need to update instead of blindly update the node after the restart?

But the issue is that there is no change. From what I understand, this updateNode is making sure the Node status is up to date. If we just restart the kubelet there is no change in the status and no reason to update anything. We need to make sure setLastObservedNodeAddresses is called once even if no change has been made. Maybe we can add it to updateNode?

Just wanted to add that we only ever end up in this situation if the node status has NEVER (since kubelet start) been updated. So it adds a "forced" update on every kubelet (re)start, which is actually the same as the behavior before #128640. It would then update the node status after the status report interval no matter if there was a change or not.

sftim · 2025-02-20T09:40:27Z

Does this PR introduce an improvement that cluster operators could notice? I think it does, and so there should be a changelog entry.

lentzi90 · 2025-02-20T10:16:12Z

Updated description to add changelog entry. Is there a way to mark that #128640 combined with this PR essentially means no change? I.e. this PR (AFAIC) fixes a startup bug introduced in #128640 and makes the behavior the same as it was before.
What I am trying to say is that I wonder if it is confusing that the changelog message now suggests a change even when these two PRs together means that things stay the same (obviously ignore the other changes introduced in that PR)

Edit: Nevermind, of course this should show up as a change since the change was already in v1.32.0. I don't know what I was thinking. Somehow I thought it wasn't released. I guess I am too used to testing unreleased things.

aojea · 2025-02-20T12:14:35Z

 	if kl.lastStatusReportTime.IsZero() {
-		return false
+		return true
 	}


so this is the problem, if we remove it , we keep exactly the same logic, the thing is that now is more obvius.

Can you please also expand on the comment to explain the problem in more detail and reference the bug you opened?

If the node.status is never updated after a kubelet restart, then the periodic update loop is never executed

something like that

Sure! Updated 🙂

aojea · 2025-02-20T12:16:41Z

/sig node
/assign @SergeyKanzhelev @yujuhong
/cc @liggitt
/priority important-soon
/kind regression

Thanks for the detailed investigation, reviewers please check the referenced issue for more context, but the bottomline is that we may never do the periodic update after a kubelet restart if there are no changes on the node, as a consequence some race happens with certificates and we don't know if there are more things depending on this

If the last status report time is zero, we consider the update status period expired to ensure that the status is properly updated on restarts. Otherwise, there is a risk that the kubelets internal state (e.g. addresses and hostname) is never set until there is a change in the node's status. Signed-off-by: Lennart Jern <lennart.jern@est.tech>

k8s-ci-robot · 2025-02-20T13:11:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lentzi90
Once this PR has been reviewed and has the lgtm label, please ask for approval from sergeykanzhelev. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

pkg/kubelet/OWNERS

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

liggitt · 2025-02-20T20:29:43Z

+	if !changed && !kl.isUpdateStatusPeriodExpired() {
 		kl.markVolumesFromNode(node)
 		return nil
 	}


if updateNode returns changed==false, meaning there is no change in what we want to report for the node status, why is it important to call patchNodeStatus once on node startup?

I don't really understand the description of this PR - "there is a risk that the kubelets internal state (e.g. addresses and hostname) is never set until there is a change in the node's status" ... shouldn't changed be true if that is the case?

Sorry, I have updated the description to clarify. The gist is that the kubelet service certificate CSR cannot be created until getLastObservedNodeAddresses returns an actual address, and that doesn't happen until setLastObservedNodeAddresses has been called, which doesn't happen anywhere else than patchNodeStatus.
Note that the current behavior is also a clear change from how it was before #128640 and that it doesn't respect --node-status-update-frequency until the node status has changed the first time. After that it will happily update it regularly, but before there is any change, it will just wait indefinitely.

I see. I think we should revert #128640, pick the revert to 1.32, and rework it in 1.33, since that caused the regression... the isUpdateStatusPeriodExpired function is really hard to reason about for zero-value timestamps

aojea · 2025-02-21T16:31:46Z

/hold

we are going with the revert #130348

aojea · 2025-02-21T16:32:58Z

@lentzi90 if you depend on this behavior I encourage you to create an unit or integration test that ensure this does not regress anymore in the future, it can be tricky but it will be worth it if you depend on it

k8s-ci-robot · 2025-02-21T19:55:16Z

PR needs rebase.

Details

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-sigs/prow repository.

lentzi90 · 2025-02-24T06:32:43Z

Closing, since the revert makes the issue go away. I will try to figure out how to make a test that can catch this

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 20, 2025

k8s-ci-robot requested review from andrewsykim and dims February 20, 2025 07:18

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 20, 2025

aojea reviewed Feb 20, 2025

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 20, 2025

lentzi90 changed the title ~~Ensure that kubelet updates status on restart~~ Ensure that kubelet updates state on restart Feb 20, 2025

aojea reviewed Feb 20, 2025

View reviewed changes

k8s-ci-robot assigned SergeyKanzhelev and yujuhong Feb 20, 2025

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Feb 20, 2025

k8s-ci-robot requested a review from liggitt February 20, 2025 12:16

k8s-ci-robot added kind/regression Categorizes issue or PR as related to a regression from a prior release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 20, 2025

lentzi90 force-pushed the lentzi90/kubelet-ensure-init-update-status branch from c76a52d to 0e38b0a Compare February 20, 2025 13:11

liggitt reviewed Feb 20, 2025

View reviewed changes

This was referenced Feb 21, 2025

Add random interval to nodeStatusReport interval every time after an actual node status change #128640

Merged

Revert "Add random interval to nodeStatusReport interval every time after an actual node status change" #130348

Merged

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

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2025

lentzi90 closed this Feb 24, 2025

	func (kl *Kubelet) isUpdateStatusPeriodExperid() bool {
	if kl.lastStatusReportTime.IsZero() {
	return false
	}

Conversation

lentzi90 commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sftim commented Feb 20, 2025

Uh oh!

lentzi90 commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea commented Feb 20, 2025

Uh oh!

k8s-ci-robot commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea commented Feb 21, 2025

Uh oh!

aojea commented Feb 21, 2025

Uh oh!

k8s-ci-robot commented Feb 21, 2025

Uh oh!

lentzi90 commented Feb 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lentzi90 commented Feb 20, 2025 •

edited

Loading

lentzi90 commented Feb 20, 2025 •

edited

Loading

liggitt Feb 21, 2025 •

edited

Loading