Update kubelet node status report logic with node lease feature #69753

wangzhen127 · 2018-10-12T23:30:59Z

What this PR does / why we need it:
When node lease feature is enabled, kubelet reports node status to api server only if there is some change or it didn't report over last report interval. This is part of KEP-0009, feature #589 and issue #14733.

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 #

Special notes for your reviewer:

Release note:

kubelet: When node lease feature is enabled, kubelet reports node status to api server only if there is some change or it didn't report over last report interval.

wangzhen127 · 2018-10-12T23:31:58Z

/assign @wojtek-t
/assign @yujuhong

wangzhen127 · 2018-10-12T23:32:23Z

/kind feature

neolit123 · 2018-10-13T12:53:06Z

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews

wojtek-t

Looks good overall - minor comments mostly.

wojtek-t · 2018-10-15T09:04:45Z

pkg/kubelet/kubelet_node_status.go

if there is any change or enough time passed from the last sync, right?

wojtek-t · 2018-10-16T08:41:35Z

pkg/kubelet/kubelet_node_status.go

if updatePodCIDR would fail, theoretically node could not change.
But I'm assuming in that case, it doesn't cost us much to report status anyway, so better to be on the safe side.

Maybe worth adding a comment?

I changed the logic here. PTAL

wojtek-t · 2018-10-16T08:47:12Z

pkg/kubelet/kubelet_node_status.go

In general I'm not a fan of reflection if it's possible to avoid that.
How about something like that:

if nodeConditionsHaveChanged(originalStatus.Conditions, status.Conditions) { return true } originalConditions, conditions := originalStatus.Conditions. status.Conditions originalStatus.Conditions = nil status.Conditions = nil defer func() { originalStatus.Conditions = originalConditions status.Conditions = conditions } return !apiequality.Semantic.DeepEqual(originalStatus, status)

Done.

Just curious, why is reflection not preferred?

reflection is really slow - this is the reason why we autogenerate a bunch of code

wojtek-t · 2018-10-16T08:49:25Z

pkg/kubelet/kubelet_node_status.go

Aren't we generating conditions in the same deterministic order?
If so, maybe we can avoid sorting them?

@yujuhong - wdyt?

Kubelet does generate node conditions in a deterministic order, but there are other components that could patch the node status (e.g., NPD).

To be safe, I think we should still sort the conditions.

ok - sgtm. thanks

dashpole · 2018-10-16T17:01:48Z

pkg/kubelet/kubelet_node_status.go

is the assignment of nodeHasChanged to itself intentional here?

Good catch!

I think the assignment happens after ||. Anyway, I changed the logic here, so the original if statement is obsolete.

spiffxp · 2018-10-17T00:29:28Z

/milestone v1.13
I'm adding this to the v1.13 milestone since it relates to kubernetes/enhancements#589 which is currently planned for v1.13

yujuhong · 2018-10-17T18:24:29Z

pkg/kubelet/kubelet_node_status.go

Why do you still need to compare here if one of them is nil? Am I missing something?...

The full version is:

if originalStatus == nil && status == nil { return false } else if originalStatus == nil && status != nil { return true } else if originalStatus != nil && status == nil { return true }

So we still need the comparison here if we use a shorter version of code.

It feels quite unnecessary to invoke apiequality.Semantic.DeepEqual to check for the special cases (one of them is nil). I'd probably go with the code below, but I'll leave the decision to you.

if originalStatus == nil && status == nil { return false } if originalStatus == nil || status == nil { return true }

yujuhong · 2018-10-17T18:37:41Z

pkg/kubelet/kubelet_node_status.go

Kubelet does generate node conditions in a deterministic order, but there are other components that could patch the node status (e.g., NPD).

To be safe, I think we should still sort the conditions.

yujuhong · 2018-10-17T18:38:47Z

pkg/kubelet/kubelet_node_status.go

Good catch!

wangzhen127

Updated. PTAL

wangzhen127 · 2018-10-18T18:30:53Z

pkg/kubelet/kubelet_node_status.go

wangzhen127 · 2018-10-18T18:34:47Z

pkg/kubelet/kubelet_node_status.go

I changed the logic here. PTAL

wangzhen127 · 2018-10-18T19:16:12Z

pkg/kubelet/kubelet_node_status.go

I think the assignment happens after ||. Anyway, I changed the logic here, so the original if statement is obsolete.

wangzhen127 · 2018-10-18T19:25:04Z

pkg/kubelet/kubelet_node_status.go

The full version is:

if originalStatus == nil && status == nil { return false } else if originalStatus == nil && status != nil { return true } else if originalStatus != nil && status == nil { return true }

So we still need the comparison here if we use a shorter version of code.

wangzhen127 · 2018-10-18T19:34:39Z

pkg/kubelet/kubelet_node_status.go

Done.

Just curious, why is reflection not preferred?

wangzhen127 · 2018-10-18T19:57:32Z

pkg/kubelet/kubelet_network.go

Also note the change here. We need this return value to know if pod CIDR is actually changed or not.

wangzhen127

@liggitt, thanks for the review! I have updated the PR. PTAL

/test pull-kubernetes-node-e2e-alpha
/test pull-kubernetes-e2e-gce-alpha-features

wangzhen127 · 2018-11-06T18:46:13Z

pkg/kubelet/apis/config/types.go

Ah, I see your point. Updated the comments to make it more clear.

wangzhen127 · 2018-11-06T18:54:57Z

pkg/kubelet/apis/config/v1beta1/defaults.go

The update frequency and report frequency are independent according to KEP-0009. For example, when node lease feature is enabled, I think it is possible that one use wants to override update frequency to 20s and still keep the default (1 min) report frequency. In such case, the user would just specify the update frequency and the report frequency should not derive from the update frequency.

/cc @wojtek-t

wangzhen127 · 2018-11-06T19:17:57Z

pkg/kubelet/kubelet_node_status.go

tryUpdateNodeStatus does not touch label. I don't think we need to check labels here. Also confirmed with @yujuhong offline.

wangzhen127 · 2018-11-06T19:44:25Z

pkg/kubelet/kubelet_node_status.go

The conditions are merged by keys, where the keys are the type: "patchStrategy:"merge" patchMergeKey:"type"". See

kubernetes/staging/src/k8s.io/api/core/v1/types.go

Line 3962 in 8dcdec0

Conditions []NodeCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,4,rep,name=conditions"`

wangzhen127 · 2018-11-06T19:47:10Z

pkg/kubelet/kubelet_node_status.go

This is done every update frequency, i.e., once every 10 sec with default configuration. I updated it to use the copy now.

wangzhen127 · 2018-11-06T21:29:26Z

/retest
/test pull-kubernetes-node-e2e-alpha
/test pull-kubernetes-e2e-gce-alpha-features

wangzhen127 · 2018-11-06T23:08:09Z

/test pull-kubernetes-node-e2e-alpha
/test pull-kubernetes-e2e-gce-alpha-features

wangzhen127 · 2018-11-07T19:27:43Z

I have updated NodeStatusReportFrequency's default value to derive from NodeStatusUpdateFrequency if it is set explicitly. PTAL

/test pull-kubernetes-node-e2e-alpha
/test pull-kubernetes-e2e-gce-alpha-features

wojtek-t · 2018-11-07T19:41:11Z

/lgtm

wojtek-t · 2018-11-07T19:43:01Z

/approve

liggitt

/approve

/hold
noted a few documentation issues, @wojtek-t can re-lgtm once those are resolved

liggitt · 2018-11-07T19:43:34Z

pkg/kubelet/apis/config/types.go

nit: nodeStatusReportFrequency, as this value is used in documentation

liggitt · 2018-11-07T19:45:05Z

staging/src/k8s.io/kubelet/config/v1beta1/types.go

document the default uses the explicit nodeStatusUpdateFrequency value if provided, otherwise, 1m

include the improved documentation about what happens if a status update is computed in this comment as well

k8s-ci-robot · 2018-11-07T19:47:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, wangzhen127, wojtek-t, yujuhong

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~cmd/kubeadm/OWNERS~~ [wojtek-t]
~~pkg/kubelet/OWNERS~~ [wojtek-t,yujuhong]
~~staging/src/k8s.io/kubelet/config/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt,wojtek-t]

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

When node lease feature is enabled, kubelet reports node status to api server only if there is some change or it didn't report over last report interval.

wangzhen127

Updated the documentation. PTAL.

wangzhen127 · 2018-11-07T19:53:28Z

pkg/kubelet/apis/config/types.go

wangzhen127 · 2018-11-07T20:00:00Z

staging/src/k8s.io/kubelet/config/v1beta1/types.go

wangzhen127 · 2018-11-07T20:01:35Z

/test pull-kubernetes-node-e2e-alpha
/test pull-kubernetes-e2e-gce-alpha-features

wojtek-t · 2018-11-07T20:19:25Z

/lgtm

wangzhen127 · 2018-11-07T20:20:33Z

/hold cancel

yujuhong · 2018-11-07T21:42:38Z

/lgtm

wangzhen127 · 2018-11-07T21:45:33Z

/retest

krmayankk · 2019-01-28T08:37:50Z

pkg/kubelet/kubelet_node_status.go

+	for i := range conditionsCopy {
+		originalConditionsCopy[i].LastHeartbeatTime = replacedheartbeatTime
+		conditionsCopy[i].LastHeartbeatTime = replacedheartbeatTime
+		if !apiequality.Semantic.DeepEqual(&originalConditionsCopy[i], &conditionsCopy[i]) {


how does this work if additional conditions got added or removed in the node ?

It should just work, right?

krmayankk · 2019-01-28T08:40:25Z

pkg/kubelet/kubelet_node_status_test.go

+			},
+			status: &v1.NodeStatus{
+				Conditions: []v1.NodeCondition{readyCondition, memoryPressureCondition},
+			},


can we add a UT for a condition getting removed. So original has 2 or 3 and current node status has less or removed ?

Sure. Do you want to add one? I can help review. :)

k8s-ci-robot requested review from dashpole and feiskyer October 12, 2018 23:31

k8s-ci-robot assigned wojtek-t and yujuhong Oct 12, 2018

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 12, 2018

wojtek-t reviewed Oct 16, 2018

View reviewed changes

dashpole reviewed Oct 16, 2018

View reviewed changes

k8s-ci-robot added this to the v1.13 milestone Oct 17, 2018

yujuhong reviewed Oct 17, 2018

View reviewed changes

wangzhen127 force-pushed the diff-node-status branch from 4bebbe9 to 5bbc125 Compare October 18, 2018 19:56

wangzhen127 commented Oct 18, 2018

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 18, 2018

wangzhen127 commented Oct 18, 2018

View reviewed changes

wangzhen127 force-pushed the diff-node-status branch from 5bbc125 to 3979ed6 Compare October 18, 2018 20:22

wangzhen127 commented Nov 6, 2018

View reviewed changes

k8s-ci-robot requested a review from wojtek-t November 6, 2018 19:59

wangzhen127 force-pushed the diff-node-status branch from 2b8cad8 to 1d148d0 Compare November 6, 2018 23:01

wangzhen127 force-pushed the diff-node-status branch from 1d148d0 to 07b242a Compare November 7, 2018 19:26

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

liggitt reviewed Nov 7, 2018

View reviewed changes

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2018

Update kubelet node status report logic with node lease feature

98fc4a1

When node lease feature is enabled, kubelet reports node status to api server only if there is some change or it didn't report over last report interval.

wangzhen127 force-pushed the diff-node-status branch from 07b242a to 98fc4a1 Compare November 7, 2018 19:59

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2018

wangzhen127 commented Nov 7, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 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 Nov 7, 2018

k8s-ci-robot merged commit 16d0992 into kubernetes:master Nov 8, 2018

wangzhen127 deleted the diff-node-status branch November 8, 2018 04:30

Huang-Wei mentioned this pull request Jan 22, 2019

scheduler: makes pod less racing so as to be put back into activeQ properly #73078

Merged

krmayankk reviewed Jan 28, 2019

View reviewed changes

Update kubelet node status report logic with node lease feature #69753

Update kubelet node status report logic with node lease feature #69753

Uh oh!

Conversation

wangzhen127 commented Oct 12, 2018

Uh oh!

wangzhen127 commented Oct 12, 2018

Uh oh!

wangzhen127 commented Oct 12, 2018

Uh oh!

neolit123 commented Oct 13, 2018

Uh oh!

wojtek-t left a comment

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!

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!

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!

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!

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!

spiffxp commented Oct 17, 2018

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!

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!

wangzhen127 left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

wangzhen127 left a comment

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!

Choose a reason for hiding this comment