Repository: kubernetes/kubernetes (language: go)

Issue:
kubelet periodic node status update should not be aligned
### What happened?

The default `nodeStatusUpdateFrequency` is 10s with 4% jitter.
The default `nodeStatusReportFrequency` is 5m.

The node status will be updated either there's a change or it has been longer than `nodeStatusReportFrequency` since the previous update.
`nodeStatusReportFrequency` 
https://github.com/kubernetes/kubernetes/blob/55b83c92b3b69cd53d5bf22b8ccff859a005241a/pkg/kubelet/kubelet_node_status.go#L580-L581

The jitter of `nodeStatusUpdateFrequency` won't effectively make the periodic node status update to be spread evenly over the 5-minute window.
The node status update requests from thousands of nodes are still sent at almost the same time after a couple of days. It can cause the CPU usage to spike every 5 minutes.

The following is the APIServer CPU utilization on 96CPU instances. The cluster has 5k nodes that are added in a short period of time. The cluster is idle without any application workload. The utilization can go up to 64% and go down to 14% just because of these  node status update requests.
<img width="1756" alt="Screenshot 2024-04-05 at 12 59 16 PM" src="https://github.com/kubernetes/kubernetes/assets/2589105/cb9eac79-1c12-4baa-84d9-0c7c639d4d40">


### What did you expect to happen?

I'd expect the node status update requests from kubelet to be spread evenly so that the apiserver won't have a minute that it has to handle the requests from most nodes every 5 minutes. 

### How can we reproduce it (as minimally and precisely as possible)?

Bring up a cluster and add nodes in a short period of time.

### Anything else we need to know?
Ref: https://github.com/kubernetes/kubernetes/pull/105272/files

_No response_

### Kubernetes version

<details>
Cutting edge 1.30
```console
$ kubectl version
# paste output here
```

</details>


### Cloud provider

<details>
EKS
</details>


### OS version

<details>

```console
# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here
```

</details>


### Install tools

<details>

</details>


### Container runtime (CRI) and version (if applicable)

<details>

</details>


### Related plugins (CNI, CSI, ...) and versions (if applicable)

<details>

</details>



Discussion / hints:
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](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
cc: @wojtek-t  since you wrote https://github.com/kubernetes/kubernetes/pull/105272/files

/sig node
/sig scalability
If the node used a random timer (eg uniform over 1m to 5m) for its first status update, and then stuck to _n_ second intervals with small jitter, that would go a long way towards smoothing out load in this scenario.
I also wonder if this is more of a feature request than a bug.
/remove-kind bug
/kind feature

Removing the bug label as this is a feature request. We welcome any changes for this issue.
The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle stale`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle stale

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle rotten`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle rotten

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/
/remove-lifecycle rotten
@mengqiy trying for a fix in 1.32? (reminder code freeze is very close!)
@dims @sftim Created PR https://github.com/kubernetes/kubernetes/pull/128394 and hopefully we can get it merged before 1.32 code freeze.

Fix patch:
diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go
index 6c1019dca3e41..c498027ec8698 100644
--- a/pkg/kubelet/kubelet.go
+++ b/pkg/kubelet/kubelet.go
@@ -1240,6 +1240,12 @@ type Kubelet struct {
 	// status to master. It is only used when node lease feature is enabled.
 	nodeStatusReportFrequency time.Duration
 
+	// delayAfterNodeStatusChange is the one-time random duration that we add to the next node status report interval
+	// every time when there's an actual node status change. But all future node status update that is not caused by
+	// real status change will stick with nodeStatusReportFrequency. The random duration is a uniform distribution over
+	// [-0.5*nodeStatusReportFrequency, 0.5*nodeStatusReportFrequency]
+	delayAfterNodeStatusChange time.Duration
+
 	// lastStatusReportTime is the time when node status was last reported.
 	lastStatusReportTime time.Time
 
diff --git a/pkg/kubelet/kubelet_node_status.go b/pkg/kubelet/kubelet_node_status.go
index ebe36617f05b9..0505821a75bbb 100644
--- a/pkg/kubelet/kubelet_node_status.go
+++ b/pkg/kubelet/kubelet_node_status.go
@@ -19,6 +19,7 @@ package kubelet
 import (
 	"context"
 	"fmt"
+	"math/rand"
 	"net"
 	goruntime "runtime"
 	"sort"
@@ -579,13 +580,21 @@ func (kl *Kubelet) tryUpdateNodeStatus(ctx context.Context, tryNumber int) error
 	}
 
 	node, changed := kl.updateNode(ctx, originalNode)
-	shouldPatchNodeStatus := changed || kl.clock.Since(kl.lastStatusReportTime) >= kl.nodeStatusReportFrequency
-
-	if !shouldPatchNodeStatus {
+	// no need to update the status yet
+	if !changed && !kl.isUpdateStatusPeriodExperid() {
 		kl.markVolumesFromNode(node)
 		return nil
 	}
 
+	// We need to update the node status, if this is caused by a node change we want to calculate a new
+	// random delay so we avoid all the nodes to reach the apiserver at the same time. If the update is not related
+	// to a node change, because we run over the period, we reset the random delay so the node keeps updating
+	// its status at the same cadence
+	if changed {
+		kl.delayAfterNodeStatusChange = kl.calculateDelay()
+	} else {
+		kl.delayAfterNodeStatusChange = 0
+	}
 	updatedNode, err := kl.patchNodeStatus(originalNode, node)
 	if err == nil {
 		kl.markVolumesFromNode(updatedNode)
@@ -593,6 +602,20 @@ func (kl *Kubelet) tryUpdateNodeStatus(ctx context.Context, tryNumber int) error
 	return err
 }
 
+func (kl *Kubelet) isUpdateStatusPeriodExperid() bool {
+	if kl.lastStatusReportTime.IsZero() {
+		return false
+	}
+	if kl.clock.Since(kl.lastStatusReportTime) >= kl.nodeStatusReportFrequency+kl.delayAfterNodeStatusChange {
+		return true
+	}
+	return false
+}
+
+func (kl *Kubelet) calculateDelay() time.Duration {
+	return time.Duration(float64(kl.nodeStatusReportFrequency) * (-0.5 + rand.Float64()))
+}
+
 // updateNode creates a copy of originalNode and runs update logic on it.
 // It returns the updated node object and a bool indicating if anything has been changed.
 func (kl *Kubelet) updateNode(ctx context.Context, originalNode *v1.Node) (*v1.Node, bool) {


Test patch:
diff --git a/pkg/kubelet/kubelet_node_status_test.go b/pkg/kubelet/kubelet_node_status_test.go
index 64eca034457d5..12a6835fd5d6e 100644
--- a/pkg/kubelet/kubelet_node_status_test.go
+++ b/pkg/kubelet/kubelet_node_status_test.go
@@ -849,7 +849,10 @@ func TestUpdateNodeStatusWithLease(t *testing.T) {
 	// Since this test retroactively overrides the stub container manager,
 	// we have to regenerate default status setters.
 	kubelet.setNodeStatusFuncs = kubelet.defaultNodeStatusFuncs()
-	kubelet.nodeStatusReportFrequency = time.Minute
+	// You will add up to 50% of nodeStatusReportFrequency of additional random latency for
+	// kubelet to determine if update node status is needed due to time passage. We need to
+	// take that into consideration to ensure this test pass all time.
+	kubelet.nodeStatusReportFrequency = 30 * time.Second
 
 	kubeClient := testKubelet.fakeKubeClient
 	existingNode := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: testKubeletHostname}}
@@ -3088,3 +3091,67 @@ func TestUpdateNodeAddresses(t *testing.T) {
 		})
 	}
 }
+
+func TestIsUpdateStatusPeriodExperid(t *testing.T) {
+	testcases := []struct {
+		name                       string
+		lastStatusReportTime       time.Time
+		delayAfterNodeStatusChange time.Duration
+		expectExpired              bool
+	}{
+		{
+			name:                       "no status update before and no delay",
+			lastStatusReportTime:       time.Time{},
+			delayAfterNodeStatusChange: 0,
+			expectExpired:              false,
+		},
+		{
+			name:                       "no status update before and existing delay",
+			lastStatusReportTime:       time.Time{},
+			delayAfterNodeStatusChange: 30 * time.Second,
+			expectExpired:              false,
+		},
+		{
+			name:                       "not expired and no delay",
+			lastStatusReportTime:       time.Now().Add(-4 * time.Minute),
+			delayAfterNodeStatusChange: 0,
+			expectExpired:              false,
+		},
+		{
+			name:                       "not expired",
+			lastStatusReportTime:       time.Now().Add(-5 * time.Minute),
+			delayAfterNodeStatusChange: time.Minute,
+			expectExpired:              false,
+		},
+		{
+			name:                       "expired",
+			lastStatusReportTime:       time.Now().Add(-4 * time.Minute),
+			delayAfterNodeStatusChange: -2 * time.Minute,
+			expectExpired:              true,
+		},
+	}
+
+	testKubelet := newTestKubelet(t, false /* controllerAttachDetachEnabled */)
+	defer testKubelet.Cleanup()
+	kubelet := testKubelet.kubelet
+	kubelet.nodeStatusReportFrequency = 5 * time.Minute
+
+	for _, tc := range testcases {
+		kubelet.lastStatusReportTime = tc.lastStatusReportTime
+		kubelet.delayAfterNodeStatusChange = tc.delayAfterNodeStatusChange
+		expired := kubelet.isUpdateStatusPeriodExperid()
+		assert.Equal(t, tc.expectExpired, expired, tc.name)
+	}
+}
+
+func TestCalculateDelay(t *testing.T) {
+	testKubelet := newTestKubelet(t, false /* controllerAttachDetachEnabled */)
+	defer testKubelet.Cleanup()
+	kubelet := testKubelet.kubelet
+	kubelet.nodeStatusReportFrequency = 5 * time.Minute
+
+	for i := 0; i < 100; i++ {
+		randomDelay := kubelet.calculateDelay()
+		assert.LessOrEqual(t, randomDelay.Abs(), kubelet.nodeStatusReportFrequency/2)
+	}
+}