From 36e3d1e70334da699e86abe8f80bed2dfb239c93 Mon Sep 17 00:00:00 2001 From: Jan Chaloupka Date: Sun, 19 Apr 2020 21:58:39 +0200 Subject: [PATCH 1/4] Drop local implementation of toleratesTaint in favor of k8s.io/api/core/v1.Toleration.TolerateTaint Functionally identical implementation of toleratesTaint is already provided in k8s.io/api --- pkg/descheduler/strategies/node_taint.go | 20 +------------------ pkg/descheduler/strategies/node_taint_test.go | 2 +- 2 files changed, 2 insertions(+), 20 deletions(-) diff --git a/pkg/descheduler/strategies/node_taint.go b/pkg/descheduler/strategies/node_taint.go index f85a8403d..ab4835888 100644 --- a/pkg/descheduler/strategies/node_taint.go +++ b/pkg/descheduler/strategies/node_taint.go @@ -97,24 +97,6 @@ func getNoScheduleTaints(taints []v1.Taint) []v1.Taint { return result } -//toleratesTaint returns true if a toleration tolerates a taint, or false otherwise -func toleratesTaint(toleration *v1.Toleration, taint *v1.Taint) bool { - - if (len(toleration.Key) > 0 && toleration.Key != taint.Key) || - (len(toleration.Effect) > 0 && toleration.Effect != taint.Effect) { - return false - } - switch toleration.Operator { - // empty operator means Equal - case "", TolerationOpEqual: - return toleration.Value == taint.Value - case TolerationOpExists: - return true - default: - return false - } -} - // allTaintsTolerated returns true if all are tolerated, or false otherwise. func allTaintsTolerated(taints []v1.Taint, tolerations []v1.Toleration) bool { if len(taints) == 0 { @@ -126,7 +108,7 @@ func allTaintsTolerated(taints []v1.Taint, tolerations []v1.Toleration) bool { for i := range taints { tolerated := false for j := range tolerations { - if toleratesTaint(&tolerations[j], &taints[i]) { + if tolerations[j].ToleratesTaint(&taints[i]) { tolerated = true break } diff --git a/pkg/descheduler/strategies/node_taint_test.go b/pkg/descheduler/strategies/node_taint_test.go index 7e467fb02..3e5c41641 100644 --- a/pkg/descheduler/strategies/node_taint_test.go +++ b/pkg/descheduler/strategies/node_taint_test.go @@ -272,7 +272,7 @@ func TestToleratesTaint(t *testing.T) { }, } for _, tc := range testCases { - if tolerated := toleratesTaint(&tc.toleration, &tc.taint); tc.expectTolerated != tolerated { + if tolerated := tc.toleration.ToleratesTaint(&tc.taint); tc.expectTolerated != tolerated { t.Errorf("[%s] expect %v, got %v: toleration %+v, taint %s", tc.description, tc.expectTolerated, tolerated, tc.toleration, tc.taint.ToString()) } } From 0e9b33b822a5e473d1f19c4c276f729494825075 Mon Sep 17 00:00:00 2001 From: Jan Chaloupka Date: Sun, 19 Apr 2020 22:07:34 +0200 Subject: [PATCH 2/4] allTaintsTolerated: remove for iteration through tolerations which is already implemented in utils.TolerationsTolerateTaint --- pkg/descheduler/strategies/node_taint.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/pkg/descheduler/strategies/node_taint.go b/pkg/descheduler/strategies/node_taint.go index ab4835888..78a8de6da 100644 --- a/pkg/descheduler/strategies/node_taint.go +++ b/pkg/descheduler/strategies/node_taint.go @@ -102,18 +102,11 @@ func allTaintsTolerated(taints []v1.Taint, tolerations []v1.Toleration) bool { if len(taints) == 0 { return true } - if len(tolerations) == 0 && len(taints) > 0 { + if len(tolerations) == 0 { return false } for i := range taints { - tolerated := false - for j := range tolerations { - if tolerations[j].ToleratesTaint(&taints[i]) { - tolerated = true - break - } - } - if !tolerated { + if !utils.TolerationsTolerateTaint(tolerations, &taints[i]) { return false } } From 1c300a98819f90e0379d0c1a89aa039883819ffe Mon Sep 17 00:00:00 2001 From: Jan Chaloupka Date: Sun, 19 Apr 2020 22:18:49 +0200 Subject: [PATCH 3/4] Drop getNoScheduleTaints and allTaintsTolerated in favor of utils.TolerationsTolerateTaintsWithFilter --- pkg/descheduler/strategies/node_taint.go | 39 +++---------------- pkg/descheduler/strategies/node_taint_test.go | 19 --------- 2 files changed, 5 insertions(+), 53 deletions(-) diff --git a/pkg/descheduler/strategies/node_taint.go b/pkg/descheduler/strategies/node_taint.go index 78a8de6da..b018d006b 100644 --- a/pkg/descheduler/strategies/node_taint.go +++ b/pkg/descheduler/strategies/node_taint.go @@ -73,42 +73,13 @@ func deletePodsViolatingNodeTaints(client clientset.Interface, policyGroupVersio // checkPodsSatisfyTolerations checks if the node's taints (NoSchedule) are still satisfied by pods' tolerations. func checkPodsSatisfyTolerations(pod *v1.Pod, node *v1.Node) bool { - tolerations := pod.Spec.Tolerations - taints := node.Spec.Taints - if len(taints) == 0 { - return true - } - noScheduleTaints := getNoScheduleTaints(taints) - if !allTaintsTolerated(noScheduleTaints, tolerations) { + if !utils.TolerationsTolerateTaintsWithFilter( + pod.Spec.Tolerations, + node.Spec.Taints, + func(taint *v1.Taint) bool { return taint.Effect == v1.TaintEffectNoSchedule }, + ) { klog.V(2).Infof("Not all taints are tolerated after update for Pod %v on node %v", pod.Name, node.Name) return false } return true } - -// getNoScheduleTaints return a slice of NoSchedule taints from the a slice of taints that it receives. -func getNoScheduleTaints(taints []v1.Taint) []v1.Taint { - result := []v1.Taint{} - for i := range taints { - if taints[i].Effect == v1.TaintEffectNoSchedule { - result = append(result, taints[i]) - } - } - return result -} - -// allTaintsTolerated returns true if all are tolerated, or false otherwise. -func allTaintsTolerated(taints []v1.Taint, tolerations []v1.Toleration) bool { - if len(taints) == 0 { - return true - } - if len(tolerations) == 0 { - return false - } - for i := range taints { - if !utils.TolerationsTolerateTaint(tolerations, &taints[i]) { - return false - } - } - return true -} diff --git a/pkg/descheduler/strategies/node_taint_test.go b/pkg/descheduler/strategies/node_taint_test.go index 3e5c41641..8749aa972 100644 --- a/pkg/descheduler/strategies/node_taint_test.go +++ b/pkg/descheduler/strategies/node_taint_test.go @@ -277,22 +277,3 @@ func TestToleratesTaint(t *testing.T) { } } } - -func TestFilterNoExecuteTaints(t *testing.T) { - taints := []v1.Taint{ - { - Key: "one", - Value: "one", - Effect: v1.TaintEffectNoExecute, - }, - { - Key: "two", - Value: "two", - Effect: v1.TaintEffectNoSchedule, - }, - } - taints = getNoScheduleTaints(taints) - if len(taints) != 1 || taints[0].Key != "two" { - t.Errorf("Filtering doesn't work. Got %v", taints) - } -} From 6db7c3b92ccf5316ef8ba236285120fff147dc52 Mon Sep 17 00:00:00 2001 From: Jan Chaloupka Date: Sun, 19 Apr 2020 22:23:27 +0200 Subject: [PATCH 4/4] Call utils.TolerationsTolerateTaintsWithFilter directly, not through checkPodsSatisfyTolerations --- pkg/descheduler/strategies/node_taint.go | 25 +++++-------------- pkg/descheduler/strategies/node_taint_test.go | 12 ++++----- 2 files changed, 12 insertions(+), 25 deletions(-) diff --git a/pkg/descheduler/strategies/node_taint.go b/pkg/descheduler/strategies/node_taint.go index b018d006b..3fa63518b 100644 --- a/pkg/descheduler/strategies/node_taint.go +++ b/pkg/descheduler/strategies/node_taint.go @@ -28,11 +28,6 @@ import ( "k8s.io/klog" ) -const ( - TolerationOpExists v1.TolerationOperator = "Exists" - TolerationOpEqual v1.TolerationOperator = "Equal" -) - // RemovePodsViolatingNodeTaints with elimination strategy func RemovePodsViolatingNodeTaints(ds *options.DeschedulerServer, strategy api.DeschedulerStrategy, policyGroupVersion string, nodes []*v1.Node, nodePodCount utils.NodePodEvictedCount) { if !strategy.Enabled { @@ -56,7 +51,12 @@ func deletePodsViolatingNodeTaints(client clientset.Interface, policyGroupVersio if maxPodsToEvict > 0 && nodePodCount[node]+1 > maxPodsToEvict { break } - if !checkPodsSatisfyTolerations(pods[i], node) { + if !utils.TolerationsTolerateTaintsWithFilter( + pods[i].Spec.Tolerations, + node.Spec.Taints, + func(taint *v1.Taint) bool { return taint.Effect == v1.TaintEffectNoSchedule }, + ) { + klog.V(2).Infof("Not all taints with NoSchedule effect are tolerated after update for pod %v on node %v", pods[i].Name, node.Name) success, err := evictions.EvictPod(client, pods[i], policyGroupVersion, dryRun) if !success { klog.Errorf("Error when evicting pod: %#v (%#v)\n", pods[i].Name, err) @@ -70,16 +70,3 @@ func deletePodsViolatingNodeTaints(client clientset.Interface, policyGroupVersio } return podsEvicted } - -// checkPodsSatisfyTolerations checks if the node's taints (NoSchedule) are still satisfied by pods' tolerations. -func checkPodsSatisfyTolerations(pod *v1.Pod, node *v1.Node) bool { - if !utils.TolerationsTolerateTaintsWithFilter( - pod.Spec.Tolerations, - node.Spec.Taints, - func(taint *v1.Taint) bool { return taint.Effect == v1.TaintEffectNoSchedule }, - ) { - klog.V(2).Infof("Not all taints are tolerated after update for Pod %v on node %v", pod.Name, node.Name) - return false - } - return true -} diff --git a/pkg/descheduler/strategies/node_taint_test.go b/pkg/descheduler/strategies/node_taint_test.go index 8749aa972..509844a52 100644 --- a/pkg/descheduler/strategies/node_taint_test.go +++ b/pkg/descheduler/strategies/node_taint_test.go @@ -188,7 +188,7 @@ func TestToleratesTaint(t *testing.T) { description: "toleration and taint have the same key and effect, and operator is Exists, and taint has no value, expect tolerated", toleration: v1.Toleration{ Key: "foo", - Operator: TolerationOpExists, + Operator: v1.TolerationOpExists, Effect: v1.TaintEffectNoSchedule, }, taint: v1.Taint{ @@ -201,7 +201,7 @@ func TestToleratesTaint(t *testing.T) { description: "toleration and taint have the same key and effect, and operator is Exists, and taint has some value, expect tolerated", toleration: v1.Toleration{ Key: "foo", - Operator: TolerationOpExists, + Operator: v1.TolerationOpExists, Effect: v1.TaintEffectNoSchedule, }, taint: v1.Taint{ @@ -215,7 +215,7 @@ func TestToleratesTaint(t *testing.T) { description: "toleration and taint have the same effect, toleration has empty key and operator is Exists, means match all taints, expect tolerated", toleration: v1.Toleration{ Key: "", - Operator: TolerationOpExists, + Operator: v1.TolerationOpExists, Effect: v1.TaintEffectNoSchedule, }, taint: v1.Taint{ @@ -229,7 +229,7 @@ func TestToleratesTaint(t *testing.T) { description: "toleration and taint have the same key, effect and value, and operator is Equal, expect tolerated", toleration: v1.Toleration{ Key: "foo", - Operator: TolerationOpEqual, + Operator: v1.TolerationOpEqual, Value: "bar", Effect: v1.TaintEffectNoSchedule, }, @@ -244,7 +244,7 @@ func TestToleratesTaint(t *testing.T) { description: "toleration and taint have the same key and effect, but different values, and operator is Equal, expect not tolerated", toleration: v1.Toleration{ Key: "foo", - Operator: TolerationOpEqual, + Operator: v1.TolerationOpEqual, Value: "value1", Effect: v1.TaintEffectNoSchedule, }, @@ -259,7 +259,7 @@ func TestToleratesTaint(t *testing.T) { description: "toleration and taint have the same key and value, but different effects, and operator is Equal, expect not tolerated", toleration: v1.Toleration{ Key: "foo", - Operator: TolerationOpEqual, + Operator: v1.TolerationOpEqual, Value: "bar", Effect: v1.TaintEffectNoSchedule, },