forgiveness library changes

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,

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 {

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)

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

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++
 		}
 	}

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 {