standardize protectionEnabled param

pkg/framework/plugins/defaultevictor/constraints.go

@@ -14,7 +14,6 @@ limitations under the License.
 package defaultevictor
 
 import (
-	"context"
 	"fmt"
 	"time"
 
@@ -23,109 +22,9 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/klog/v2"
 
-	"sigs.k8s.io/descheduler/pkg/api"
-	podutil "sigs.k8s.io/descheduler/pkg/descheduler/pod"
 	frameworktypes "sigs.k8s.io/descheduler/pkg/framework/types"
-	"sigs.k8s.io/descheduler/pkg/utils"
 )
 
-func evictionConstraintsForFailedBarePods(logger klog.Logger, evictFailedBarePods bool) []constraint {
-	if evictFailedBarePods {
-		logger.V(1).Info("Warning: EvictFailedBarePods is set to True. This could cause eviction of pods without ownerReferences.")
-		return []constraint{
-			func(pod *v1.Pod) error {
-				ownerRefList := podutil.OwnerRef(pod)
-				if len(ownerRefList) == 0 && pod.Status.Phase != v1.PodFailed {
-					return fmt.Errorf("pod does not have any ownerRefs and is not in failed phase")
-				}
-				return nil
-			},
-		}
-	}
-	return []constraint{
-		func(pod *v1.Pod) error {
-			if len(podutil.OwnerRef(pod)) == 0 {
-				return fmt.Errorf("pod does not have any ownerRefs")
-			}
-			return nil
-		},
-	}
-}
-
-func evictionConstraintsForSystemCriticalPods(logger klog.Logger, evictSystemCriticalPods bool, priorityThreshold *api.PriorityThreshold, handle frameworktypes.Handle) ([]constraint, error) {
-	var constraints []constraint
-
-	if !evictSystemCriticalPods {
-		constraints = append(constraints, func(pod *v1.Pod) error {
-			if utils.IsCriticalPriorityPod(pod) {
-				return fmt.Errorf("pod has system critical priority")
-			}
-			return nil
-		})
-
-		if priorityThreshold != nil && (priorityThreshold.Value != nil || len(priorityThreshold.Name) > 0) {
-			thresholdPriority, err := utils.GetPriorityValueFromPriorityThreshold(context.TODO(), handle.ClientSet(), priorityThreshold)
-			if err != nil {
-				logger.Error(err, "failed to get priority threshold")
-				return nil, err
-			}
-			constraints = append(constraints, func(pod *v1.Pod) error {
-				if !IsPodEvictableBasedOnPriority(pod, thresholdPriority) {
-					return fmt.Errorf("pod has higher priority than specified priority class threshold")
-				}
-				return nil
-			})
-		}
-	} else {
-		logger.V(1).Info("Warning: EvictSystemCriticalPods is set to True. This could cause eviction of Kubernetes system pods.")
-	}
-
-	return constraints, nil
-}
-
-func evictionConstraintsForLocalStoragePods(evictLocalStoragePods bool) []constraint {
-	if !evictLocalStoragePods {
-		return []constraint{
-			func(pod *v1.Pod) error {
-				if utils.IsPodWithLocalStorage(pod) {
-					return fmt.Errorf("pod has local storage and descheduler is not configured with evictLocalStoragePods")
-				}
-				return nil
-			},
-		}
-	}
-	return nil
-}
-
-func evictionConstraintsForDaemonSetPods(evictDaemonSetPods bool) []constraint {
-	if !evictDaemonSetPods {
-		return []constraint{
-			func(pod *v1.Pod) error {
-				ownerRefList := podutil.OwnerRef(pod)
-				if utils.IsDaemonsetPod(ownerRefList) {
-					return fmt.Errorf("pod is related to daemonset and descheduler is not configured with evictDaemonSetPods")
-				}
-				return nil
-			},
-		}
-	}
-	return nil
-}
-
-func evictionConstraintsForPvcPods(ignorePvcPods bool) []constraint {
-	if ignorePvcPods {
-		return []constraint{
-			func(pod *v1.Pod) error {
-				if utils.IsPodWithPVC(pod) {
-					return fmt.Errorf("pod has a PVC and descheduler is configured to ignore PVC pods")
-				}
-				return nil
-			},
-		}
-	}
-	return nil
-}
-
 func evictionConstraintsForLabelSelector(logger klog.Logger, labelSelector *metav1.LabelSelector) ([]constraint, error) {
 	if labelSelector != nil {
 		selector, err := metav1.LabelSelectorAsSelector(labelSelector)
@@ -193,35 +92,3 @@ func evictionConstraintsForMinPodAge(minPodAge *metav1.Duration) []constraint {
 	}
 	return nil
 }
-
-func evictionConstraintsForIgnorePodsWithoutPDB(ignorePodsWithoutPDB bool, handle frameworktypes.Handle) []constraint {
-	if ignorePodsWithoutPDB {
-		return []constraint{
-			func(pod *v1.Pod) error {
-				hasPdb, err := utils.IsPodCoveredByPDB(pod, handle.SharedInformerFactory().Policy().V1().PodDisruptionBudgets().Lister())
-				if err != nil {
-					return fmt.Errorf("unable to check if pod is covered by PodDisruptionBudget: %w", err)
-				}
-				if !hasPdb {
-					return fmt.Errorf("no PodDisruptionBudget found for pod")
-				}
-				return nil
-			},
-		}
-	}
-	return nil
-}
-
-func evictionConstraintsForResourceClaimsPods(protectionEnabled bool) []constraint {
-	if protectionEnabled {
-		return []constraint{
-			func(pod *v1.Pod) error {
-				if utils.IsPodWithResourceClaims(pod) {
-					return fmt.Errorf("pod has ResourceClaims and descheduler is configured to protect ResourceClaims pods")
-				}
-				return nil
-			},
-		}
-	}
-	return nil
-}

pkg/framework/plugins/defaultevictor/defaultevictor.go

@@ -131,47 +131,119 @@ func applyEffectivePodProtections(d *DefaultEvictor, podProtections []PodProtect
 
 func applyFailedBarePodsProtection(d *DefaultEvictor, protectionMap map[PodProtection]bool) {
 	isProtectionEnabled := protectionMap[FailedBarePods]
-	d.constraints = append(d.constraints, evictionConstraintsForFailedBarePods(d.logger, !isProtectionEnabled)...)
+	if !isProtectionEnabled {
+		d.logger.V(1).Info("Warning: EvictFailedBarePods is set to True. This could cause eviction of pods without ownerReferences.")
+		d.constraints = append(d.constraints, func(pod *v1.Pod) error {
+			ownerRefList := podutil.OwnerRef(pod)
+			if len(ownerRefList) == 0 && pod.Status.Phase != v1.PodFailed {
+				return fmt.Errorf("pod does not have any ownerRefs and is not in failed phase")
+			}
+			return nil
+		})
+	} else {
+		d.constraints = append(d.constraints, func(pod *v1.Pod) error {
+			if len(podutil.OwnerRef(pod)) == 0 {
+				return fmt.Errorf("pod does not have any ownerRefs")
+			}
+			return nil
+		})
+	}
 }
 
 func applySystemCriticalPodsProtection(d *DefaultEvictor, protectionMap map[PodProtection]bool, handle frameworktypes.Handle) error {
 	isProtectionEnabled := protectionMap[SystemCriticalPods]
-	constraints, err := evictionConstraintsForSystemCriticalPods(
-		d.logger,
-		!isProtectionEnabled,
-		d.args.PriorityThreshold,
-		handle,
-	)
-	if err != nil {
-		return err
+	if !isProtectionEnabled {
+		d.logger.V(1).Info("Warning: System critical pod protection is disabled. This could cause eviction of Kubernetes system pods.")
+		return nil
+	}
+
+	d.constraints = append(d.constraints, func(pod *v1.Pod) error {
+		if utils.IsCriticalPriorityPod(pod) {
+			return fmt.Errorf("pod has system critical priority and is protected against eviction")
+		}
+		return nil
+	})
+
+	priorityThreshold := d.args.PriorityThreshold
+	if priorityThreshold != nil && (priorityThreshold.Value != nil || len(priorityThreshold.Name) > 0) {
+		thresholdPriority, err := utils.GetPriorityValueFromPriorityThreshold(context.TODO(), handle.ClientSet(), priorityThreshold)
+		if err != nil {
+			d.logger.Error(err, "failed to get priority threshold")
+			return err
+		}
+		d.constraints = append(d.constraints, func(pod *v1.Pod) error {
+			if !IsPodEvictableBasedOnPriority(pod, thresholdPriority) {
+				return fmt.Errorf("pod has higher priority than specified priority class threshold")
+			}
+			return nil
+		})
 	}
-	d.constraints = append(d.constraints, constraints...)
 	return nil
 }
 
 func applyLocalStoragePodsProtection(d *DefaultEvictor, protectionMap map[PodProtection]bool) {
 	isProtectionEnabled := protectionMap[PodsWithLocalStorage]
-	d.constraints = append(d.constraints, evictionConstraintsForLocalStoragePods(!isProtectionEnabled)...)
+	if isProtectionEnabled {
+		d.constraints = append(d.constraints, func(pod *v1.Pod) error {
+			if utils.IsPodWithLocalStorage(pod) {
+				return fmt.Errorf("pod has local storage and is protected against eviction")
+			}
+			return nil
+		})
+	}
 }
 
 func applyDaemonSetPodsProtection(d *DefaultEvictor, protectionMap map[PodProtection]bool) {
 	isProtectionEnabled := protectionMap[DaemonSetPods]
-	d.constraints = append(d.constraints, evictionConstraintsForDaemonSetPods(!isProtectionEnabled)...)
+	if isProtectionEnabled {
+		d.constraints = append(d.constraints, func(pod *v1.Pod) error {
+			ownerRefList := podutil.OwnerRef(pod)
+			if utils.IsDaemonsetPod(ownerRefList) {
+				return fmt.Errorf("daemonset pods are protected against eviction")
+			}
+			return nil
+		})
+	}
 }
 
 func applyPvcPodsProtection(d *DefaultEvictor, protectionMap map[PodProtection]bool) {
 	isProtectionEnabled := protectionMap[PodsWithPVC]
-	d.constraints = append(d.constraints, evictionConstraintsForPvcPods(isProtectionEnabled)...)
+	if isProtectionEnabled {
+		d.constraints = append(d.constraints, func(pod *v1.Pod) error {
+			if utils.IsPodWithPVC(pod) {
+				return fmt.Errorf("pod with PVC is protected against eviction")
+			}
+			return nil
+		})
+	}
 }
 
 func applyPodsWithoutPDBProtection(d *DefaultEvictor, protectionMap map[PodProtection]bool, handle frameworktypes.Handle) {
 	isProtectionEnabled := protectionMap[PodsWithoutPDB]
-	d.constraints = append(d.constraints, evictionConstraintsForIgnorePodsWithoutPDB(isProtectionEnabled, handle)...)
+	if isProtectionEnabled {
+		d.constraints = append(d.constraints, func(pod *v1.Pod) error {
+			hasPdb, err := utils.IsPodCoveredByPDB(pod, handle.SharedInformerFactory().Policy().V1().PodDisruptionBudgets().Lister())
+			if err != nil {
+				return fmt.Errorf("unable to check if pod is covered by PodDisruptionBudget: %w", err)
+			}
+			if !hasPdb {
+				return fmt.Errorf("pod does not have a PodDisruptionBudget and is protected against eviction")
+			}
+			return nil
+		})
+	}
 }
 
 func applyPodsWithResourceClaimsProtection(d *DefaultEvictor, protectionMap map[PodProtection]bool) {
 	isProtectionEnabled := protectionMap[PodsWithResourceClaims]
-	d.constraints = append(d.constraints, evictionConstraintsForResourceClaimsPods(isProtectionEnabled)...)
+	if isProtectionEnabled {
+		d.constraints = append(d.constraints, func(pod *v1.Pod) error {
+			if utils.IsPodWithResourceClaims(pod) {
+				return fmt.Errorf("pod has ResourceClaims and descheduler is configured to protect ResourceClaims pods")
+			}
+			return nil
+		})
+	}
 }
 
 // getEffectivePodProtections determines which policies are currently active.

pkg/framework/plugins/defaultevictor/defaultevictor_test.go

@@ -794,7 +794,7 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			result: true,
 		},
 		{
-			description: "Pod with local storage is evicted because 'withLocalStorage' is in disabledDefaultPodProtections",
+			description: "Pod with local storage is evicted because 'PodsWithLocalStorage' is in DefaultDisabled",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p18", 400, 0, n1.Name, func(pod *v1.Pod) {
 					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
@@ -813,7 +813,7 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			result: true,
 		},
 		{
-			description: "DaemonSet pod is evicted because 'daemonSetPods' is in disabledDefaultPodProtections",
+			description: "DaemonSet pod is evicted because 'DaemonSetPods' is in DefaultDisabled",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p19", 400, 0, n1.Name, func(pod *v1.Pod) {
 					pod.ObjectMeta.OwnerReferences = []metav1.OwnerReference{
@@ -831,7 +831,7 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			result: true,
 		},
 		{
-			description: "Pod with PVC is not evicted because 'withPVC' is in extraPodProtections",
+			description: "Pod with PVC is not evicted because 'PodsWithPVC' is in ExtraEnabled",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p20", 400, 0, n1.Name, func(pod *v1.Pod) {
 					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
@@ -850,7 +850,7 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			result: false,
 		},
 		{
-			description: "Pod without PDB is not evicted because 'withoutPDB' is in extraPodProtections",
+			description: "Pod without PDB is not evicted because 'PodsWithoutPDB' is in ExtraEnabled",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p21", 400, 0, n1.Name, func(pod *v1.Pod) {
 					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
@@ -862,7 +862,7 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			result: false,
 		},
 		{
-			description: "Pod with ResourceClaims is not evicted because 'PodsWithResourceClaims' is in extraPodProtections",
+			description: "Pod with ResourceClaims is not evicted because 'PodsWithResourceClaims' is in ExtraEnabled",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p20", 400, 0, n1.Name, func(pod *v1.Pod) {
 					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()