Repository: kubernetes/kubernetes (language: go)

Issue:
[FG:InPlacePodVerticalScaling] Improve allocated resources checkpointing
There is some low-hanging fruit for improving the checkpointing of allocated resources:

1. When the pod allocation is updated, the Kubelet always calls set allocation at the pod level, but the status manager sets it for each container, calling through to the checkpoint state. This causes the checkpoint file to be written in quick succession for each container in the pod. It would be better to just update the checkpoint once per-pod.
    - set pod allocation for each container https://github.com/kubernetes/kubernetes/blob/e8b59afec65762ec33c683f376d5a1e5959476c2/pkg/kubelet/status/status_manager.go#L254
    - write the checkpoint for each container: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/status/state/state_checkpoint.go#L138
2. ~~Failure to write the checkpoint should be logged, but not block the resize from proceeding with the in-memory cache. The checkpoint is only used on kubelet restart, and the failure state for that looks the same whether or not syncing the pod proceeds. Better to treat the checkpointing as best effort.~~ I changed my mind about this. Better to not mask the problem. If the checkpoint can't be written on resize, better to leave the resize in a deferred state, otherwise we risk the node becoming unstable on restart.

/kind feature
/sig node
/priority important-longterm


Discussion / hints:
/milestone v1.32
/triage accepted
/assign
Hi @tallclair 
I would like to send a gentle remind. The code freeze is starting 02:00 UTC Friday November 8th 2024 (about 3 weeks from now), and while there is still time, we want to ensure that each PR has a chance to be merged on time.
As the PR is tagged for 1.32, is it still planned for this release?
Thank you!
Hey @tallclair
⚠️ Do we still intend to merge this for v1.32? Just a reminder that the code freeze is starting 02:00 UTC Friday November 8th 2024 (a little less than 1 week from now). Please make sure the PR has both lgtm and approved labels before the code freeze. Thanks!
Since this isn't high priority and code freeze has passed, we're going to remove the 1.32 milestone.

/milestone clear
/milestone v1.33
/assign

Fix patch:
diff --git a/pkg/kubelet/status/state/state.go b/pkg/kubelet/status/state/state.go
index 1ed6606d15b5e..6fcd784ef1c09 100644
--- a/pkg/kubelet/status/state/state.go
+++ b/pkg/kubelet/status/state/state.go
@@ -47,6 +47,7 @@ type Reader interface {
 
 type writer interface {
 	SetContainerResourceAllocation(podUID string, containerName string, alloc v1.ResourceRequirements) error
+	SetPodResourceAllocation(podUID string, alloc map[string]v1.ResourceRequirements) error
 	SetPodResizeStatus(podUID string, resizeStatus v1.PodResizeStatus)
 	Delete(podUID string, containerName string) error
 }
diff --git a/pkg/kubelet/status/state/state_checkpoint.go b/pkg/kubelet/status/state/state_checkpoint.go
index 6ff93ad25c81e..8a37da184e3c9 100644
--- a/pkg/kubelet/status/state/state_checkpoint.go
+++ b/pkg/kubelet/status/state/state_checkpoint.go
@@ -127,6 +127,17 @@ func (sc *stateCheckpoint) SetContainerResourceAllocation(podUID string, contain
 	return sc.storeState()
 }
 
+// SetPodResourceAllocation sets pod resource allocation
+func (sc *stateCheckpoint) SetPodResourceAllocation(podUID string, alloc map[string]v1.ResourceRequirements) error {
+	sc.mux.Lock()
+	defer sc.mux.Unlock()
+	err := sc.cache.SetPodResourceAllocation(podUID, alloc)
+	if err != nil {
+		return err
+	}
+	return sc.storeState()
+}
+
 // SetPodResizeStatus sets the last resize decision for a pod
 func (sc *stateCheckpoint) SetPodResizeStatus(podUID string, resizeStatus v1.PodResizeStatus) {
 	sc.mux.Lock()
@@ -165,6 +176,10 @@ func (sc *noopStateCheckpoint) SetContainerResourceAllocation(_ string, _ string
 	return nil
 }
 
+func (sc *noopStateCheckpoint) SetPodResourceAllocation(_ string, _ map[string]v1.ResourceRequirements) error {
+	return nil
+}
+
 func (sc *noopStateCheckpoint) SetPodResizeStatus(_ string, _ v1.PodResizeStatus) {}
 
 func (sc *noopStateCheckpoint) Delete(_ string, _ string) error {
diff --git a/pkg/kubelet/status/state/state_mem.go b/pkg/kubelet/status/state/state_mem.go
index 193c55f7e052b..8ed47872755ac 100644
--- a/pkg/kubelet/status/state/state_mem.go
+++ b/pkg/kubelet/status/state/state_mem.go
@@ -77,6 +77,15 @@ func (s *stateMemory) SetContainerResourceAllocation(podUID string, containerNam
 	return nil
 }
 
+func (s *stateMemory) SetPodResourceAllocation(podUID string, alloc map[string]v1.ResourceRequirements) error {
+	s.Lock()
+	defer s.Unlock()
+
+	s.podAllocation[podUID] = alloc
+	klog.V(3).InfoS("Updated pod resource allocation", "podUID", podUID, "allocation", alloc)
+	return nil
+}
+
 func (s *stateMemory) SetPodResizeStatus(podUID string, resizeStatus v1.PodResizeStatus) {
 	s.Lock()
 	defer s.Unlock()
diff --git a/pkg/kubelet/status/status_manager.go b/pkg/kubelet/status/status_manager.go
index c2d7b9485e6bf..e6f89b1f627ff 100644
--- a/pkg/kubelet/status/status_manager.go
+++ b/pkg/kubelet/status/status_manager.go
@@ -310,25 +310,23 @@ func (m *manager) GetPodResizeStatus(podUID types.UID) v1.PodResizeStatus {
 func (m *manager) SetPodAllocation(pod *v1.Pod) error {
 	m.podStatusesLock.RLock()
 	defer m.podStatusesLock.RUnlock()
+
+	podAlloc := make(map[string]v1.ResourceRequirements)
 	for _, container := range pod.Spec.Containers {
 		alloc := *container.Resources.DeepCopy()
-		if err := m.state.SetContainerResourceAllocation(string(pod.UID), container.Name, alloc); err != nil {
-			return err
-		}
+		podAlloc[container.Name] = alloc
 	}
 
 	if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) {
 		for _, container := range pod.Spec.InitContainers {
 			if podutil.IsRestartableInitContainer(&container) {
 				alloc := *container.Resources.DeepCopy()
-				if err := m.state.SetContainerResourceAllocation(string(pod.UID), container.Name, alloc); err != nil {
-					return err
-				}
+				podAlloc[container.Name] = alloc
 			}
 		}
 	}
 
-	return nil
+	return m.state.SetPodResourceAllocation(string(pod.UID), podAlloc)
 }
 
 // SetPodResizeStatus checkpoints the last resizing decision for the pod.


Test patch:
diff --git a/pkg/kubelet/status/state/state_checkpoint_test.go b/pkg/kubelet/status/state/state_checkpoint_test.go
index 9a927b60372c8..02ffc0d367a15 100644
--- a/pkg/kubelet/status/state/state_checkpoint_test.go
+++ b/pkg/kubelet/status/state/state_checkpoint_test.go
@@ -110,11 +110,9 @@ func Test_stateCheckpoint_storeState(t *testing.T) {
 			originalSC, err := NewStateCheckpoint(testDir, testCheckpoint)
 			require.NoError(t, err)
 
-			for podUID, containerAlloc := range tt.args.podResourceAllocation {
-				for containerName, alloc := range containerAlloc {
-					err = originalSC.SetContainerResourceAllocation(podUID, containerName, alloc)
-					require.NoError(t, err)
-				}
+			for podUID, alloc := range tt.args.podResourceAllocation {
+				err = originalSC.SetPodResourceAllocation(podUID, alloc)
+				require.NoError(t, err)
 			}
 
 			actual := originalSC.GetPodResourceAllocation()