From 36dcb57407f7b0a48a25505b385b5cc5afe87550 Mon Sep 17 00:00:00 2001 From: Kevin Date: Sat, 14 Jan 2017 16:18:14 +0800 Subject: [PATCH 1/4] forgiveness library changes --- pkg/api/v1/helpers.go | 108 ++++++- pkg/api/v1/helpers_test.go | 283 ++++++++++++++++++ pkg/kubectl/cmd/taint.go | 29 +- .../algorithm/predicates/predicates.go | 26 +- .../algorithm/priorities/taint_toleration.go | 2 +- test/e2e/framework/util.go | 20 +- 6 files changed, 389 insertions(+), 79 deletions(-) diff --git a/pkg/api/v1/helpers.go b/pkg/api/v1/helpers.go index 97a636a1984..7624223431f 100644 --- a/pkg/api/v1/helpers.go +++ b/pkg/api/v1/helpers.go @@ -20,7 +20,9 @@ import ( "encoding/json" "fmt" "strings" + "time" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/types" @@ -285,6 +287,10 @@ func GetTolerationsFromPodAnnotations(annotations map[string]string) ([]Tolerati return tolerations, nil } +func GetPodTolerations(pod *Pod) ([]Toleration, error) { + return GetTolerationsFromPodAnnotations(pod.Annotations) +} + // GetTaintsFromNodeAnnotations gets the json serialized taints data from Pod.Annotations // and converts it to the []Taint type in api. func GetTaintsFromNodeAnnotations(annotations map[string]string) ([]Taint, error) { @@ -298,35 +304,105 @@ func GetTaintsFromNodeAnnotations(annotations map[string]string) ([]Taint, error return taints, nil } -// TolerationToleratesTaint checks if the toleration tolerates the taint. -func TolerationToleratesTaint(toleration *Toleration, taint *Taint) bool { - if len(toleration.Effect) != 0 && toleration.Effect != taint.Effect { +func GetNodeTaints(node *Node) ([]Taint, error) { + return GetTaintsFromNodeAnnotations(node.Annotations) +} + +// ToleratesTaint checks if the toleration tolerates the taint. +func (t *Toleration) ToleratesTaint(taint *Taint) bool { + // empty toleration effect means match all taint effects + if len(t.Effect) > 0 && t.Effect != taint.Effect { return false } - if toleration.Key != taint.Key { + // empty toleration key means match all taint keys + if len(t.Key) > 0 && t.Key != taint.Key { return false } - // TODO: Use proper defaulting when Toleration becomes a field of PodSpec - if (len(toleration.Operator) == 0 || toleration.Operator == TolerationOpEqual) && toleration.Value == taint.Value { - return true + + // nil TolerationSeconds means tolerate the taint forever + if t.TolerationSeconds != nil { + // taint with no added time indicated can only be tolerated + // by toleration with no tolerationSeconds. + if taint.TimeAdded.IsZero() { + return false + } + + // TODO: need to take time skew into consideration, make sure toleration won't become out of age ealier than expected. + if metav1.Now().After(taint.TimeAdded.Add(time.Second * time.Duration(*t.TolerationSeconds))) { + return false + } } - if toleration.Operator == TolerationOpExists { + + // TODO: Use proper defaulting when Toleration becomes a field of PodSpec + switch t.Operator { + // empty operator means Equal + case "", TolerationOpEqual: + return t.Value == taint.Value + case TolerationOpExists: return true + default: + return false + } +} + +// TolerationsTolerateTaint checks if taint is tolerated by any of the tolerations. +func TolerationsTolerateTaint(tolerations []Toleration, taint *Taint) bool { + for i := range tolerations { + if tolerations[i].ToleratesTaint(taint) { + return true + } } return false } -// TaintToleratedByTolerations checks if taint is tolerated by any of the tolerations. -func TaintToleratedByTolerations(taint *Taint, tolerations []Toleration) bool { - tolerated := false - for i := range tolerations { - if TolerationToleratesTaint(&tolerations[i], taint) { - tolerated = true - break +type taintsFilterFunc func(*Taint) bool + +// TolerationsTolerateTaintsWithFilter checks if given tolerations tolerates +// all the interesting taints in given taint list. +func TolerationsTolerateTaintsWithFilter(tolerations []Toleration, taints []Taint, isInterestingTaint taintsFilterFunc) bool { + if len(taints) == 0 { + return true + } + + for i := range taints { + if isInterestingTaint != nil && !isInterestingTaint(&taints[i]) { + continue + } + + if !TolerationsTolerateTaint(tolerations, &taints[i]) { + return false } } - return tolerated + + return true +} + +// DeleteTaintsByKey removes all the taints that have the same key to given taintKey +func DeleteTaintsByKey(taints []Taint, taintKey string) ([]Taint, bool) { + newTaints := []Taint{} + deleted := false + for i := range taints { + if taintKey == taints[i].Key { + deleted = true + continue + } + newTaints = append(newTaints, taints[i]) + } + return newTaints, deleted +} + +func DeleteTaint(taints []Taint, taintToDelete *Taint) ([]Taint, bool) { + newTaints := []Taint{} + deleted := false + for i := range taints { + if taintToDelete.MatchTaint(taints[i]) { + deleted = true + continue + } + newTaints = append(newTaints, taints[i]) + } + return newTaints, deleted } // MatchTaint checks if the taint matches taintToMatch. Taints are unique by key:effect, diff --git a/pkg/api/v1/helpers_test.go b/pkg/api/v1/helpers_test.go index 37da086d94a..aca4da1ce46 100644 --- a/pkg/api/v1/helpers_test.go +++ b/pkg/api/v1/helpers_test.go @@ -280,6 +280,289 @@ func TestMatchTaint(t *testing.T) { } } +func TestTolerationToleratesTaint(t *testing.T) { + genTolerationSeconds := func(f int64) *int64 { + return &f + } + + testCases := []struct { + description string + toleration Toleration + taint Taint + expectTolerated bool + }{ + { + description: "toleration and taint have the same key and effect, and operator is Exists, and taint has no value, expect tolerated", + toleration: Toleration{ + Key: "foo", + Operator: TolerationOpExists, + Effect: TaintEffectNoSchedule, + }, + taint: Taint{ + Key: "foo", + Effect: TaintEffectNoSchedule, + }, + expectTolerated: true, + }, + { + description: "toleration and taint have the same key and effect, and operator is Exists, and taint has some value, expect tolerated", + toleration: Toleration{ + Key: "foo", + Operator: TolerationOpExists, + Effect: TaintEffectNoSchedule, + }, + taint: Taint{ + Key: "foo", + Value: "bar", + Effect: TaintEffectNoSchedule, + }, + expectTolerated: true, + }, + { + description: "toleration and taint have the same effect, toleration has empty key and operator is Exists, means match all taints, expect tolerated", + toleration: Toleration{ + Key: "", + Operator: TolerationOpExists, + Effect: TaintEffectNoSchedule, + }, + taint: Taint{ + Key: "foo", + Value: "bar", + Effect: TaintEffectNoSchedule, + }, + expectTolerated: true, + }, + { + description: "toleration and taint have the same key, effect and value, and operator is Equal, expect tolerated", + toleration: Toleration{ + Key: "foo", + Operator: TolerationOpEqual, + Value: "bar", + Effect: TaintEffectNoSchedule, + }, + taint: Taint{ + Key: "foo", + Value: "bar", + Effect: TaintEffectNoSchedule, + }, + expectTolerated: true, + }, + { + description: "toleration and taint have the same key and effect, but different values, and operator is Equal, expect not tolerated", + toleration: Toleration{ + Key: "foo", + Operator: TolerationOpEqual, + Value: "value1", + Effect: TaintEffectNoSchedule, + }, + taint: Taint{ + Key: "foo", + Value: "value2", + Effect: TaintEffectNoSchedule, + }, + expectTolerated: false, + }, + { + description: "toleration and taint have the same key and value, but different effects, and operator is Equal, expect not tolerated", + toleration: Toleration{ + Key: "foo", + Operator: TolerationOpEqual, + Value: "bar", + Effect: TaintEffectNoSchedule, + }, + taint: Taint{ + Key: "foo", + Value: "bar", + Effect: TaintEffectNoExecute, + }, + expectTolerated: false, + }, + { + description: "expect toleration with nil tolerationSeconds tolerates taint that is newly added", + toleration: Toleration{ + Key: "foo", + Operator: TolerationOpExists, + Effect: TaintEffectNoExecute, + }, + taint: Taint{ + Key: "foo", + Effect: TaintEffectNoExecute, + TimeAdded: metav1.Now(), + }, + expectTolerated: true, + }, + { + description: "forgiveness toleration has not timed out, expect tolerated", + toleration: Toleration{ + Key: "foo", + Operator: TolerationOpExists, + Effect: TaintEffectNoExecute, + TolerationSeconds: genTolerationSeconds(300), + }, + taint: Taint{ + Key: "foo", + Effect: TaintEffectNoExecute, + TimeAdded: metav1.Unix(metav1.Now().Unix()-100, 0), + }, + expectTolerated: true, + }, + { + description: "forgiveness toleration has timed out, expect not tolerated", + toleration: Toleration{ + Key: "foo", + Operator: TolerationOpExists, + Effect: TaintEffectNoExecute, + TolerationSeconds: genTolerationSeconds(300), + }, + taint: Taint{ + Key: "foo", + Effect: TaintEffectNoExecute, + TimeAdded: metav1.Unix(metav1.Now().Unix()-1000, 0), + }, + expectTolerated: false, + }, + { + description: "toleration with explicit forgiveness can't tolerate taint with no added time, expect not tolerated", + toleration: Toleration{ + Key: "foo", + Operator: TolerationOpExists, + Effect: TaintEffectNoExecute, + TolerationSeconds: genTolerationSeconds(300), + }, + taint: Taint{ + Key: "foo", + Effect: TaintEffectNoExecute, + }, + expectTolerated: false, + }, + } + for _, tc := range testCases { + 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()) + } + } +} + +func TestTolerationsTolerateTaintsWithFilter(t *testing.T) { + testCases := []struct { + description string + tolerations []Toleration + taints []Taint + isInterestingTaint taintsFilterFunc + expectTolerated bool + }{ + { + description: "empty tolerations tolerate empty taints", + tolerations: []Toleration{}, + taints: []Taint{}, + isInterestingTaint: func(t *Taint) bool { return true }, + expectTolerated: true, + }, + { + description: "non-empty tolerations tolerate empty taints", + tolerations: []Toleration{ + { + Key: "foo", + Operator: "Exists", + Effect: TaintEffectNoSchedule, + }, + }, + taints: []Taint{}, + isInterestingTaint: func(t *Taint) bool { return true }, + expectTolerated: true, + }, + { + description: "tolerations match all taints, expect tolerated", + tolerations: []Toleration{ + { + Key: "foo", + Operator: "Exists", + Effect: TaintEffectNoSchedule, + }, + }, + taints: []Taint{ + { + Key: "foo", + Effect: TaintEffectNoSchedule, + }, + }, + isInterestingTaint: func(t *Taint) bool { return true }, + expectTolerated: true, + }, + { + description: "tolerations don't match taints, but no taint is interested, expect tolerated", + tolerations: []Toleration{ + { + Key: "foo", + Operator: "Exists", + Effect: TaintEffectNoSchedule, + }, + }, + taints: []Taint{ + { + Key: "bar", + Effect: TaintEffectNoSchedule, + }, + }, + isInterestingTaint: func(t *Taint) bool { return false }, + expectTolerated: true, + }, + { + description: "no isInterestedTaint indicated, means all taints are interested, tolerations don't match taints, expect untolerated", + tolerations: []Toleration{ + { + Key: "foo", + Operator: "Exists", + Effect: TaintEffectNoSchedule, + }, + }, + taints: []Taint{ + { + Key: "bar", + Effect: TaintEffectNoSchedule, + }, + }, + isInterestingTaint: nil, + expectTolerated: false, + }, + { + description: "tolerations match interested taints, expect tolerated", + tolerations: []Toleration{ + { + Key: "foo", + Operator: "Exists", + Effect: TaintEffectNoExecute, + }, + }, + taints: []Taint{ + { + Key: "foo", + Effect: TaintEffectNoExecute, + }, + { + Key: "bar", + Effect: TaintEffectNoSchedule, + }, + }, + isInterestingTaint: func(t *Taint) bool { return t.Effect == TaintEffectNoExecute }, + expectTolerated: true, + }, + } + + for _, tc := range testCases { + if tc.expectTolerated != TolerationsTolerateTaintsWithFilter(tc.tolerations, tc.taints, tc.isInterestingTaint) { + filteredTaints := []Taint{} + for _, taint := range tc.taints { + if tc.isInterestingTaint != nil && !tc.isInterestingTaint(&taint) { + continue + } + filteredTaints = append(filteredTaints, taint) + } + t.Errorf("[%s] expect tolerations %+v tolerate filtered taints %+v in taints %+v", tc.description, tc.tolerations, filteredTaints, tc.taints) + } + } +} + func TestGetAvoidPodsFromNode(t *testing.T) { controllerFlag := true testCases := []struct { diff --git a/pkg/kubectl/cmd/taint.go b/pkg/kubectl/cmd/taint.go index 2c0e5eeb56f..535fe416b64 100644 --- a/pkg/kubectl/cmd/taint.go +++ b/pkg/kubectl/cmd/taint.go @@ -112,24 +112,6 @@ func NewCmdTaint(f cmdutil.Factory, out io.Writer) *cobra.Command { return cmd } -func deleteTaint(taints []v1.Taint, taintToDelete v1.Taint) ([]v1.Taint, error) { - newTaints := []v1.Taint{} - found := false - for _, taint := range taints { - if taint.Key == taintToDelete.Key && - (len(taintToDelete.Effect) == 0 || taint.Effect == taintToDelete.Effect) { - found = true - continue - } - newTaints = append(newTaints, taint) - } - - if !found { - return nil, fmt.Errorf("taint key=\"%s\" and effect=\"%s\" not found.", taintToDelete.Key, taintToDelete.Effect) - } - return newTaints, nil -} - // reorganizeTaints returns the updated set of taints, taking into account old taints that were not updated, // old taints that were updated, old taints that were deleted, and new taints. func reorganizeTaints(accessor metav1.Object, overwrite bool, taintsToAdd []v1.Taint, taintsToRemove []v1.Taint) ([]v1.Taint, error) { @@ -160,9 +142,14 @@ func reorganizeTaints(accessor metav1.Object, overwrite bool, taintsToAdd []v1.T allErrs := []error{} for _, taintToRemove := range taintsToRemove { - newTaints, err = deleteTaint(newTaints, taintToRemove) - if err != nil { - allErrs = append(allErrs, err) + removed := false + if len(taintToRemove.Effect) > 0 { + newTaints, removed = v1.DeleteTaint(newTaints, &taintToRemove) + } else { + newTaints, removed = v1.DeleteTaintsByKey(newTaints, taintToRemove.Key) + } + if !removed { + allErrs = append(allErrs, fmt.Errorf("taint %q not found", taintToRemove.ToString())) } } return newTaints, utilerrors.NewAggregate(allErrs) diff --git a/plugin/pkg/scheduler/algorithm/predicates/predicates.go b/plugin/pkg/scheduler/algorithm/predicates/predicates.go index 501192881d7..00ad5a788c6 100644 --- a/plugin/pkg/scheduler/algorithm/predicates/predicates.go +++ b/plugin/pkg/scheduler/algorithm/predicates/predicates.go @@ -1158,33 +1158,15 @@ func PodToleratesNodeTaints(pod *v1.Pod, meta interface{}, nodeInfo *schedulerca return false, nil, err } - if tolerationsToleratesTaints(tolerations, taints) { + if v1.TolerationsTolerateTaintsWithFilter(tolerations, taints, func(t *v1.Taint) bool { + // PodToleratesNodeTaints is only interested in NoSchedule taints. + return t.Effect == v1.TaintEffectNoSchedule + }) { return true, nil, nil } return false, []algorithm.PredicateFailureReason{ErrTaintsTolerationsNotMatch}, nil } -func tolerationsToleratesTaints(tolerations []v1.Toleration, taints []v1.Taint) bool { - // If the taint list is nil/empty, it is tolerated by all tolerations by default. - if len(taints) == 0 { - return true - } - - for i := range taints { - taint := &taints[i] - // skip taints that have effect PreferNoSchedule, since it is for priorities - if taint.Effect == v1.TaintEffectPreferNoSchedule { - continue - } - - if len(tolerations) == 0 || !v1.TaintToleratedByTolerations(taint, tolerations) { - return false - } - } - - return true -} - // Determine if a pod is scheduled with best-effort QoS func isPodBestEffort(pod *v1.Pod) bool { return qos.GetPodQOS(pod) == v1.PodQOSBestEffort diff --git a/plugin/pkg/scheduler/algorithm/priorities/taint_toleration.go b/plugin/pkg/scheduler/algorithm/priorities/taint_toleration.go index d08c12b00f9..bebc30564ec 100644 --- a/plugin/pkg/scheduler/algorithm/priorities/taint_toleration.go +++ b/plugin/pkg/scheduler/algorithm/priorities/taint_toleration.go @@ -34,7 +34,7 @@ func countIntolerableTaintsPreferNoSchedule(taints []v1.Taint, tolerations []v1. continue } - if !v1.TaintToleratedByTolerations(taint, tolerations) { + if !v1.TolerationsTolerateTaint(tolerations, taint) { intolerableTaints++ } } diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index 888bc1c48c1..c2cf3070cb5 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -2517,23 +2517,6 @@ func ExpectNodeHasTaint(c clientset.Interface, nodeName string, taint v1.Taint) } } -func deleteTaint(oldTaints []v1.Taint, taintToDelete v1.Taint) ([]v1.Taint, error) { - newTaints := []v1.Taint{} - found := false - for _, oldTaint := range oldTaints { - if oldTaint.MatchTaint(taintToDelete) { - found = true - continue - } - newTaints = append(newTaints, taintToDelete) - } - - if !found { - return nil, fmt.Errorf("taint %s not found.", taintToDelete.ToString()) - } - return newTaints, nil -} - // RemoveTaintOffNode is for cleaning up taints temporarily added to node, // won't fail if target taint doesn't exist or has been removed. func RemoveTaintOffNode(c clientset.Interface, nodeName string, taint v1.Taint) { @@ -2552,8 +2535,7 @@ func RemoveTaintOffNode(c clientset.Interface, nodeName string, taint v1.Taint) return } - newTaints, err := deleteTaint(nodeTaints, taint) - ExpectNoError(err) + newTaints, _ := v1.DeleteTaint(nodeTaints, &taint) if len(newTaints) == 0 { delete(node.Annotations, v1.TaintsAnnotationKey) } else { From 6a2ef1646b4b200ad580d9f96883b8fcb27b1bb1 Mon Sep 17 00:00:00 2001 From: Kevin Date: Tue, 24 Jan 2017 01:33:33 +0800 Subject: [PATCH 2/4] address review comments --- pkg/api/v1/helpers.go | 22 +++++++++++----- pkg/api/v1/helpers_test.go | 52 +++++++++++++++++++------------------- 2 files changed, 41 insertions(+), 33 deletions(-) diff --git a/pkg/api/v1/helpers.go b/pkg/api/v1/helpers.go index 7624223431f..da74e382152 100644 --- a/pkg/api/v1/helpers.go +++ b/pkg/api/v1/helpers.go @@ -309,20 +309,28 @@ func GetNodeTaints(node *Node) ([]Taint, error) { } // ToleratesTaint checks if the toleration tolerates the taint. +// The matching follows the rules below: +// (1) Empty toleration.effect means to match all taint effects, +// otherwise taint effect must equal to toleration.effect. +// (2) If toleration.operator is 'Exists', it means to match all taint values. +// (3) Empty toleration.key means to match all taint keys. +// If toleration.key is empty, toleration.operator must be 'Exists'; +// this combination means to match all taint values and all taint keys. +// (4) Nil toleration.tolerationSeconds means to match taints with +// any value of taint.timeAdded. +// (5) Non-nil positive toleration.tolerationSeconds means to +// match the taint for only a duration that starts at taint.timeAdded. func (t *Toleration) ToleratesTaint(taint *Taint) bool { - // empty toleration effect means match all taint effects if len(t.Effect) > 0 && t.Effect != taint.Effect { return false } - // empty toleration key means match all taint keys if len(t.Key) > 0 && t.Key != taint.Key { return false } - // nil TolerationSeconds means tolerate the taint forever if t.TolerationSeconds != nil { - // taint with no added time indicated can only be tolerated + // taint with no timeAdded indicated can only be tolerated // by toleration with no tolerationSeconds. if taint.TimeAdded.IsZero() { return false @@ -359,14 +367,14 @@ func TolerationsTolerateTaint(tolerations []Toleration, taint *Taint) bool { type taintsFilterFunc func(*Taint) bool // TolerationsTolerateTaintsWithFilter checks if given tolerations tolerates -// all the interesting taints in given taint list. -func TolerationsTolerateTaintsWithFilter(tolerations []Toleration, taints []Taint, isInterestingTaint taintsFilterFunc) bool { +// all the taints that apply to the filter in given taint list. +func TolerationsTolerateTaintsWithFilter(tolerations []Toleration, taints []Taint, applyFilter taintsFilterFunc) bool { if len(taints) == 0 { return true } for i := range taints { - if isInterestingTaint != nil && !isInterestingTaint(&taints[i]) { + if applyFilter != nil && !applyFilter(&taints[i]) { continue } diff --git a/pkg/api/v1/helpers_test.go b/pkg/api/v1/helpers_test.go index aca4da1ce46..7ad6d173a93 100644 --- a/pkg/api/v1/helpers_test.go +++ b/pkg/api/v1/helpers_test.go @@ -445,18 +445,18 @@ func TestTolerationToleratesTaint(t *testing.T) { func TestTolerationsTolerateTaintsWithFilter(t *testing.T) { testCases := []struct { - description string - tolerations []Toleration - taints []Taint - isInterestingTaint taintsFilterFunc - expectTolerated bool + description string + tolerations []Toleration + taints []Taint + applyFilter taintsFilterFunc + expectTolerated bool }{ { - description: "empty tolerations tolerate empty taints", - tolerations: []Toleration{}, - taints: []Taint{}, - isInterestingTaint: func(t *Taint) bool { return true }, - expectTolerated: true, + description: "empty tolerations tolerate empty taints", + tolerations: []Toleration{}, + taints: []Taint{}, + applyFilter: func(t *Taint) bool { return true }, + expectTolerated: true, }, { description: "non-empty tolerations tolerate empty taints", @@ -467,9 +467,9 @@ func TestTolerationsTolerateTaintsWithFilter(t *testing.T) { Effect: TaintEffectNoSchedule, }, }, - taints: []Taint{}, - isInterestingTaint: func(t *Taint) bool { return true }, - expectTolerated: true, + taints: []Taint{}, + applyFilter: func(t *Taint) bool { return true }, + expectTolerated: true, }, { description: "tolerations match all taints, expect tolerated", @@ -486,11 +486,11 @@ func TestTolerationsTolerateTaintsWithFilter(t *testing.T) { Effect: TaintEffectNoSchedule, }, }, - isInterestingTaint: func(t *Taint) bool { return true }, - expectTolerated: true, + applyFilter: func(t *Taint) bool { return true }, + expectTolerated: true, }, { - description: "tolerations don't match taints, but no taint is interested, expect tolerated", + description: "tolerations don't match taints, but no taints apply to the filter, expect tolerated", tolerations: []Toleration{ { Key: "foo", @@ -504,11 +504,11 @@ func TestTolerationsTolerateTaintsWithFilter(t *testing.T) { Effect: TaintEffectNoSchedule, }, }, - isInterestingTaint: func(t *Taint) bool { return false }, - expectTolerated: true, + applyFilter: func(t *Taint) bool { return false }, + expectTolerated: true, }, { - description: "no isInterestedTaint indicated, means all taints are interested, tolerations don't match taints, expect untolerated", + description: "no filterFunc indicated, means all taints apply to the filter, tolerations don't match taints, expect untolerated", tolerations: []Toleration{ { Key: "foo", @@ -522,11 +522,11 @@ func TestTolerationsTolerateTaintsWithFilter(t *testing.T) { Effect: TaintEffectNoSchedule, }, }, - isInterestingTaint: nil, - expectTolerated: false, + applyFilter: nil, + expectTolerated: false, }, { - description: "tolerations match interested taints, expect tolerated", + description: "tolerations match taints, expect tolerated", tolerations: []Toleration{ { Key: "foo", @@ -544,16 +544,16 @@ func TestTolerationsTolerateTaintsWithFilter(t *testing.T) { Effect: TaintEffectNoSchedule, }, }, - isInterestingTaint: func(t *Taint) bool { return t.Effect == TaintEffectNoExecute }, - expectTolerated: true, + applyFilter: func(t *Taint) bool { return t.Effect == TaintEffectNoExecute }, + expectTolerated: true, }, } for _, tc := range testCases { - if tc.expectTolerated != TolerationsTolerateTaintsWithFilter(tc.tolerations, tc.taints, tc.isInterestingTaint) { + if tc.expectTolerated != TolerationsTolerateTaintsWithFilter(tc.tolerations, tc.taints, tc.applyFilter) { filteredTaints := []Taint{} for _, taint := range tc.taints { - if tc.isInterestingTaint != nil && !tc.isInterestingTaint(&taint) { + if tc.applyFilter != nil && !tc.applyFilter(&taint) { continue } filteredTaints = append(filteredTaints, taint) From 3c550c32fbcf66a6a5619e96587dcf89bfb4a8b9 Mon Sep 17 00:00:00 2001 From: Kevin Date: Sun, 29 Jan 2017 00:39:35 +0800 Subject: [PATCH 3/4] address review comments 2 --- pkg/api/v1/helpers.go | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/pkg/api/v1/helpers.go b/pkg/api/v1/helpers.go index da74e382152..3d1b11ee4ff 100644 --- a/pkg/api/v1/helpers.go +++ b/pkg/api/v1/helpers.go @@ -316,10 +316,10 @@ func GetNodeTaints(node *Node) ([]Taint, error) { // (3) Empty toleration.key means to match all taint keys. // If toleration.key is empty, toleration.operator must be 'Exists'; // this combination means to match all taint values and all taint keys. -// (4) Nil toleration.tolerationSeconds means to match taints with -// any value of taint.timeAdded. +// (4) Nil toleration.tolerationSeconds means to tolerate the taint forever. // (5) Non-nil positive toleration.tolerationSeconds means to -// match the taint for only a duration that starts at taint.timeAdded. +// match the taint for only a duration since the taint was observed +// by the TaintManager. func (t *Toleration) ToleratesTaint(taint *Taint) bool { if len(t.Effect) > 0 && t.Effect != taint.Effect { return false @@ -329,17 +329,9 @@ func (t *Toleration) ToleratesTaint(taint *Taint) bool { return false } - if t.TolerationSeconds != nil { - // taint with no timeAdded indicated can only be tolerated - // by toleration with no tolerationSeconds. - if taint.TimeAdded.IsZero() { - return false - } - - // TODO: need to take time skew into consideration, make sure toleration won't become out of age ealier than expected. - if metav1.Now().After(taint.TimeAdded.Add(time.Second * time.Duration(*t.TolerationSeconds))) { - return false - } + // TODO: need to take time skew into consideration, make sure toleration won't become out of age ealier than expected. + if t.TolerationSeconds != nil && metav1.Now().After(taint.TimeAdded.Add(time.Second*time.Duration(*t.TolerationSeconds))) { + return false } // TODO: Use proper defaulting when Toleration becomes a field of PodSpec From b410f3f4c27e2ff0c287ec8dcb94de1622163a09 Mon Sep 17 00:00:00 2001 From: Kevin Date: Tue, 31 Jan 2017 21:20:42 +0800 Subject: [PATCH 4/4] address review comments 3 --- pkg/api/v1/helpers.go | 12 +------- pkg/api/v1/helpers_test.go | 61 -------------------------------------- 2 files changed, 1 insertion(+), 72 deletions(-) diff --git a/pkg/api/v1/helpers.go b/pkg/api/v1/helpers.go index 3d1b11ee4ff..edf7de6043f 100644 --- a/pkg/api/v1/helpers.go +++ b/pkg/api/v1/helpers.go @@ -20,9 +20,7 @@ import ( "encoding/json" "fmt" "strings" - "time" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/types" @@ -316,10 +314,6 @@ func GetNodeTaints(node *Node) ([]Taint, error) { // (3) Empty toleration.key means to match all taint keys. // If toleration.key is empty, toleration.operator must be 'Exists'; // this combination means to match all taint values and all taint keys. -// (4) Nil toleration.tolerationSeconds means to tolerate the taint forever. -// (5) Non-nil positive toleration.tolerationSeconds means to -// match the taint for only a duration since the taint was observed -// by the TaintManager. func (t *Toleration) ToleratesTaint(taint *Taint) bool { if len(t.Effect) > 0 && t.Effect != taint.Effect { return false @@ -329,11 +323,6 @@ func (t *Toleration) ToleratesTaint(taint *Taint) bool { return false } - // TODO: need to take time skew into consideration, make sure toleration won't become out of age ealier than expected. - if t.TolerationSeconds != nil && metav1.Now().After(taint.TimeAdded.Add(time.Second*time.Duration(*t.TolerationSeconds))) { - return false - } - // TODO: Use proper defaulting when Toleration becomes a field of PodSpec switch t.Operator { // empty operator means Equal @@ -392,6 +381,7 @@ func DeleteTaintsByKey(taints []Taint, taintKey string) ([]Taint, bool) { return newTaints, deleted } +// DeleteTaint removes all the the taints that have the same key and effect to given taintToDelete. func DeleteTaint(taints []Taint, taintToDelete *Taint) ([]Taint, bool) { newTaints := []Taint{} deleted := false diff --git a/pkg/api/v1/helpers_test.go b/pkg/api/v1/helpers_test.go index 7ad6d173a93..62acc6285b7 100644 --- a/pkg/api/v1/helpers_test.go +++ b/pkg/api/v1/helpers_test.go @@ -281,9 +281,6 @@ func TestMatchTaint(t *testing.T) { } func TestTolerationToleratesTaint(t *testing.T) { - genTolerationSeconds := func(f int64) *int64 { - return &f - } testCases := []struct { description string @@ -377,64 +374,6 @@ func TestTolerationToleratesTaint(t *testing.T) { }, expectTolerated: false, }, - { - description: "expect toleration with nil tolerationSeconds tolerates taint that is newly added", - toleration: Toleration{ - Key: "foo", - Operator: TolerationOpExists, - Effect: TaintEffectNoExecute, - }, - taint: Taint{ - Key: "foo", - Effect: TaintEffectNoExecute, - TimeAdded: metav1.Now(), - }, - expectTolerated: true, - }, - { - description: "forgiveness toleration has not timed out, expect tolerated", - toleration: Toleration{ - Key: "foo", - Operator: TolerationOpExists, - Effect: TaintEffectNoExecute, - TolerationSeconds: genTolerationSeconds(300), - }, - taint: Taint{ - Key: "foo", - Effect: TaintEffectNoExecute, - TimeAdded: metav1.Unix(metav1.Now().Unix()-100, 0), - }, - expectTolerated: true, - }, - { - description: "forgiveness toleration has timed out, expect not tolerated", - toleration: Toleration{ - Key: "foo", - Operator: TolerationOpExists, - Effect: TaintEffectNoExecute, - TolerationSeconds: genTolerationSeconds(300), - }, - taint: Taint{ - Key: "foo", - Effect: TaintEffectNoExecute, - TimeAdded: metav1.Unix(metav1.Now().Unix()-1000, 0), - }, - expectTolerated: false, - }, - { - description: "toleration with explicit forgiveness can't tolerate taint with no added time, expect not tolerated", - toleration: Toleration{ - Key: "foo", - Operator: TolerationOpExists, - Effect: TaintEffectNoExecute, - TolerationSeconds: genTolerationSeconds(300), - }, - taint: Taint{ - Key: "foo", - Effect: TaintEffectNoExecute, - }, - expectTolerated: false, - }, } for _, tc := range testCases { if tolerated := tc.toleration.ToleratesTaint(&tc.taint); tc.expectTolerated != tolerated {