Deprecate descheduler.alpha.kubernetes.io/evict in favor of descheduler.beta.kubernetes.io/evict with Boolean logic

In the past the descheduler was only checking the presence
of descheduler.alpha.kubernetes.io/evict annotation to force
a pod to be an eviction candidate.
descheduler.alpha.kubernetes.io/evict=false was producing the
same effect of descheduler.alpha.kubernetes.io/evict=true.
If the user wants to exclude a specific pod from the descheduler
actions, the suggested solution was to re-create it
setting an higher priority.
This could potentially work for best effort pods that can
be easily recreated but it's not a viable solution for other use
cases like VMs (executed in pod) that can be temporarily
excluded from the descheduler action without the need to reboot
them.
Let's deprecate descheduler.alpha.kubernetes.io/evict introducing
descheduler.beta.kubernetes.io/evict with proper Boolean logic.

Signed-off-by: Simone Tiraboschi <[email protected]>

Author: tiraboschi

README.md

@@ -1013,10 +1013,16 @@ never evicted because these pods won't be recreated. (Standalone pods in failed
 best effort pods are evicted before burstable and guaranteed pods.
 * All types of pods with the annotation `descheduler.alpha.kubernetes.io/evict` are eligible for eviction. This
   annotation is used to override checks which prevent eviction and users can select which pod is evicted.
+  This annotation is deprecated and could be ignored in future releases. Please use `descheduler.beta.kubernetes.io/evict=true|false` instead.
+* `descheduler.beta.kubernetes.io/evict` annotation can be used to selectively force eviction eligibility.
+  All types of pods with the annotation `descheduler.beta.kubernetes.io/evict=true` are eligible for eviction.
+  This annotation is used to override checks which prevent eviction and users can select which pod is evicted.
   Users should know how and if the pod will be recreated.
   The annotation only affects internal descheduler checks.
+  `descheduler.beta.kubernetes.io/evict=false` will force a pod to be considered not eligible for eviction.
   The anti-disruption protection provided by the [/eviction](https://kubernetes.io/docs/concepts/scheduling-eviction/api-eviction/)
   subresource is still respected.
+  If both `descheduler.alpha.kubernetes.io/evict` and `descheduler.beta.kubernetes.io/evict` are set at the same time, the value of `descheduler.beta.kubernetes.io/evict` will be consumed.
 * Pods with a non-nil DeletionTimestamp are not evicted by default.
 
 Setting `--v=4` or greater on the Descheduler will log all reasons why any pod is not evictable.

pkg/framework/plugins/defaultevictor/defaultevictor.go

@@ -18,6 +18,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strconv"
 	"time"
 
 	v1 "k8s.io/api/core/v1"
@@ -34,8 +35,9 @@ import (
 )
 
 const (
-	PluginName            = "DefaultEvictor"
-	evictPodAnnotationKey = "descheduler.alpha.kubernetes.io/evict"
+	PluginName                 = "DefaultEvictor"
+	evictPodAnnotationKeyAlpha = "descheduler.alpha.kubernetes.io/evict"
+	evictPodAnnotationKeyBeta  = "descheduler.beta.kubernetes.io/evict"
 )
 
 var _ frameworktypes.EvictorPlugin = &DefaultEvictor{}
@@ -58,10 +60,27 @@ func IsPodEvictableBasedOnPriority(pod *v1.Pod, priority int32) bool {
 	return pod.Spec.Priority == nil || *pod.Spec.Priority < priority
 }
 
-// HaveEvictAnnotation checks if the pod have evict annotation
-func HaveEvictAnnotation(pod *v1.Pod) bool {
-	_, found := pod.ObjectMeta.Annotations[evictPodAnnotationKey]
-	return found
+// HaveEvictAnnotation checks if the pod have evict annotation and its value
+func HaveEvictAnnotation(pod *v1.Pod) (bool, bool, string) {
+	svalue, found := pod.ObjectMeta.Annotations[evictPodAnnotationKeyBeta]
+	bvalue := false
+	annotationKey := ""
+	if found {
+		annotationKey = evictPodAnnotationKeyBeta
+		c, err := strconv.ParseBool(svalue)
+		if err != nil {
+			bvalue = false
+		} else {
+			bvalue = c
+		}
+	} else {
+		// backward compatibility
+		if _, found = pod.ObjectMeta.Annotations[evictPodAnnotationKeyAlpha]; found {
+			annotationKey = evictPodAnnotationKeyAlpha
+			bvalue = true
+		}
+	}
+	return bvalue, found, annotationKey
 }
 
 // New builds plugin from its arguments while passing a handle
@@ -236,8 +255,11 @@ func (d *DefaultEvictor) PreEvictionFilter(pod *v1.Pod) bool {
 func (d *DefaultEvictor) Filter(pod *v1.Pod) bool {
 	checkErrs := []error{}
 
-	if HaveEvictAnnotation(pod) {
-		return true
+	if value, found, annotationKey := HaveEvictAnnotation(pod); found {
+		if value {
+			return true
+		}
+		checkErrs = append(checkErrs, fmt.Errorf("pod is annotated with %v=%v", annotationKey, value))
 	}
 
 	if utils.IsMirrorPod(pod) {

pkg/framework/plugins/defaultevictor/defaultevictor_test.go

@@ -379,7 +379,18 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			description: "Normal pod eviction with normal ownerRefs and descheduler.alpha.kubernetes.io/evict annotation",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p2", 400, 0, n1.Name, func(pod *v1.Pod) {
-					pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
+					pod.Annotations = map[string]string{evictPodAnnotationKeyAlpha: ""}
+					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
+				}),
+			},
+			evictLocalStoragePods:   false,
+			evictSystemCriticalPods: false,
+			result:                  true,
+		}, {
+			description: "Normal pod eviction with normal ownerRefs and descheduler.beta.kubernetes.io/evict=true annotation",
+			pods: []*v1.Pod{
+				test.BuildTestPod("p2", 400, 0, n1.Name, func(pod *v1.Pod) {
+					pod.Annotations = map[string]string{evictPodAnnotationKeyBeta: "true"}
 					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
 				}),
 			},
@@ -397,10 +408,10 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			evictSystemCriticalPods: false,
 			result:                  true,
 		}, {
-			description: "Normal pod eviction with replicaSet ownerRefs and descheduler.alpha.kubernetes.io/evict annotation",
+			description: "Normal pod eviction with replicaSet ownerRefs and descheduler.beta.kubernetes.io/evict=true annotation",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p4", 400, 0, n1.Name, func(pod *v1.Pod) {
-					pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
+					pod.Annotations = map[string]string{evictPodAnnotationKeyBeta: "true"}
 					pod.ObjectMeta.OwnerReferences = test.GetReplicaSetOwnerRefList()
 				}),
 			},
@@ -418,16 +429,41 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			evictSystemCriticalPods: false,
 			result:                  true,
 		}, {
-			description: "Normal pod eviction with statefulSet ownerRefs and descheduler.alpha.kubernetes.io/evict annotation",
+			description: "Normal pod eviction with statefulSet ownerRefs and descheduler.beta.kubernetes.io/evict=true annotation",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p19", 400, 0, n1.Name, func(pod *v1.Pod) {
-					pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
+					pod.Annotations = map[string]string{evictPodAnnotationKeyBeta: "true"}
 					pod.ObjectMeta.OwnerReferences = test.GetStatefulSetOwnerRefList()
 				}),
 			},
 			evictLocalStoragePods:   false,
 			evictSystemCriticalPods: false,
 			result:                  true,
+		}, {
+			description: "Pod not evicted because it has descheduler.beta.kubernetes.io/evict=false annotation",
+			pods: []*v1.Pod{
+				test.BuildTestPod("p2", 400, 0, n1.Name, func(pod *v1.Pod) {
+					pod.Annotations = map[string]string{evictPodAnnotationKeyBeta: "false"}
+					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
+				}),
+			},
+			evictLocalStoragePods:   false,
+			evictSystemCriticalPods: false,
+			result:                  false,
+		}, {
+			description: "Pod not evicted because it has descheduler.alpha.kubernetes.io/evict but also descheduler.beta.kubernetes.io/evict=false annotation",
+			pods: []*v1.Pod{
+				test.BuildTestPod("p2", 400, 0, n1.Name, func(pod *v1.Pod) {
+					pod.Annotations = map[string]string{
+						evictPodAnnotationKeyAlpha: "",
+						evictPodAnnotationKeyBeta:  "false",
+					}
+					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
+				}),
+			},
+			evictLocalStoragePods:   false,
+			evictSystemCriticalPods: false,
+			result:                  false,
 		}, {
 			description: "Pod not evicted because it is bound to a PV and evictLocalStoragePods = false",
 			pods: []*v1.Pod{
@@ -471,10 +507,32 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			evictSystemCriticalPods: false,
 			result:                  true,
 		}, {
-			description: "Pod is evicted because it is bound to a PV and evictLocalStoragePods = false, but it has scheduler.alpha.kubernetes.io/evict annotation",
+			description: "Pod is evicted because it is bound to a PV and evictLocalStoragePods = false, but it has descheduler.alpha.kubernetes.io/evict annotation",
+			pods: []*v1.Pod{
+				test.BuildTestPod("p7", 400, 0, n1.Name, func(pod *v1.Pod) {
+					pod.Annotations = map[string]string{evictPodAnnotationKeyAlpha: ""}
+					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
+					pod.Spec.Volumes = []v1.Volume{
+						{
+							Name: "sample",
+							VolumeSource: v1.VolumeSource{
+								HostPath: &v1.HostPathVolumeSource{Path: "somePath"},
+								EmptyDir: &v1.EmptyDirVolumeSource{
+									SizeLimit: resource.NewQuantity(int64(10), resource.BinarySI),
+								},
+							},
+						},
+					}
+				}),
+			},
+			evictLocalStoragePods:   false,
+			evictSystemCriticalPods: false,
+			result:                  true,
+		}, {
+			description: "Pod is evicted because it is bound to a PV and evictLocalStoragePods = false, but it has descheduler.beta.kubernetes.io/evict=true annotation",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p7", 400, 0, n1.Name, func(pod *v1.Pod) {
-					pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
+					pod.Annotations = map[string]string{evictPodAnnotationKeyBeta: "true"}
 					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
 					pod.Spec.Volumes = []v1.Volume{
 						{
@@ -504,10 +562,10 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			evictSystemCriticalPods: false,
 			result:                  false,
 		}, {
-			description: "Pod is evicted because it is part of a daemonSet, but it has scheduler.alpha.kubernetes.io/evict annotation",
+			description: "Pod is evicted because it is part of a daemonSet, but it has descheduler.beta.kubernetes.io/evict=true annotation",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p9", 400, 0, n1.Name, func(pod *v1.Pod) {
-					pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
+					pod.Annotations = map[string]string{evictPodAnnotationKeyBeta: "true"}
 					pod.ObjectMeta.OwnerReferences = test.GetDaemonSetOwnerRefList()
 				}),
 			},
@@ -526,12 +584,12 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			evictSystemCriticalPods: false,
 			result:                  false,
 		}, {
-			description: "Pod is evicted because it is a mirror pod, but it has scheduler.alpha.kubernetes.io/evict annotation",
+			description: "Pod is evicted because it is a mirror pod, but it has descheduler.beta.kubernetes.io/evict=true annotation",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p11", 400, 0, n1.Name, func(pod *v1.Pod) {
 					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
 					pod.Annotations = test.GetMirrorPodAnnotation()
-					pod.Annotations["descheduler.alpha.kubernetes.io/evict"] = "true"
+					pod.Annotations[evictPodAnnotationKeyBeta] = "true"
 				}),
 			},
 			evictLocalStoragePods:   false,
@@ -550,14 +608,14 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			evictSystemCriticalPods: false,
 			result:                  false,
 		}, {
-			description: "Pod is evicted because it has system critical priority, but it has scheduler.alpha.kubernetes.io/evict annotation",
+			description: "Pod is evicted because it has system critical priority, but it has descheduler.beta.kubernetes.io/evict=true annotation",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p13", 400, 0, n1.Name, func(pod *v1.Pod) {
 					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
 					priority := utils.SystemCriticalPriority
 					pod.Spec.Priority = &priority
 					pod.Annotations = map[string]string{
-						"descheduler.alpha.kubernetes.io/evict": "true",
+						evictPodAnnotationKeyBeta: "true",
 					}
 				}),
 			},
@@ -577,11 +635,11 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			priorityThreshold:       &lowPriority,
 			result:                  false,
 		}, {
-			description: "Pod is evicted because it has a priority higher than the configured priority threshold, but it has scheduler.alpha.kubernetes.io/evict annotation",
+			description: "Pod is evicted because it has a priority higher than the configured priority threshold, but it has descheduler.beta.kubernetes.io/evict=true annotation",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p15", 400, 0, n1.Name, func(pod *v1.Pod) {
 					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
-					pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
+					pod.Annotations = map[string]string{evictPodAnnotationKeyBeta: "true"}
 					pod.Spec.Priority = &highPriority
 				}),
 			},
@@ -602,11 +660,11 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			evictSystemCriticalPods: true,
 			result:                  true,
 		}, {
-			description: "Pod is evicted because it has system critical priority, but evictSystemCriticalPods = true and it has scheduler.alpha.kubernetes.io/evict annotation",
+			description: "Pod is evicted because it has system critical priority, but evictSystemCriticalPods = true and it has descheduler.beta.kubernetes.io/evict=true annotation",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p16", 400, 0, n1.Name, func(pod *v1.Pod) {
 					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
-					pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
+					pod.Annotations = map[string]string{evictPodAnnotationKeyBeta: "true"}
 					priority := utils.SystemCriticalPriority
 					pod.Spec.Priority = &priority
 				}),
@@ -627,11 +685,11 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			priorityThreshold:       &lowPriority,
 			result:                  true,
 		}, {
-			description: "Pod is evicted because it has a priority higher than the configured priority threshold, but evictSystemCriticalPods = true and it has scheduler.alpha.kubernetes.io/evict annotation",
+			description: "Pod is evicted because it has a priority higher than the configured priority threshold, but evictSystemCriticalPods = true and it has descheduler.beta.kubernetes.io/evict=true annotation",
 			pods: []*v1.Pod{
 				test.BuildTestPod("p17", 400, 0, n1.Name, func(pod *v1.Pod) {
 					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
-					pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
+					pod.Annotations = map[string]string{evictPodAnnotationKeyBeta: "true"}
 					pod.Spec.Priority = &highPriority
 				}),
 			},

test/e2e/e2e_test.go

@@ -23,6 +23,7 @@ import (
 	"math"
 	"os"
 	"sort"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -1211,8 +1212,17 @@ func TestPodLabelSelector(t *testing.T) {
 	}
 }
 
-func TestEvictAnnotation(t *testing.T) {
+func TestEvictAnnotationFalse(t *testing.T) {
+	testEvictAnnotation(t, false)
+}
+
+func TestEvictAnnotationTrue(t *testing.T) {
+	testEvictAnnotation(t, true)
+}
+
+func testEvictAnnotation(t *testing.T, evictAnnotationValue bool) {
 	ctx := context.Background()
+	const podNumber = 5
 
 	clientSet, _, nodeLister, _ := initializeClient(ctx, t)
 
@@ -1222,18 +1232,26 @@ func TestEvictAnnotation(t *testing.T) {
 	}
 	defer clientSet.CoreV1().Namespaces().Delete(ctx, testNamespace.Name, metav1.DeleteOptions{})
 
-	t.Log("Create RC with pods with local storage which require descheduler.alpha.kubernetes.io/evict annotation to be set for eviction")
-	rc := RcByNameContainer("test-rc-evict-annotation", testNamespace.Name, int32(5), map[string]string{"test": "annotation"}, nil, "")
-	rc.Spec.Template.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
-	rc.Spec.Template.Spec.Volumes = []v1.Volume{
-		{
-			Name: "sample",
-			VolumeSource: v1.VolumeSource{
-				EmptyDir: &v1.EmptyDirVolumeSource{
-					SizeLimit: resource.NewQuantity(int64(10), resource.BinarySI),
+	if evictAnnotationValue {
+		t.Log("Create RC with pods with local storage which require descheduler.beta.kubernetes.io/evict=true annotation to be set for eviction")
+	} else {
+		t.Log("Create RC with pods with descheduler.beta.kubernetes.io/evict=false annotation to skip eviction")
+	}
+
+	rc := RcByNameContainer("test-rc-evict-annotation", testNamespace.Name, int32(podNumber), map[string]string{"test": "annotation"}, nil, "")
+	rc.Spec.Template.Annotations = map[string]string{"descheduler.beta.kubernetes.io/evict": strconv.FormatBool(evictAnnotationValue)}
+	if evictAnnotationValue {
+		t.Logf("Adding a local storage volume")
+		rc.Spec.Template.Spec.Volumes = []v1.Volume{
+			{
+				Name: "sample",
+				VolumeSource: v1.VolumeSource{
+					EmptyDir: &v1.EmptyDirVolumeSource{
+						SizeLimit: resource.NewQuantity(int64(10), resource.BinarySI),
+					},
 				},
 			},
-		},
+		}
 	}
 
 	if _, err := clientSet.CoreV1().ReplicationControllers(rc.Namespace).Create(ctx, rc, metav1.CreateOptions{}); err != nil {
@@ -1260,25 +1278,36 @@ func TestEvictAnnotation(t *testing.T) {
 	t.Log("Running PodLifetime plugin")
 	runPodLifetimePlugin(ctx, t, clientSet, nodeLister, nil, "", nil, false, false, nil, nil)
 
-	if err := wait.PollUntilContextTimeout(ctx, 5*time.Second, time.Minute, true, func(ctx context.Context) (bool, error) {
-		podList, err = clientSet.CoreV1().Pods(rc.Namespace).List(ctx, metav1.ListOptions{LabelSelector: labels.SelectorFromSet(rc.Spec.Template.Labels).String()})
-		if err != nil {
-			return false, fmt.Errorf("unable to list pods after running plugin: %v", err)
-		}
+	if evictAnnotationValue {
+		if err := wait.PollUntilContextTimeout(ctx, 5*time.Second, time.Minute, true, func(ctx context.Context) (bool, error) {
+			podList, err = clientSet.CoreV1().Pods(rc.Namespace).List(ctx, metav1.ListOptions{LabelSelector: labels.SelectorFromSet(rc.Spec.Template.Labels).String()})
+			if err != nil {
+				return false, fmt.Errorf("unable to list pods after running plugin: %v", err)
+			}
 
-		excludePodNames := getPodNames(podList.Items)
-		sort.Strings(excludePodNames)
-		t.Logf("Existing pods: %v", excludePodNames)
+			existingPodNames := getPodNames(podList.Items)
+			sort.Strings(existingPodNames)
+			t.Logf("Existing pods: %v", existingPodNames)
+			if len(intersectStrings(initialPodNames, existingPodNames)) > 0 {
+				t.Logf("Not every pods was evicted")
+				return false, nil
+			}
 
-		// validate no pods were deleted
-		if len(intersectStrings(initialPodNames, excludePodNames)) > 0 {
-			t.Logf("Not every pods was evicted")
-			return false, nil
+			return true, nil
+		}); err != nil {
+			t.Fatalf("Error waiting for pods to be deleted: %v", err)
 		}
+	} else {
+		t.Logf("Waiting 10s for eventual deletions")
+		time.Sleep(10 * time.Second)
+		existingPodNames := getPodNames(podList.Items)
+		sort.Strings(existingPodNames)
+		t.Logf("Existing pods: %v", existingPodNames)
 
-		return true, nil
-	}); err != nil {
-		t.Fatalf("Error waiting for pods to be deleted: %v", err)
+		// validate no pods were deleted
+		if len(intersectStrings(initialPodNames, existingPodNames)) != podNumber {
+			t.Fatalf("None of %v pods are expected to be deleted", initialPodNames)
+		}
 	}
 }