panic when pod uid is not set instead of returning an error

kubernetes-sigs · Nov 19, 2024 · cfbb740 · cfbb740
1 parent 4425072
commit cfbb740
Showing 1 changed file with 27 additions and 64 deletions.
diff --git 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,6 @@ func (erc *evictionRequestsCache) assumePod(pod *v1.Pod) error {
 	erc.requestsPerNode[pod.Spec.NodeName]++
 	erc.requestsPerNamespace[pod.Namespace]++
 	erc.requestsTotal++
-	return nil
 }
 
 // no locking, expected to be invoked from protected methods only
@@ -182,28 +176,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 +267,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 +285,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 +312,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 +334,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 +457,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 +577,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
 			}
 		}