From 12435b26fcf1b4f85279b6d0efd02aa4bcb64899 Mon Sep 17 00:00:00 2001 From: vinay kulkarni Date: Sat, 4 Mar 2023 08:07:40 +0000 Subject: [PATCH 1/3] Fix nil pointer access panic in kubelet from uninitialized pod allocation checkpoint manager in standalone kubelet scenario --- pkg/kubelet/kubelet.go | 80 +++++++++++++++++----------- pkg/kubelet/kubelet_pods.go | 14 +++-- pkg/kubelet/status/status_manager.go | 34 ++++++++---- 3 files changed, 86 insertions(+), 42 deletions(-) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index fecdcc6a45b..4d476cfd4d7 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -2105,21 +2105,30 @@ func (kl *Kubelet) canAdmitPod(pods []*v1.Pod, pod *v1.Pod) (bool, string, strin // TODO: out of resource eviction should have a pod admitter call-out attrs := &lifecycle.PodAdmitAttributes{Pod: pod, OtherPods: pods} if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { + // If pod resource allocation checkpoint manager (checkpointState) is nil, it is likely because + // kubelet is bootstrapping kube-system pods on master node. In this scenario, pod resource resize + // is neither expected nor can be handled. Fall back to regular 'canAdmitPod' processing. + // TODO(vinaykul,InPlacePodVerticalScaling): Investigate if we can toss out all this checkpointing + // code and instead rely on ResourceAllocation / Resize values persisted in PodStatus. (Ref: KEP 2527) // Use allocated resources values from checkpoint store (source of truth) to determine fit - otherPods := make([]*v1.Pod, 0, len(pods)) checkpointState := kl.statusManager.State() - for _, p := range pods { - op := p.DeepCopy() - for _, c := range op.Spec.Containers { - resourcesAllocated, found := checkpointState.GetContainerResourceAllocation(string(p.UID), c.Name) - if c.Resources.Requests != nil && found { - c.Resources.Requests[v1.ResourceCPU] = resourcesAllocated[v1.ResourceCPU] - c.Resources.Requests[v1.ResourceMemory] = resourcesAllocated[v1.ResourceMemory] + if checkpointState != nil { + otherPods := make([]*v1.Pod, 0, len(pods)) + for _, p := range pods { + op := p.DeepCopy() + for _, c := range op.Spec.Containers { + resourcesAllocated, found := checkpointState.GetContainerResourceAllocation(string(p.UID), c.Name) + if c.Resources.Requests != nil && found { + c.Resources.Requests[v1.ResourceCPU] = resourcesAllocated[v1.ResourceCPU] + c.Resources.Requests[v1.ResourceMemory] = resourcesAllocated[v1.ResourceMemory] + } } + otherPods = append(otherPods, op) } - otherPods = append(otherPods, op) + attrs.OtherPods = otherPods + } else { + klog.ErrorS(nil, "pod resource allocation checkpoint manager is not initialized.") } - attrs.OtherPods = otherPods } for _, podAdmitHandler := range kl.admitHandlers { if result := podAdmitHandler.Admit(attrs); !result.Admit { @@ -2404,28 +2413,39 @@ func (kl *Kubelet) HandlePodAdditions(pods []*v1.Pod) { activePods := kl.filterOutInactivePods(existingPods) if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { - // To handle kubelet restarts, test pod admissibility using ResourcesAllocated values - // (for cpu & memory) from checkpoint store. If found, that is the source of truth. + // If pod resource allocation checkpoint manager (checkpointState) is nil, it is likely because + // kubelet is bootstrapping kube-system pods on master node. In this scenario, pod resource resize + // is neither expected nor can be handled. Fall back to regular 'canAdmitPod' processing. + // TODO(vinaykul,InPlacePodVerticalScaling): Investigate if we can toss out all this checkpointing + // code and instead rely on ResourceAllocation / Resize values persisted in PodStatus. (Ref: KEP 2527) checkpointState := kl.statusManager.State() - podCopy := pod.DeepCopy() - for _, c := range podCopy.Spec.Containers { - resourcesAllocated, found := checkpointState.GetContainerResourceAllocation(string(pod.UID), c.Name) - if c.Resources.Requests != nil && found { - c.Resources.Requests[v1.ResourceCPU] = resourcesAllocated[v1.ResourceCPU] - c.Resources.Requests[v1.ResourceMemory] = resourcesAllocated[v1.ResourceMemory] + if checkpointState != nil { + // To handle kubelet restarts, test pod admissibility using ResourcesAllocated values + // (for cpu & memory) from checkpoint store. If found, that is the source of truth. + podCopy := pod.DeepCopy() + for _, c := range podCopy.Spec.Containers { + resourcesAllocated, found := checkpointState.GetContainerResourceAllocation(string(pod.UID), c.Name) + if c.Resources.Requests != nil && found { + c.Resources.Requests[v1.ResourceCPU] = resourcesAllocated[v1.ResourceCPU] + c.Resources.Requests[v1.ResourceMemory] = resourcesAllocated[v1.ResourceMemory] + } + } + // Check if we can admit the pod; if not, reject it. + if ok, reason, message := kl.canAdmitPod(activePods, podCopy); !ok { + kl.rejectPod(pod, reason, message) + continue + } + // For new pod, checkpoint the resource values at which the Pod has been admitted + if err := kl.statusManager.SetPodAllocation(podCopy); err != nil { + //TODO(vinaykul,InPlacePodVerticalScaling): Can we recover from this in some way? Investigate + klog.ErrorS(err, "SetPodAllocation failed", "pod", klog.KObj(pod)) + } + } else { + // Check if we can admit the pod; if not, reject it. + if ok, reason, message := kl.canAdmitPod(activePods, pod); !ok { + kl.rejectPod(pod, reason, message) + continue } - } - - // Check if we can admit the pod; if not, reject it. - if ok, reason, message := kl.canAdmitPod(activePods, podCopy); !ok { - kl.rejectPod(pod, reason, message) - continue - } - - // For new pod, checkpoint the resource values at which the Pod has been admitted - if err := kl.statusManager.SetPodAllocation(podCopy); err != nil { - //TODO(vinaykul,InPlacePodVerticalScaling): Can we recover from this in some way? Investigate - klog.ErrorS(err, "SetPodAllocation failed", "pod", klog.KObj(pod)) } } else { // Check if we can admit the pod; if not, reject it. diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index 5ee3b9f4bec..af6b39c63d5 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -1473,8 +1473,12 @@ func (kl *Kubelet) determinePodResizeStatus(pod *v1.Pod, podStatus *v1.PodStatus } } else { checkpointState := kl.statusManager.State() - if resizeStatus, found := checkpointState.GetPodResizeStatus(string(pod.UID)); found { - podResizeStatus = resizeStatus + if checkpointState != nil { + if resizeStatus, found := checkpointState.GetPodResizeStatus(string(pod.UID)); found { + podResizeStatus = resizeStatus + } + } else { + klog.ErrorS(nil, "pod resource allocation checkpoint manager is not initialized.") } } return podResizeStatus @@ -1770,7 +1774,11 @@ func (kl *Kubelet) convertToAPIContainerStatuses(pod *v1.Pod, podStatus *kubecon // ResourcesAllocated values come from checkpoint. It is the source-of-truth. found := false checkpointState := kl.statusManager.State() - status.ResourcesAllocated, found = checkpointState.GetContainerResourceAllocation(string(pod.UID), cName) + if checkpointState != nil { + status.ResourcesAllocated, found = checkpointState.GetContainerResourceAllocation(string(pod.UID), cName) + } else { + klog.ErrorS(nil, "pod resource allocation checkpoint manager is not initialized.") + } if !(container.Resources.Requests == nil && container.Resources.Limits == nil) && !found { // Log error and fallback to ResourcesAllocated in oldStatus if it exists klog.ErrorS(nil, "resource allocation not found in checkpoint store", "pod", pod.Name, "container", cName) diff --git a/pkg/kubelet/status/status_manager.go b/pkg/kubelet/status/status_manager.go index fdf99b04074..f02919487c5 100644 --- a/pkg/kubelet/status/status_manager.go +++ b/pkg/kubelet/status/status_manager.go @@ -183,6 +183,17 @@ func isPodStatusByKubeletEqual(oldStatus, status *v1.PodStatus) bool { } func (m *manager) Start() { + // Create pod allocation checkpoint manager even if we don't have a client to allow local get/set of ResourcesAllocated & Resize + if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { + stateImpl, err := state.NewStateCheckpoint(m.stateFileDirectory, podStatusManagerStateFile) + if err != nil { + //TODO(vinaykul,InPlacePodVerticalScaling): Check if this error should be a critical (stop kubelet) failure + klog.ErrorS(err, "Could not initialize pod allocation checkpoint manager, please drain node and remove policy state file") + return + } + m.state = stateImpl + } + // Don't start the status manager if we don't have a client. This will happen // on the master, where the kubelet is responsible for bootstrapping the pods // of the master components. @@ -191,15 +202,6 @@ func (m *manager) Start() { return } - if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { - stateImpl, err := state.NewStateCheckpoint(m.stateFileDirectory, podStatusManagerStateFile) - if err != nil { - klog.ErrorS(err, "Could not initialize pod allocation checkpoint manager, please drain node and remove policy state file") - return - } - m.state = stateImpl - } - klog.InfoS("Starting to sync pod status with apiserver") //nolint:staticcheck // SA1015 Ticker can leak since this is only called once and doesn't handle termination. @@ -234,6 +236,9 @@ func (m *manager) State() state.Reader { // SetPodAllocation checkpoints the resources allocated to a pod's containers func (m *manager) SetPodAllocation(pod *v1.Pod) error { + if m.state == nil { + return fmt.Errorf("pod allocation checkpoint manager is not initialized") + } m.podStatusesLock.RLock() defer m.podStatusesLock.RUnlock() for _, container := range pod.Spec.Containers { @@ -250,6 +255,9 @@ func (m *manager) SetPodAllocation(pod *v1.Pod) error { // SetPodResizeStatus checkpoints the last resizing decision for the pod. func (m *manager) SetPodResizeStatus(podUID types.UID, resizeStatus v1.PodResizeStatus) error { + if m.state == nil { + return fmt.Errorf("pod allocation checkpoint manager is not initialized") + } m.podStatusesLock.RLock() defer m.podStatusesLock.RUnlock() return m.state.SetPodResizeStatus(string(podUID), resizeStatus) @@ -672,6 +680,10 @@ func (m *manager) deletePodStatus(uid types.UID) { delete(m.podStatuses, uid) m.podStartupLatencyHelper.DeletePodStartupState(uid) if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { + if m.state == nil { + klog.ErrorS(nil, "pod allocation checkpoint manager is not initialized") + return + } m.state.Delete(string(uid), "") } } @@ -685,6 +697,10 @@ func (m *manager) RemoveOrphanedStatuses(podUIDs map[types.UID]bool) { klog.V(5).InfoS("Removing pod from status map.", "podUID", key) delete(m.podStatuses, key) if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { + if m.state == nil { + klog.ErrorS(nil, "pod allocation checkpoint manager is not initialized") + continue + } m.state.Delete(string(key), "") } } From b0dce923f15e23cce65d19222258aaa112a698bc Mon Sep 17 00:00:00 2001 From: vinay kulkarni Date: Mon, 6 Mar 2023 06:34:53 +0000 Subject: [PATCH 2/3] Add Get interfaces for container's checkpointed ResourcesAllocated and Resize values, remove error logging for valid standalone kubelet scenario --- pkg/kubelet/kubelet.go | 80 +++++++------------ pkg/kubelet/kubelet_pods.go | 16 +--- pkg/kubelet/status/fake_status_manager.go | 10 +++ pkg/kubelet/status/status_manager.go | 40 ++++++++-- .../testing/mock_pod_status_provider.go | 30 +++++++ 5 files changed, 103 insertions(+), 73 deletions(-) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 4d476cfd4d7..f6d6d2c37e7 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -2105,30 +2105,20 @@ func (kl *Kubelet) canAdmitPod(pods []*v1.Pod, pod *v1.Pod) (bool, string, strin // TODO: out of resource eviction should have a pod admitter call-out attrs := &lifecycle.PodAdmitAttributes{Pod: pod, OtherPods: pods} if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { - // If pod resource allocation checkpoint manager (checkpointState) is nil, it is likely because - // kubelet is bootstrapping kube-system pods on master node. In this scenario, pod resource resize - // is neither expected nor can be handled. Fall back to regular 'canAdmitPod' processing. - // TODO(vinaykul,InPlacePodVerticalScaling): Investigate if we can toss out all this checkpointing - // code and instead rely on ResourceAllocation / Resize values persisted in PodStatus. (Ref: KEP 2527) // Use allocated resources values from checkpoint store (source of truth) to determine fit - checkpointState := kl.statusManager.State() - if checkpointState != nil { - otherPods := make([]*v1.Pod, 0, len(pods)) - for _, p := range pods { - op := p.DeepCopy() - for _, c := range op.Spec.Containers { - resourcesAllocated, found := checkpointState.GetContainerResourceAllocation(string(p.UID), c.Name) - if c.Resources.Requests != nil && found { - c.Resources.Requests[v1.ResourceCPU] = resourcesAllocated[v1.ResourceCPU] - c.Resources.Requests[v1.ResourceMemory] = resourcesAllocated[v1.ResourceMemory] - } + otherPods := make([]*v1.Pod, 0, len(pods)) + for _, p := range pods { + op := p.DeepCopy() + for _, c := range op.Spec.Containers { + resourcesAllocated, found := kl.statusManager.GetContainerResourceAllocation(string(p.UID), c.Name) + if c.Resources.Requests != nil && found { + c.Resources.Requests[v1.ResourceCPU] = resourcesAllocated[v1.ResourceCPU] + c.Resources.Requests[v1.ResourceMemory] = resourcesAllocated[v1.ResourceMemory] } - otherPods = append(otherPods, op) } - attrs.OtherPods = otherPods - } else { - klog.ErrorS(nil, "pod resource allocation checkpoint manager is not initialized.") + otherPods = append(otherPods, op) } + attrs.OtherPods = otherPods } for _, podAdmitHandler := range kl.admitHandlers { if result := podAdmitHandler.Admit(attrs); !result.Admit { @@ -2413,40 +2403,26 @@ func (kl *Kubelet) HandlePodAdditions(pods []*v1.Pod) { activePods := kl.filterOutInactivePods(existingPods) if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { - // If pod resource allocation checkpoint manager (checkpointState) is nil, it is likely because - // kubelet is bootstrapping kube-system pods on master node. In this scenario, pod resource resize - // is neither expected nor can be handled. Fall back to regular 'canAdmitPod' processing. - // TODO(vinaykul,InPlacePodVerticalScaling): Investigate if we can toss out all this checkpointing - // code and instead rely on ResourceAllocation / Resize values persisted in PodStatus. (Ref: KEP 2527) - checkpointState := kl.statusManager.State() - if checkpointState != nil { - // To handle kubelet restarts, test pod admissibility using ResourcesAllocated values - // (for cpu & memory) from checkpoint store. If found, that is the source of truth. - podCopy := pod.DeepCopy() - for _, c := range podCopy.Spec.Containers { - resourcesAllocated, found := checkpointState.GetContainerResourceAllocation(string(pod.UID), c.Name) - if c.Resources.Requests != nil && found { - c.Resources.Requests[v1.ResourceCPU] = resourcesAllocated[v1.ResourceCPU] - c.Resources.Requests[v1.ResourceMemory] = resourcesAllocated[v1.ResourceMemory] - } - } - // Check if we can admit the pod; if not, reject it. - if ok, reason, message := kl.canAdmitPod(activePods, podCopy); !ok { - kl.rejectPod(pod, reason, message) - continue - } - // For new pod, checkpoint the resource values at which the Pod has been admitted - if err := kl.statusManager.SetPodAllocation(podCopy); err != nil { - //TODO(vinaykul,InPlacePodVerticalScaling): Can we recover from this in some way? Investigate - klog.ErrorS(err, "SetPodAllocation failed", "pod", klog.KObj(pod)) - } - } else { - // Check if we can admit the pod; if not, reject it. - if ok, reason, message := kl.canAdmitPod(activePods, pod); !ok { - kl.rejectPod(pod, reason, message) - continue + // To handle kubelet restarts, test pod admissibility using ResourcesAllocated values + // (for cpu & memory) from checkpoint store. If found, that is the source of truth. + podCopy := pod.DeepCopy() + for _, c := range podCopy.Spec.Containers { + resourcesAllocated, found := kl.statusManager.GetContainerResourceAllocation(string(pod.UID), c.Name) + if c.Resources.Requests != nil && found { + c.Resources.Requests[v1.ResourceCPU] = resourcesAllocated[v1.ResourceCPU] + c.Resources.Requests[v1.ResourceMemory] = resourcesAllocated[v1.ResourceMemory] } } + // Check if we can admit the pod; if not, reject it. + if ok, reason, message := kl.canAdmitPod(activePods, podCopy); !ok { + kl.rejectPod(pod, reason, message) + continue + } + // For new pod, checkpoint the resource values at which the Pod has been admitted + if err := kl.statusManager.SetPodAllocation(podCopy); err != nil { + //TODO(vinaykul,InPlacePodVerticalScaling): Can we recover from this in some way? Investigate + klog.ErrorS(err, "SetPodAllocation failed", "pod", klog.KObj(pod)) + } } else { // Check if we can admit the pod; if not, reject it. if ok, reason, message := kl.canAdmitPod(activePods, pod); !ok { diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index af6b39c63d5..335af666f5c 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -1472,13 +1472,8 @@ func (kl *Kubelet) determinePodResizeStatus(pod *v1.Pod, podStatus *v1.PodStatus klog.ErrorS(err, "SetPodResizeStatus failed", "pod", pod.Name) } } else { - checkpointState := kl.statusManager.State() - if checkpointState != nil { - if resizeStatus, found := checkpointState.GetPodResizeStatus(string(pod.UID)); found { - podResizeStatus = resizeStatus - } - } else { - klog.ErrorS(nil, "pod resource allocation checkpoint manager is not initialized.") + if resizeStatus, found := kl.statusManager.GetPodResizeStatus(string(pod.UID)); found { + podResizeStatus = resizeStatus } } return podResizeStatus @@ -1773,12 +1768,7 @@ func (kl *Kubelet) convertToAPIContainerStatuses(pod *v1.Pod, podStatus *kubecon container := kubecontainer.GetContainerSpec(pod, cName) // ResourcesAllocated values come from checkpoint. It is the source-of-truth. found := false - checkpointState := kl.statusManager.State() - if checkpointState != nil { - status.ResourcesAllocated, found = checkpointState.GetContainerResourceAllocation(string(pod.UID), cName) - } else { - klog.ErrorS(nil, "pod resource allocation checkpoint manager is not initialized.") - } + status.ResourcesAllocated, found = kl.statusManager.GetContainerResourceAllocation(string(pod.UID), cName) if !(container.Resources.Requests == nil && container.Resources.Limits == nil) && !found { // Log error and fallback to ResourcesAllocated in oldStatus if it exists klog.ErrorS(nil, "resource allocation not found in checkpoint store", "pod", pod.Name, "container", cName) diff --git a/pkg/kubelet/status/fake_status_manager.go b/pkg/kubelet/status/fake_status_manager.go index ee4b3f5f36f..18b61bf7c7a 100644 --- a/pkg/kubelet/status/fake_status_manager.go +++ b/pkg/kubelet/status/fake_status_manager.go @@ -68,6 +68,16 @@ func (m *fakeManager) State() state.Reader { return m.state } +func (m *fakeManager) GetContainerResourceAllocation(podUID string, containerName string) (v1.ResourceList, bool) { + klog.InfoS("GetContainerResourceAllocation()") + return m.state.GetContainerResourceAllocation(podUID, containerName) +} + +func (m *fakeManager) GetPodResizeStatus(podUID string) (v1.PodResizeStatus, bool) { + klog.InfoS("GetPodResizeStatus()") + return "", false +} + func (m *fakeManager) SetPodAllocation(pod *v1.Pod) error { klog.InfoS("SetPodAllocation()") for _, container := range pod.Spec.Containers { diff --git a/pkg/kubelet/status/status_manager.go b/pkg/kubelet/status/status_manager.go index f02919487c5..e35ad681e1e 100644 --- a/pkg/kubelet/status/status_manager.go +++ b/pkg/kubelet/status/status_manager.go @@ -140,6 +140,12 @@ type Manager interface { // State returns a read-only interface to the internal status manager state. State() state.Reader + // GetContainerResourceAllocation returns checkpointed ResourcesAllocated value for the container + GetContainerResourceAllocation(podUID string, containerName string) (v1.ResourceList, bool) + + // GetPodResizeStatus returns checkpointed PodStatus.Resize value + GetPodResizeStatus(podUID string) (v1.PodResizeStatus, bool) + // SetPodAllocation checkpoints the resources allocated to a pod's containers. SetPodAllocation(pod *v1.Pod) error @@ -234,6 +240,28 @@ func (m *manager) State() state.Reader { return m.state } +// GetContainerResourceAllocation returns the last checkpointed ResourcesAllocated values +// If checkpoint manager has not been initialized, it returns nil, false +func (m *manager) GetContainerResourceAllocation(podUID string, containerName string) (v1.ResourceList, bool) { + m.podStatusesLock.RLock() + defer m.podStatusesLock.RUnlock() + if m.state != nil { + return m.state.GetContainerResourceAllocation(podUID, containerName) + } + return nil, false +} + +// GetPodResizeStatus returns the last checkpointed ResizeStaus value +// If checkpoint manager has not been initialized, it returns nil, false +func (m *manager) GetPodResizeStatus(podUID string) (v1.PodResizeStatus, bool) { + m.podStatusesLock.RLock() + defer m.podStatusesLock.RUnlock() + if m.state != nil { + return m.state.GetPodResizeStatus(podUID) + } + return "", false +} + // SetPodAllocation checkpoints the resources allocated to a pod's containers func (m *manager) SetPodAllocation(pod *v1.Pod) error { if m.state == nil { @@ -680,11 +708,9 @@ func (m *manager) deletePodStatus(uid types.UID) { delete(m.podStatuses, uid) m.podStartupLatencyHelper.DeletePodStartupState(uid) if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { - if m.state == nil { - klog.ErrorS(nil, "pod allocation checkpoint manager is not initialized") - return + if m.state != nil { + m.state.Delete(string(uid), "") } - m.state.Delete(string(uid), "") } } @@ -697,11 +723,9 @@ func (m *manager) RemoveOrphanedStatuses(podUIDs map[types.UID]bool) { klog.V(5).InfoS("Removing pod from status map.", "podUID", key) delete(m.podStatuses, key) if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { - if m.state == nil { - klog.ErrorS(nil, "pod allocation checkpoint manager is not initialized") - continue + if m.state != nil { + m.state.Delete(string(key), "") } - m.state.Delete(string(key), "") } } } diff --git a/pkg/kubelet/status/testing/mock_pod_status_provider.go b/pkg/kubelet/status/testing/mock_pod_status_provider.go index 3112a4ab625..457c79d9ba1 100644 --- a/pkg/kubelet/status/testing/mock_pod_status_provider.go +++ b/pkg/kubelet/status/testing/mock_pod_status_provider.go @@ -189,6 +189,36 @@ func (m *MockManager) EXPECT() *MockManagerMockRecorder { return m.recorder } +// GetContainerResourceAllocation mocks base method. +func (m *MockManager) GetContainerResourceAllocation(podUID, containerName string) (v1.ResourceList, bool) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetContainerResourceAllocation", podUID, containerName) + ret0, _ := ret[0].(v1.ResourceList) + ret1, _ := ret[1].(bool) + return ret0, ret1 +} + +// GetContainerResourceAllocation indicates an expected call of GetContainerResourceAllocation. +func (mr *MockManagerMockRecorder) GetContainerResourceAllocation(podUID, containerName interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetContainerResourceAllocation", reflect.TypeOf((*MockManager)(nil).GetContainerResourceAllocation), podUID, containerName) +} + +// GetPodResizeStatus mocks base method. +func (m *MockManager) GetPodResizeStatus(podUID string) (v1.PodResizeStatus, bool) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetPodResizeStatus", podUID) + ret0, _ := ret[0].(v1.PodResizeStatus) + ret1, _ := ret[1].(bool) + return ret0, ret1 +} + +// GetPodResizeStatus indicates an expected call of GetPodResizeStatus. +func (mr *MockManagerMockRecorder) GetPodResizeStatus(podUID interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPodResizeStatus", reflect.TypeOf((*MockManager)(nil).GetPodResizeStatus), podUID) +} + // GetPodStatus mocks base method. func (m *MockManager) GetPodStatus(uid types.UID) (v1.PodStatus, bool) { m.ctrl.T.Helper() From 98e8f42f330f4708009df04c270ce3799baec5c5 Mon Sep 17 00:00:00 2001 From: vinay kulkarni Date: Tue, 7 Mar 2023 05:59:34 +0000 Subject: [PATCH 3/3] panic on pod resources alloc checkpoint failure --- pkg/kubelet/status/fake_status_manager.go | 5 --- pkg/kubelet/status/status_manager.go | 38 ++++--------------- .../testing/mock_pod_status_provider.go | 15 -------- 3 files changed, 7 insertions(+), 51 deletions(-) diff --git a/pkg/kubelet/status/fake_status_manager.go b/pkg/kubelet/status/fake_status_manager.go index 18b61bf7c7a..42cd611984e 100644 --- a/pkg/kubelet/status/fake_status_manager.go +++ b/pkg/kubelet/status/fake_status_manager.go @@ -63,11 +63,6 @@ func (m *fakeManager) RemoveOrphanedStatuses(podUIDs map[types.UID]bool) { return } -func (m *fakeManager) State() state.Reader { - klog.InfoS("State()") - return m.state -} - func (m *fakeManager) GetContainerResourceAllocation(podUID string, containerName string) (v1.ResourceList, bool) { klog.InfoS("GetContainerResourceAllocation()") return m.state.GetContainerResourceAllocation(podUID, containerName) diff --git a/pkg/kubelet/status/status_manager.go b/pkg/kubelet/status/status_manager.go index e35ad681e1e..7ee44059cb4 100644 --- a/pkg/kubelet/status/status_manager.go +++ b/pkg/kubelet/status/status_manager.go @@ -137,9 +137,6 @@ type Manager interface { // the provided podUIDs. RemoveOrphanedStatuses(podUIDs map[types.UID]bool) - // State returns a read-only interface to the internal status manager state. - State() state.Reader - // GetContainerResourceAllocation returns checkpointed ResourcesAllocated value for the container GetContainerResourceAllocation(podUID string, containerName string) (v1.ResourceList, bool) @@ -189,13 +186,13 @@ func isPodStatusByKubeletEqual(oldStatus, status *v1.PodStatus) bool { } func (m *manager) Start() { - // Create pod allocation checkpoint manager even if we don't have a client to allow local get/set of ResourcesAllocated & Resize + // Create pod allocation checkpoint manager even if client is nil so as to allow local get/set of ResourcesAllocated & Resize if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { stateImpl, err := state.NewStateCheckpoint(m.stateFileDirectory, podStatusManagerStateFile) if err != nil { - //TODO(vinaykul,InPlacePodVerticalScaling): Check if this error should be a critical (stop kubelet) failure + // This is a crictical, non-recoverable failure. klog.ErrorS(err, "Could not initialize pod allocation checkpoint manager, please drain node and remove policy state file") - return + panic(err) } m.state = stateImpl } @@ -235,20 +232,12 @@ func (m *manager) Start() { }, 0) } -// State returns the pod resources checkpoint state of the pod status manager -func (m *manager) State() state.Reader { - return m.state -} - // GetContainerResourceAllocation returns the last checkpointed ResourcesAllocated values // If checkpoint manager has not been initialized, it returns nil, false func (m *manager) GetContainerResourceAllocation(podUID string, containerName string) (v1.ResourceList, bool) { m.podStatusesLock.RLock() defer m.podStatusesLock.RUnlock() - if m.state != nil { - return m.state.GetContainerResourceAllocation(podUID, containerName) - } - return nil, false + return m.state.GetContainerResourceAllocation(podUID, containerName) } // GetPodResizeStatus returns the last checkpointed ResizeStaus value @@ -256,17 +245,11 @@ func (m *manager) GetContainerResourceAllocation(podUID string, containerName st func (m *manager) GetPodResizeStatus(podUID string) (v1.PodResizeStatus, bool) { m.podStatusesLock.RLock() defer m.podStatusesLock.RUnlock() - if m.state != nil { - return m.state.GetPodResizeStatus(podUID) - } - return "", false + return m.state.GetPodResizeStatus(podUID) } // SetPodAllocation checkpoints the resources allocated to a pod's containers func (m *manager) SetPodAllocation(pod *v1.Pod) error { - if m.state == nil { - return fmt.Errorf("pod allocation checkpoint manager is not initialized") - } m.podStatusesLock.RLock() defer m.podStatusesLock.RUnlock() for _, container := range pod.Spec.Containers { @@ -283,9 +266,6 @@ func (m *manager) SetPodAllocation(pod *v1.Pod) error { // SetPodResizeStatus checkpoints the last resizing decision for the pod. func (m *manager) SetPodResizeStatus(podUID types.UID, resizeStatus v1.PodResizeStatus) error { - if m.state == nil { - return fmt.Errorf("pod allocation checkpoint manager is not initialized") - } m.podStatusesLock.RLock() defer m.podStatusesLock.RUnlock() return m.state.SetPodResizeStatus(string(podUID), resizeStatus) @@ -708,9 +688,7 @@ func (m *manager) deletePodStatus(uid types.UID) { delete(m.podStatuses, uid) m.podStartupLatencyHelper.DeletePodStartupState(uid) if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { - if m.state != nil { - m.state.Delete(string(uid), "") - } + m.state.Delete(string(uid), "") } } @@ -723,9 +701,7 @@ func (m *manager) RemoveOrphanedStatuses(podUIDs map[types.UID]bool) { klog.V(5).InfoS("Removing pod from status map.", "podUID", key) delete(m.podStatuses, key) if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { - if m.state != nil { - m.state.Delete(string(key), "") - } + m.state.Delete(string(key), "") } } } diff --git a/pkg/kubelet/status/testing/mock_pod_status_provider.go b/pkg/kubelet/status/testing/mock_pod_status_provider.go index 457c79d9ba1..2de1bee1691 100644 --- a/pkg/kubelet/status/testing/mock_pod_status_provider.go +++ b/pkg/kubelet/status/testing/mock_pod_status_provider.go @@ -27,7 +27,6 @@ import ( v1 "k8s.io/api/core/v1" types "k8s.io/apimachinery/pkg/types" container "k8s.io/kubernetes/pkg/kubelet/container" - state "k8s.io/kubernetes/pkg/kubelet/status/state" ) // MockPodStatusProvider is a mock of PodStatusProvider interface. @@ -322,20 +321,6 @@ func (mr *MockManagerMockRecorder) Start() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Start", reflect.TypeOf((*MockManager)(nil).Start)) } -// State mocks base method. -func (m *MockManager) State() state.Reader { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "State") - ret0, _ := ret[0].(state.Reader) - return ret0 -} - -// State indicates an expected call of State. -func (mr *MockManagerMockRecorder) State() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "State", reflect.TypeOf((*MockManager)(nil).State)) -} - // TerminatePod mocks base method. func (m *MockManager) TerminatePod(pod *v1.Pod) { m.ctrl.T.Helper()