From 18c833f93bad74a6f76f572735fbf25e36d1750a Mon Sep 17 00:00:00 2001 From: Jan Chaloupka Date: Tue, 19 Nov 2024 14:58:58 +0100 Subject: [PATCH] panic when pod uid is not set instead of returning an error --- pkg/descheduler/evictions/evictions.go | 92 ++++++++------------------ 1 file changed, 28 insertions(+), 64 deletions(-) diff --git a/pkg/descheduler/evictions/evictions.go b/pkg/descheduler/evictions/evictions.go index edb02ea823..e833d8d1fa 100644 --- a/pkg/descheduler/evictions/evictions.go +++ b/pkg/descheduler/evictions/evictions.go @@ -18,7 +18,6 @@ package evictions import ( "context" - "errors" "fmt" "strings" "sync" @@ -120,40 +119,36 @@ func (erc *evictionRequestsCache) TotalEvictionRequests() uint { } // getPodKey returns the string key of a pod. -func getPodKey(pod *v1.Pod) (string, error) { +func getPodKey(pod *v1.Pod) string { uid := string(pod.UID) + // Every pod is expected to have the UID set. + // When the descheduling framework is used for simulation + // user created workload may forget to set the UID. if len(uid) == 0 { - return "", errors.New("cannot get cache key for pod with empty UID") + panic(fmt.Errorf("cannot get cache key for %v/%v pod with empty UID", pod.Namespace, pod.Name)) } - return uid, nil + return uid } -func (erc *evictionRequestsCache) addPod(pod *v1.Pod) error { +func (erc *evictionRequestsCache) addPod(pod *v1.Pod) { erc.mu.Lock() defer erc.mu.Unlock() - uid, err := getPodKey(pod) - if err != nil { - return fmt.Errorf("unable to get pod key: %v", err) - } + uid := getPodKey(pod) if _, exists := erc.requests[uid]; exists { - return nil + return } erc.requests[uid] = evictionRequestItem{podNamespace: pod.Namespace, podName: pod.Name, podNodeName: pod.Spec.NodeName} erc.requestsPerNode[pod.Spec.NodeName]++ erc.requestsPerNamespace[pod.Namespace]++ erc.requestsTotal++ - return nil } -func (erc *evictionRequestsCache) assumePod(pod *v1.Pod) error { +func (erc *evictionRequestsCache) assumePod(pod *v1.Pod) { erc.mu.Lock() defer erc.mu.Unlock() - uid, err := getPodKey(pod) - if err != nil { - return fmt.Errorf("unable to get pod key: %v", err) - } + uid := getPodKey(pod) if _, exists := erc.requests[uid]; exists { - return nil + return } erc.requests[uid] = evictionRequestItem{ podNamespace: pod.Namespace, @@ -165,7 +160,7 @@ func (erc *evictionRequestsCache) assumePod(pod *v1.Pod) error { erc.requestsPerNode[pod.Spec.NodeName]++ erc.requestsPerNamespace[pod.Namespace]++ erc.requestsTotal++ - return nil + return } // no locking, expected to be invoked from protected methods only @@ -182,28 +177,21 @@ func (erc *evictionRequestsCache) deleteItem(uid string) { delete(erc.requests, uid) } -func (erc *evictionRequestsCache) deletePod(pod *v1.Pod) error { +func (erc *evictionRequestsCache) deletePod(pod *v1.Pod) { erc.mu.Lock() defer erc.mu.Unlock() - uid, err := getPodKey(pod) - if err != nil { - return fmt.Errorf("unable to get pod key: %v", err) - } + uid := getPodKey(pod) if _, exists := erc.requests[uid]; exists { erc.deleteItem(uid) } - return nil } -func (erc *evictionRequestsCache) hasPod(pod *v1.Pod) (bool, error) { +func (erc *evictionRequestsCache) hasPod(pod *v1.Pod) bool { erc.mu.RLock() defer erc.mu.RUnlock() - uid, err := getPodKey(pod) - if err != nil { - return false, fmt.Errorf("unable to get pod key: %v", err) - } + uid := getPodKey(pod) _, exists := erc.requests[uid] - return exists, nil + return exists } var ( @@ -280,9 +268,7 @@ func NewPodEvictor( // Ignore completed/suceeeded or failed pods if pod.Status.Phase != v1.PodSucceeded && pod.Status.Phase != v1.PodFailed { klog.V(3).InfoS("Eviction in background detected. Adding pod to the cache.", "pod", klog.KObj(pod)) - if err := erCache.addPod(pod); err != nil { - klog.ErrorS(err, "Unable to add pod to cache", "pod", pod) - } + erCache.addPod(pod) } } } @@ -300,22 +286,16 @@ func NewPodEvictor( } // Ignore pod's that are not subject to an eviction in background if _, exists := newPod.Annotations[EvictionRequestAnnotationKey]; !exists { - if has, err := erCache.hasPod(newPod); err == nil && has { + if erCache.hasPod(newPod) { klog.V(3).InfoS("Pod with eviction in background lost annotation. Removing pod from the cache.", "pod", klog.KObj(newPod)) } - if err := erCache.deletePod(newPod); err != nil { - // If the deletion fails the cache may block eviction - klog.ErrorS(err, "Unable to delete updated pod from cache", "pod", newPod) - } + erCache.deletePod(newPod) return } // Remove completed/suceeeded or failed pods from the cache if newPod.Status.Phase == v1.PodSucceeded || newPod.Status.Phase == v1.PodFailed { klog.V(3).InfoS("Pod with eviction in background completed. Removing pod from the cache.", "pod", klog.KObj(newPod)) - if err := erCache.deletePod(newPod); err != nil { - // If the deletion fails the cache may block eviction - klog.ErrorS(err, "Unable to delete updated pod from cache", "pod", newPod) - } + erCache.deletePod(newPod) return } // Ignore any pod that does not have eviction in progress @@ -333,18 +313,11 @@ func NewPodEvictor( // got terminated with no-retry, requesting a new eviction is a normal // operation. klog.V(3).InfoS("Eviction in background canceled (annotation removed). Removing pod from the cache.", "annotation", EvictionInProgressAnnotationKey, "pod", klog.KObj(newPod)) - if err := erCache.deletePod(newPod); err != nil { - // If the deletion fails the cache may block eviction - klog.ErrorS(err, "Unable to delete updated pod from cache", "pod", newPod) - return - } + erCache.deletePod(newPod) return } // Pick up the eviction in progress - if err := erCache.addPod(newPod); err != nil { - klog.ErrorS(err, "Unable to add pod to cache", "pod", newPod) - return - } + erCache.addPod(newPod) }, DeleteFunc: func(obj interface{}) { var pod *v1.Pod @@ -362,13 +335,10 @@ func NewPodEvictor( klog.ErrorS(nil, "Cannot convert to *v1.Pod", "obj", t) return } - if has, err := erCache.hasPod(pod); err == nil && has { + if erCache.hasPod(pod) { klog.V(3).InfoS("Pod with eviction in background deleted/evicted. Removing pod from the cache.", "pod", klog.KObj(pod)) } - if err := erCache.deletePod(pod); err != nil { - klog.ErrorS(err, "Unable to delete pod from cache", "pod", pod) - return - } + erCache.deletePod(pod) }, }, ) @@ -488,11 +458,7 @@ func (pe *PodEvictor) EvictPod(ctx context.Context, pod *v1.Pod, opts EvictOptio if pe.featureGates.Enabled(features.EvictionsInBackground) { // eviction in background requested if _, exists := pod.Annotations[EvictionRequestAnnotationKey]; exists { - exists, err := pe.erCache.hasPod(pod) - if err != nil { - return fmt.Errorf("unable to check whether a pod exists in the cache of eviction requests: %v", err) - } - if exists { + if pe.erCache.hasPod(pod) { klog.V(3).InfoS("Eviction in background already requested (ignoring)", "pod", klog.KObj(pod)) return nil } @@ -612,9 +578,7 @@ func (pe *PodEvictor) evictPod(ctx context.Context, pod *v1.Pod) (bool, error) { klog.V(3).InfoS("Ignoring eviction of a completed/failed pod", "pod", klog.KObj(pod)) return true, nil } - if err := pe.erCache.assumePod(pod); err != nil { - klog.ErrorS(err, "eviction request: unable to assume pod to cache", "pod", pod) - } + pe.erCache.assumePod(pod) return true, nil } }