From 4a4748d23cc672fa21755852e81443230aaf81bb Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Mon, 4 Nov 2024 14:49:33 -0800 Subject: [PATCH] Determine resize status from state in handlePodResourcesResize --- pkg/kubelet/kubelet.go | 24 ++++++++------ pkg/kubelet/kubelet_pods.go | 9 ------ pkg/kubelet/kubelet_test.go | 63 ++++++++++++++++++++++++++++++------- 3 files changed, 66 insertions(+), 30 deletions(-) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 3e65c73100b..dc6d33363a8 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -1828,7 +1828,7 @@ func (kl *Kubelet) SyncPod(ctx context.Context, updateType kubetypes.SyncPodType // this conveniently retries any Deferred resize requests // TODO(vinaykul,InPlacePodVerticalScaling): Investigate doing this in HandlePodUpdates + periodic SyncLoop scan // See: https://github.com/kubernetes/kubernetes/pull/102884#discussion_r663160060 - pod, err = kl.handlePodResourcesResize(pod) + pod, err = kl.handlePodResourcesResize(pod, podStatus) if err != nil { return false, err } @@ -2793,12 +2793,20 @@ func (kl *Kubelet) canResizePod(pod *v1.Pod) (bool, v1.PodResizeStatus) { return true, v1.PodResizeStatusInProgress } -func (kl *Kubelet) handlePodResourcesResize(pod *v1.Pod) (*v1.Pod, error) { +// handlePodResourcesResize returns the "allocated pod", which should be used for all resource +// calculations after this function is called. It also updates the cached ResizeStatus according to +// the allocation decision and pod status. +func (kl *Kubelet) handlePodResourcesResize(pod *v1.Pod, podStatus *kubecontainer.PodStatus) (*v1.Pod, error) { allocatedPod, updated := kl.statusManager.UpdatePodFromAllocation(pod) if !updated { - // Unless a resize is in-progress, clear the resize status. - resizeStatus, _ := kl.statusManager.GetPodResizeStatus(string(pod.UID)) - if resizeStatus != v1.PodResizeStatusInProgress { + resizeInProgress := !allocatedResourcesMatchStatus(allocatedPod, podStatus) + if resizeInProgress { + // If a resize in progress, make sure the cache has the correct state in case the Kubelet restarted. + if err := kl.statusManager.SetPodResizeStatus(pod.UID, v1.PodResizeStatusInProgress); err != nil { + klog.ErrorS(err, "Failed to set resize status to InProgress", "pod", format.Pod(pod)) + } + } else { + // (Desired == Allocated == Actual) => clear the resize status. if err := kl.statusManager.SetPodResizeStatus(pod.UID, ""); err != nil { klog.ErrorS(err, "Failed to clear resize status", "pod", format.Pod(pod)) } @@ -2819,10 +2827,8 @@ func (kl *Kubelet) handlePodResourcesResize(pod *v1.Pod) (*v1.Pod, error) { allocatedPod = pod } if resizeStatus != "" { - // Save resize decision to checkpoint - if err := kl.statusManager.SetPodResizeStatus(allocatedPod.UID, resizeStatus); err != nil { - //TODO(vinaykul,InPlacePodVerticalScaling): Can we recover from this in some way? Investigate - klog.ErrorS(err, "SetPodResizeStatus failed", "pod", klog.KObj(allocatedPod)) + if err := kl.statusManager.SetPodResizeStatus(pod.UID, resizeStatus); err != nil { + klog.ErrorS(err, "Failed to set resize status", "pod", format.Pod(pod), "resizeStatus", resizeStatus) } } return allocatedPod, nil diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index faddc801ccb..b043c69316f 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -1757,15 +1757,6 @@ func (kl *Kubelet) determinePodResizeStatus(allocatedPod *v1.Pod, podStatus *kub } resizeStatus, _ := kl.statusManager.GetPodResizeStatus(string(allocatedPod.UID)) - // If the resize was in-progress and the actual resources match the allocated resources, mark - // the resize as complete by clearing the resize status. - if resizeStatus == v1.PodResizeStatusInProgress && - allocatedResourcesMatchStatus(allocatedPod, podStatus) { - if err := kl.statusManager.SetPodResizeStatus(allocatedPod.UID, ""); err != nil { - klog.ErrorS(err, "SetPodResizeStatus failed", "pod", format.Pod(allocatedPod)) - } - return "" - } return resizeStatus } diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index 70e05796b21..e2282c130f6 100644 --- a/pkg/kubelet/kubelet_test.go +++ b/pkg/kubelet/kubelet_test.go @@ -2665,11 +2665,12 @@ func TestHandlePodResourcesResize(t *testing.T) { defer kubelet.podManager.RemovePod(testPod1) tests := []struct { - name string - pod *v1.Pod - newRequests v1.ResourceList - expectedAllocations v1.ResourceList - expectedResize v1.PodResizeStatus + name string + pod *v1.Pod + newRequests v1.ResourceList + newRequestsAllocated bool // Whether the new requests have already been allocated (but not actuated) + expectedAllocations v1.ResourceList + expectedResize v1.PodResizeStatus }{ { name: "Request CPU and memory decrease - expect InProgress", @@ -2720,25 +2721,63 @@ func TestHandlePodResourcesResize(t *testing.T) { expectedAllocations: v1.ResourceList{v1.ResourceCPU: cpu1000m, v1.ResourceMemory: mem1000M}, expectedResize: v1.PodResizeStatusInfeasible, }, + { + name: "CPU increase in progress - expect InProgress", + pod: testPod2, + newRequests: v1.ResourceList{v1.ResourceCPU: cpu1500m, v1.ResourceMemory: mem1000M}, + newRequestsAllocated: true, + expectedAllocations: v1.ResourceList{v1.ResourceCPU: cpu1500m, v1.ResourceMemory: mem1000M}, + expectedResize: v1.PodResizeStatusInProgress, + }, + { + name: "No resize", + pod: testPod2, + newRequests: v1.ResourceList{v1.ResourceCPU: cpu1000m, v1.ResourceMemory: mem1000M}, + expectedAllocations: v1.ResourceList{v1.ResourceCPU: cpu1000m, v1.ResourceMemory: mem1000M}, + expectedResize: "", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { kubelet.statusManager = status.NewFakeManager() - require.NoError(t, kubelet.statusManager.SetPodAllocation(tt.pod)) - pod := tt.pod.DeepCopy() - pod.Spec.Containers[0].Resources.Requests = tt.newRequests - updatedPod, err := kubelet.handlePodResourcesResize(pod) + newPod := tt.pod.DeepCopy() + newPod.Spec.Containers[0].Resources.Requests = tt.newRequests + + if !tt.newRequestsAllocated { + require.NoError(t, kubelet.statusManager.SetPodAllocation(tt.pod)) + } else { + require.NoError(t, kubelet.statusManager.SetPodAllocation(newPod)) + } + + podStatus := &kubecontainer.PodStatus{ + ID: tt.pod.UID, + Name: tt.pod.Name, + Namespace: tt.pod.Namespace, + ContainerStatuses: make([]*kubecontainer.Status, len(tt.pod.Spec.Containers)), + } + for i, c := range tt.pod.Spec.Containers { + podStatus.ContainerStatuses[i] = &kubecontainer.Status{ + Name: c.Name, + State: kubecontainer.ContainerStateRunning, + Resources: &kubecontainer.ContainerResources{ + CPURequest: c.Resources.Requests.Cpu(), + CPULimit: c.Resources.Limits.Cpu(), + MemoryLimit: c.Resources.Limits.Memory(), + }, + } + } + + updatedPod, err := kubelet.handlePodResourcesResize(newPod, podStatus) require.NoError(t, err) assert.Equal(t, tt.expectedAllocations, updatedPod.Spec.Containers[0].Resources.Requests, "updated pod spec resources") - alloc, found := kubelet.statusManager.GetContainerResourceAllocation(string(pod.UID), pod.Spec.Containers[0].Name) + alloc, found := kubelet.statusManager.GetContainerResourceAllocation(string(newPod.UID), newPod.Spec.Containers[0].Name) require.True(t, found, "container allocation") assert.Equal(t, tt.expectedAllocations, alloc.Requests, "stored container allocation") - resizeStatus, found := kubelet.statusManager.GetPodResizeStatus(string(pod.UID)) - require.True(t, found, "pod resize status") + resizeStatus, _ := kubelet.statusManager.GetPodResizeStatus(string(newPod.UID)) assert.Equal(t, tt.expectedResize, resizeStatus) }) }