Skip to content

Commit efaeeaa

Browse files
tiraboschitiraboschi
committed
Use Boolean logic for descheduler.alpha.kubernetes.io/evict annotation
In all the example and tests descheduler.alpha.kubernetes.io/evict annotation was used with = true value to force a pod to be considered evictable regardless of all the other checks. Surprisingly descheduler.alpha.kubernetes.io/evict=false was also producing the same effect. Properly implement Boolean logic so that descheduler.alpha.kubernetes.io/evict=false can be used to configure a pod to be ignored by the descheduler regardless of all the other checks. Signed-off-by: Simone Tiraboschi <[email protected]>
1 parent 6e9622b commit efaeeaa

File tree

4 files changed

+104
-46
lines changed

4 files changed

+104
-46
lines changed

‎README.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1010,10 +1010,12 @@ never evicted because these pods won't be recreated. (Standalone pods in failed
10101010
* Pods with PVCs are evicted (unless `ignorePvcPods: true` is set).
10111011
* In `LowNodeUtilization` and `RemovePodsViolatingInterPodAntiAffinity`, pods are evicted by their priority from low to high, and if they have same priority,
10121012
best effort pods are evicted before burstable and guaranteed pods.
1013-
* All types of pods with the annotation `descheduler.alpha.kubernetes.io/evict` are eligible for eviction. This
1014-
annotation is used to override checks which prevent eviction and users can select which pod is evicted.
1013+
* `descheduler.alpha.kubernetes.io/evict` annotation can be used to force eviction eligibility.
1014+
All types of pods with the annotation `descheduler.alpha.kubernetes.io/evict=true` are eligible for eviction.
1015+
This annotation is used to override checks which prevent eviction and users can select which pod is evicted.
10151016
Users should know how and if the pod will be recreated.
10161017
The annotation only affects internal descheduler checks.
1018+
`descheduler.alpha.kubernetes.io/evict=false` will force a pod to be considered not eligible for eviction.
10171019
The anti-disruption protection provided by the [/eviction](https://kubernetes.io/docs/concepts/scheduling-eviction/api-eviction/)
10181020
subresource is still respected.
10191021
* Pods with a non-nil DeletionTimestamp are not evicted by default.

‎pkg/framework/plugins/defaultevictor/defaultevictor.go

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"context"
1919
"errors"
2020
"fmt"
21+
"strings"
2122
"time"
2223

2324
v1 "k8s.io/api/core/v1"
@@ -58,10 +59,20 @@ func IsPodEvictableBasedOnPriority(pod *v1.Pod, priority int32) bool {
5859
return pod.Spec.Priority == nil || *pod.Spec.Priority < priority
5960
}
6061

61-
// HaveEvictAnnotation checks if the pod have evict annotation
62-
func HaveEvictAnnotation(pod *v1.Pod) bool {
63-
_, found := pod.ObjectMeta.Annotations[evictPodAnnotationKey]
64-
return found
62+
// HaveEvictAnnotation checks if the pod have evict annotation and its value
63+
func HaveEvictAnnotation(pod *v1.Pod) (bool, bool) {
64+
svalue, found := pod.ObjectMeta.Annotations[evictPodAnnotationKey]
65+
bvalue := false
66+
if found {
67+
switch {
68+
case strings.EqualFold(svalue, "false"):
69+
bvalue = false
70+
// backward compatibility
71+
default:
72+
bvalue = true
73+
}
74+
}
75+
return bvalue, found
6576
}
6677

6778
// New builds plugin from its arguments while passing a handle
@@ -236,8 +247,11 @@ func (d *DefaultEvictor) PreEvictionFilter(pod *v1.Pod) bool {
236247
func (d *DefaultEvictor) Filter(pod *v1.Pod) bool {
237248
checkErrs := []error{}
238249

239-
if HaveEvictAnnotation(pod) {
240-
return true
250+
if value, found := HaveEvictAnnotation(pod); found {
251+
if value {
252+
return true
253+
}
254+
checkErrs = append(checkErrs, fmt.Errorf("pod is annotated with %v=%v", evictPodAnnotationKey, value))
241255
}
242256

243257
if utils.IsMirrorPod(pod) {

‎pkg/framework/plugins/defaultevictor/defaultevictor_test.go

Lines changed: 31 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -376,10 +376,10 @@ func TestDefaultEvictorFilter(t *testing.T) {
376376
evictSystemCriticalPods: false,
377377
result: true,
378378
}, {
379-
description: "Normal pod eviction with normal ownerRefs and descheduler.alpha.kubernetes.io/evict annotation",
379+
description: "Normal pod eviction with normal ownerRefs and descheduler.alpha.kubernetes.io/evict=true annotation",
380380
pods: []*v1.Pod{
381381
test.BuildTestPod("p2", 400, 0, n1.Name, func(pod *v1.Pod) {
382-
pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
382+
pod.Annotations = map[string]string{evictPodAnnotationKey: "true"}
383383
pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
384384
}),
385385
},
@@ -397,10 +397,10 @@ func TestDefaultEvictorFilter(t *testing.T) {
397397
evictSystemCriticalPods: false,
398398
result: true,
399399
}, {
400-
description: "Normal pod eviction with replicaSet ownerRefs and descheduler.alpha.kubernetes.io/evict annotation",
400+
description: "Normal pod eviction with replicaSet ownerRefs and descheduler.alpha.kubernetes.io/evict=true annotation",
401401
pods: []*v1.Pod{
402402
test.BuildTestPod("p4", 400, 0, n1.Name, func(pod *v1.Pod) {
403-
pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
403+
pod.Annotations = map[string]string{evictPodAnnotationKey: "true"}
404404
pod.ObjectMeta.OwnerReferences = test.GetReplicaSetOwnerRefList()
405405
}),
406406
},
@@ -418,16 +418,27 @@ func TestDefaultEvictorFilter(t *testing.T) {
418418
evictSystemCriticalPods: false,
419419
result: true,
420420
}, {
421-
description: "Normal pod eviction with statefulSet ownerRefs and descheduler.alpha.kubernetes.io/evict annotation",
421+
description: "Normal pod eviction with statefulSet ownerRefs and descheduler.alpha.kubernetes.io/evict=true annotation",
422422
pods: []*v1.Pod{
423423
test.BuildTestPod("p19", 400, 0, n1.Name, func(pod *v1.Pod) {
424-
pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
424+
pod.Annotations = map[string]string{evictPodAnnotationKey: "true"}
425425
pod.ObjectMeta.OwnerReferences = test.GetStatefulSetOwnerRefList()
426426
}),
427427
},
428428
evictLocalStoragePods: false,
429429
evictSystemCriticalPods: false,
430430
result: true,
431+
}, {
432+
description: "Pod not evicted because it has descheduler.alpha.kubernetes.io/evict=false annotation",
433+
pods: []*v1.Pod{
434+
test.BuildTestPod("p2", 400, 0, n1.Name, func(pod *v1.Pod) {
435+
pod.Annotations = map[string]string{evictPodAnnotationKey: "false"}
436+
pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
437+
}),
438+
},
439+
evictLocalStoragePods: false,
440+
evictSystemCriticalPods: false,
441+
result: false,
431442
}, {
432443
description: "Pod not evicted because it is bound to a PV and evictLocalStoragePods = false",
433444
pods: []*v1.Pod{
@@ -471,10 +482,10 @@ func TestDefaultEvictorFilter(t *testing.T) {
471482
evictSystemCriticalPods: false,
472483
result: true,
473484
}, {
474-
description: "Pod is evicted because it is bound to a PV and evictLocalStoragePods = false, but it has scheduler.alpha.kubernetes.io/evict annotation",
485+
description: "Pod is evicted because it is bound to a PV and evictLocalStoragePods = false, but it has descheduler.alpha.kubernetes.io/evict=true annotation",
475486
pods: []*v1.Pod{
476487
test.BuildTestPod("p7", 400, 0, n1.Name, func(pod *v1.Pod) {
477-
pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
488+
pod.Annotations = map[string]string{evictPodAnnotationKey: "true"}
478489
pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
479490
pod.Spec.Volumes = []v1.Volume{
480491
{
@@ -504,10 +515,10 @@ func TestDefaultEvictorFilter(t *testing.T) {
504515
evictSystemCriticalPods: false,
505516
result: false,
506517
}, {
507-
description: "Pod is evicted because it is part of a daemonSet, but it has scheduler.alpha.kubernetes.io/evict annotation",
518+
description: "Pod is evicted because it is part of a daemonSet, but it has descheduler.alpha.kubernetes.io/evict=true annotation",
508519
pods: []*v1.Pod{
509520
test.BuildTestPod("p9", 400, 0, n1.Name, func(pod *v1.Pod) {
510-
pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
521+
pod.Annotations = map[string]string{evictPodAnnotationKey: "true"}
511522
pod.ObjectMeta.OwnerReferences = test.GetDaemonSetOwnerRefList()
512523
}),
513524
},
@@ -526,12 +537,12 @@ func TestDefaultEvictorFilter(t *testing.T) {
526537
evictSystemCriticalPods: false,
527538
result: false,
528539
}, {
529-
description: "Pod is evicted because it is a mirror pod, but it has scheduler.alpha.kubernetes.io/evict annotation",
540+
description: "Pod is evicted because it is a mirror pod, but it has descheduler.alpha.kubernetes.io/evict=true annotation",
530541
pods: []*v1.Pod{
531542
test.BuildTestPod("p11", 400, 0, n1.Name, func(pod *v1.Pod) {
532543
pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
533544
pod.Annotations = test.GetMirrorPodAnnotation()
534-
pod.Annotations["descheduler.alpha.kubernetes.io/evict"] = "true"
545+
pod.Annotations[evictPodAnnotationKey] = "true"
535546
}),
536547
},
537548
evictLocalStoragePods: false,
@@ -550,14 +561,14 @@ func TestDefaultEvictorFilter(t *testing.T) {
550561
evictSystemCriticalPods: false,
551562
result: false,
552563
}, {
553-
description: "Pod is evicted because it has system critical priority, but it has scheduler.alpha.kubernetes.io/evict annotation",
564+
description: "Pod is evicted because it has system critical priority, but it has descheduler.alpha.kubernetes.io/evict=true annotation",
554565
pods: []*v1.Pod{
555566
test.BuildTestPod("p13", 400, 0, n1.Name, func(pod *v1.Pod) {
556567
pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
557568
priority := utils.SystemCriticalPriority
558569
pod.Spec.Priority = &priority
559570
pod.Annotations = map[string]string{
560-
"descheduler.alpha.kubernetes.io/evict": "true",
571+
evictPodAnnotationKey: "true",
561572
}
562573
}),
563574
},
@@ -577,11 +588,11 @@ func TestDefaultEvictorFilter(t *testing.T) {
577588
priorityThreshold: &lowPriority,
578589
result: false,
579590
}, {
580-
description: "Pod is evicted because it has a priority higher than the configured priority threshold, but it has scheduler.alpha.kubernetes.io/evict annotation",
591+
description: "Pod is evicted because it has a priority higher than the configured priority threshold, but it has descheduler.alpha.kubernetes.io/evict=true annotation",
581592
pods: []*v1.Pod{
582593
test.BuildTestPod("p15", 400, 0, n1.Name, func(pod *v1.Pod) {
583594
pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
584-
pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
595+
pod.Annotations = map[string]string{evictPodAnnotationKey: "true"}
585596
pod.Spec.Priority = &highPriority
586597
}),
587598
},
@@ -602,11 +613,11 @@ func TestDefaultEvictorFilter(t *testing.T) {
602613
evictSystemCriticalPods: true,
603614
result: true,
604615
}, {
605-
description: "Pod is evicted because it has system critical priority, but evictSystemCriticalPods = true and it has scheduler.alpha.kubernetes.io/evict annotation",
616+
description: "Pod is evicted because it has system critical priority, but evictSystemCriticalPods = true and it has descheduler.alpha.kubernetes.io/evict=true annotation",
606617
pods: []*v1.Pod{
607618
test.BuildTestPod("p16", 400, 0, n1.Name, func(pod *v1.Pod) {
608619
pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
609-
pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
620+
pod.Annotations = map[string]string{evictPodAnnotationKey: "true"}
610621
priority := utils.SystemCriticalPriority
611622
pod.Spec.Priority = &priority
612623
}),
@@ -627,11 +638,11 @@ func TestDefaultEvictorFilter(t *testing.T) {
627638
priorityThreshold: &lowPriority,
628639
result: true,
629640
}, {
630-
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",
641+
description: "Pod is evicted because it has a priority higher than the configured priority threshold, but evictSystemCriticalPods = true and it has descheduler.alpha.kubernetes.io/evict=true annotation",
631642
pods: []*v1.Pod{
632643
test.BuildTestPod("p17", 400, 0, n1.Name, func(pod *v1.Pod) {
633644
pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
634-
pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
645+
pod.Annotations = map[string]string{evictPodAnnotationKey: "true"}
635646
pod.Spec.Priority = &highPriority
636647
}),
637648
},

‎test/e2e/e2e_test.go

Lines changed: 49 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,12 @@ package e2e
1919
import (
2020
"bufio"
2121
"context"
22+
"errors"
2223
"fmt"
2324
"math"
2425
"os"
2526
"sort"
27+
"strconv"
2628
"strings"
2729
"testing"
2830
"time"
@@ -1211,8 +1213,17 @@ func TestPodLabelSelector(t *testing.T) {
12111213
}
12121214
}
12131215

1214-
func TestEvictAnnotation(t *testing.T) {
1216+
func TestEvictAnnotationFalse(t *testing.T) {
1217+
testEvictAnnotation(t, false)
1218+
}
1219+
1220+
func TestEvictAnnotationTrue(t *testing.T) {
1221+
testEvictAnnotation(t, true)
1222+
}
1223+
1224+
func testEvictAnnotation(t *testing.T, evictAnnotationValue bool) {
12151225
ctx := context.Background()
1226+
const podNumber = 5
12161227

12171228
clientSet, _, nodeLister, _ := initializeClient(ctx, t)
12181229

@@ -1222,18 +1233,26 @@ func TestEvictAnnotation(t *testing.T) {
12221233
}
12231234
defer clientSet.CoreV1().Namespaces().Delete(ctx, testNamespace.Name, metav1.DeleteOptions{})
12241235

1225-
t.Log("Create RC with pods with local storage which require descheduler.alpha.kubernetes.io/evict annotation to be set for eviction")
1226-
rc := RcByNameContainer("test-rc-evict-annotation", testNamespace.Name, int32(5), map[string]string{"test": "annotation"}, nil, "")
1227-
rc.Spec.Template.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"}
1228-
rc.Spec.Template.Spec.Volumes = []v1.Volume{
1229-
{
1230-
Name: "sample",
1231-
VolumeSource: v1.VolumeSource{
1232-
EmptyDir: &v1.EmptyDirVolumeSource{
1233-
SizeLimit: resource.NewQuantity(int64(10), resource.BinarySI),
1236+
if evictAnnotationValue {
1237+
t.Log("Create RC with pods with local storage which require descheduler.alpha.kubernetes.io/evict=true annotation to be set for eviction")
1238+
} else {
1239+
t.Log("Create RC with pods with descheduler.alpha.kubernetes.io/evict=false annotation to skip eviction")
1240+
}
1241+
1242+
rc := RcByNameContainer("test-rc-evict-annotation", testNamespace.Name, int32(podNumber), map[string]string{"test": "annotation"}, nil, "")
1243+
rc.Spec.Template.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": strconv.FormatBool(evictAnnotationValue)}
1244+
if evictAnnotationValue {
1245+
t.Logf("Adding a local storage volume")
1246+
rc.Spec.Template.Spec.Volumes = []v1.Volume{
1247+
{
1248+
Name: "sample",
1249+
VolumeSource: v1.VolumeSource{
1250+
EmptyDir: &v1.EmptyDirVolumeSource{
1251+
SizeLimit: resource.NewQuantity(int64(10), resource.BinarySI),
1252+
},
12341253
},
12351254
},
1236-
},
1255+
}
12371256
}
12381257

12391258
if _, err := clientSet.CoreV1().ReplicationControllers(rc.Namespace).Create(ctx, rc, metav1.CreateOptions{}); err != nil {
@@ -1266,19 +1285,31 @@ func TestEvictAnnotation(t *testing.T) {
12661285
return false, fmt.Errorf("unable to list pods after running plugin: %v", err)
12671286
}
12681287

1269-
excludePodNames := getPodNames(podList.Items)
1270-
sort.Strings(excludePodNames)
1271-
t.Logf("Existing pods: %v", excludePodNames)
1288+
existingPodNames := getPodNames(podList.Items)
1289+
sort.Strings(existingPodNames)
1290+
t.Logf("Existing pods: %v", existingPodNames)
12721291

1273-
// validate no pods were deleted
1274-
if len(intersectStrings(initialPodNames, excludePodNames)) > 0 {
1275-
t.Logf("Not every pods was evicted")
1292+
if evictAnnotationValue {
1293+
if len(intersectStrings(initialPodNames, existingPodNames)) > 0 {
1294+
t.Logf("Not every pods was evicted")
1295+
return false, nil
1296+
}
1297+
} else {
1298+
// validate no pods were deleted
1299+
if len(intersectStrings(initialPodNames, existingPodNames)) != podNumber {
1300+
t.Logf("At least a pod was evicted")
1301+
return false, fmt.Errorf("none of %v unevictable pods are expected to be deleted", initialPodNames)
1302+
}
12761303
return false, nil
12771304
}
12781305

12791306
return true, nil
12801307
}); err != nil {
1281-
t.Fatalf("Error waiting for pods to be deleted: %v", err)
1308+
if !evictAnnotationValue && (wait.Interrupted(err) || errors.Is(err, context.DeadlineExceeded)) {
1309+
t.Logf("No one of the unevictable pod got evicted")
1310+
} else {
1311+
t.Fatalf("Error waiting for pods to be deleted: %v", err)
1312+
}
12821313
}
12831314
}
12841315

0 commit comments

Comments
 (0)