kubelet: Refactor tryUpdateNodeStatus() into smaller functions #113466

jingyuanliang · 2022-10-31T10:09:05Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Support #112618. This replaces #113188.

Which issue(s) this PR fixes:

This breaks down tryUpdateNodeStatus into multiple functions.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2022-10-31T10:09:14Z

@jingyuanliang: 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.

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.

k8s-ci-robot · 2022-10-31T10:09:14Z

Hi @jingyuanliang. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2022-10-31T10:09:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jingyuanliang
Once this PR has been reviewed and has the lgtm label, please assign derekwaynecarr for approval by writing /assign @derekwaynecarr in a comment. For more information see the Kubernetes Code Review Process.

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

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

jingyuanliang · 2022-10-31T10:12:15Z

/cc @aojea
/cc @mborsz
/cc @MrHohn

aojea · 2022-10-31T18:33:46Z

pkg/kubelet/kubelet_node_status.go

 	if originalNode == nil {
 		return fmt.Errorf("nil %q node object", kl.nodeName)
 	}

+	node, changed := kl.updateNode(originalNode)
+
+	if kl.clock.Since(kl.lastStatusReportTime) < kl.nodeStatusReportFrequency && !changed {


ok, this !changed is an optimization, right?

ah, no, it is !podCIDRChanged && !nodeStatusHasChanged(&originalNode.Status, &node.Status) && !areRequiredLabelsNotPresent

aojea · 2022-10-31T18:36:27Z

pkg/kubelet/kubelet_node_status.go

+
+	updatedNode, err := kl.patchNode(originalNode, node)
+	if err == nil {
+		kl.markVolumesFromNode(updatedNode)


Suggested change

kl.markVolumesFromNode(updatedNode)

// If update finishes successfully, mark the volumeInUse as reportedInUse to indicate

// those volumes are already updated in the node's status

kl.markVolumesFromNode(updatedNode)

I suggest to keep the comments

This comment was moved to the top of markVolumesFromNode.

oh, thanks, sorry

aojea · 2022-10-31T18:41:25Z

pkg/kubelet/kubelet_node_status.go


+// patchNode patches node on the API server based on originalNode.
+// It returns any potential error, or an updatedNode and refreshes the state of kubelet when successful.
+func (kl *Kubelet) patchNode(originalNode, node *v1.Node) (*v1.Node, error) {


it should be patchNodeStatus

aojea · 2022-10-31T19:11:30Z

so, right now it seems that the update node process is undocumented, if we break it down and we start to use independent fragments I'm not sure we'll respect the kubelet update node process now or in the future unless we create a test that defines this process

jingyuanliang · 2022-10-31T19:43:37Z

Is the current tryUpdateNodeStatus() already covered by sufficient amount of tests?

aojea · 2022-11-02T00:05:42Z

Is the current tryUpdateNodeStatus() already covered by sufficient amount of tests?

I don't think we have good tests in this area, I remember one regression that increased the time to start kubelet #99336 and went undetected

I really think we should do this, but I'm scared of regressions ...

What is your intent with this patch, I assume that this is part of a larger change, can you add subsequent commits so we can review the whole solution proposed?

aojea · 2022-11-02T00:05:48Z

/ok-to-test

jingyuanliang · 2022-11-02T03:58:03Z

#112618 was updated to show the use with this refactor. Let's keep this PR open so we can run tests on the refactor alone.

aojea · 2022-11-10T11:40:01Z

/close

superseded by #112618

k8s-ci-robot · 2022-11-10T11:40:08Z

@aojea: Closed this PR.

In response to this:

/close

superseded by #112618

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.

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 31, 2022

k8s-ci-robot requested review from feiskyer and sjenning October 31, 2022 10:10

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 Oct 31, 2022

jingyuanliang mentioned this pull request Oct 31, 2022

kubelet: Keep trying fast status update at startup until node is ready #112618

Merged

k8s-ci-robot requested review from aojea, mborsz and MrHohn October 31, 2022 10:12

jingyuanliang force-pushed the tryUpdateNodeStatusRefactor branch from 704a6a4 to add5312 Compare October 31, 2022 10:20

jingyuanliang changed the title ~~kubelet: Refactor tryUpdateNodeStatus()~~ kubelet: Refactor tryUpdateNodeStatus() into smaller functions Oct 31, 2022

jingyuanliang force-pushed the tryUpdateNodeStatusRefactor branch from add5312 to 692ba91 Compare October 31, 2022 10:22

aojea reviewed Oct 31, 2022

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 2, 2022

jingyuanliang force-pushed the tryUpdateNodeStatusRefactor branch 2 times, most recently from 18312cb to 4e1145e Compare November 2, 2022 03:38

jingyuanliang mentioned this pull request Nov 2, 2022

kubelet: UpdateNodeStatusFrom* options in [sync|update]NodeStatus() #113188

Closed

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

jingyuanliang force-pushed the tryUpdateNodeStatusRefactor branch from 4e1145e to 983ac5d Compare November 4, 2022 20:53

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

jingyuanliang force-pushed the tryUpdateNodeStatusRefactor branch from 983ac5d to 3729fe9 Compare November 5, 2022 00:26

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

jingyuanliang force-pushed the tryUpdateNodeStatusRefactor branch from 3729fe9 to 34ac70a Compare November 7, 2022 05:50

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

kubelet: Refactor tryUpdateNodeStatus() into smaller functions

3dc172a

jingyuanliang force-pushed the tryUpdateNodeStatusRefactor branch from 34ac70a to 3dc172a Compare November 7, 2022 09:48

SergeyKanzhelev added this to Triage in SIG Node PR Triage Nov 9, 2022

k8s-ci-robot closed this Nov 10, 2022

SIG Node PR Triage automation moved this from Triage to Done Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet: Refactor tryUpdateNodeStatus() into smaller functions #113466

kubelet: Refactor tryUpdateNodeStatus() into smaller functions #113466

jingyuanliang commented Oct 31, 2022 •

edited

k8s-ci-robot commented Oct 31, 2022

k8s-ci-robot commented Oct 31, 2022

k8s-ci-robot commented Oct 31, 2022

jingyuanliang commented Oct 31, 2022

aojea Oct 31, 2022

aojea Oct 31, 2022

aojea Oct 31, 2022

jingyuanliang Oct 31, 2022

aojea Nov 1, 2022

aojea Oct 31, 2022

aojea commented Oct 31, 2022

jingyuanliang commented Oct 31, 2022 •

edited

aojea commented Nov 2, 2022

aojea commented Nov 2, 2022

jingyuanliang commented Nov 2, 2022

aojea commented Nov 10, 2022

k8s-ci-robot commented Nov 10, 2022

kubelet: Refactor tryUpdateNodeStatus() into smaller functions #113466

kubelet: Refactor tryUpdateNodeStatus() into smaller functions #113466

Conversation

jingyuanliang commented Oct 31, 2022 • edited

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.:

k8s-ci-robot commented Oct 31, 2022

k8s-ci-robot commented Oct 31, 2022

k8s-ci-robot commented Oct 31, 2022

jingyuanliang commented Oct 31, 2022

aojea Oct 31, 2022

Choose a reason for hiding this comment

aojea Oct 31, 2022

Choose a reason for hiding this comment

aojea Oct 31, 2022

Choose a reason for hiding this comment

jingyuanliang Oct 31, 2022

Choose a reason for hiding this comment

aojea Nov 1, 2022

Choose a reason for hiding this comment

aojea Oct 31, 2022

Choose a reason for hiding this comment

aojea commented Oct 31, 2022

jingyuanliang commented Oct 31, 2022 • edited

aojea commented Nov 2, 2022

aojea commented Nov 2, 2022

jingyuanliang commented Nov 2, 2022

aojea commented Nov 10, 2022

k8s-ci-robot commented Nov 10, 2022

jingyuanliang commented Oct 31, 2022 •

edited

jingyuanliang commented Oct 31, 2022 •

edited