Merge pull request #32378 from kevin-wangzefeng/update-taints-e2e

Automatic merge from submit-queue

update taints e2e, restrict taints operation with key, effect

Since taints are now unique by key, effect on a node, this PR is to restrict existing taints adding/removing/updating operations in taints e2e.
Also fixes https://github.com/kubernetes/kubernetes/issues/31066#issuecomment-242870101
Related prior Issue/PR #29362 and #30590

pkg/api/helpers.go

@@ -526,6 +526,20 @@ func TaintToleratedByTolerations(taint *Taint, tolerations []Toleration) bool {
 	return tolerated
 }
 
+// MatchTaint checks if the taint matches taintToMatch. Taints are unique by key:effect,
+// if the two taints have same key:effect, regard as they match.
+func (t *Taint) MatchTaint(taintToMatch Taint) bool {
+	return t.Key == taintToMatch.Key && t.Effect == taintToMatch.Effect
+}
+
+// taint.ToString() converts taint struct to string in format key=value:effect or key:effect.
+func (t *Taint) ToString() string {
+	if len(t.Value) == 0 {
+		return fmt.Sprintf("%v:%v", t.Key, t.Effect)
+	}
+	return fmt.Sprintf("%v=%v:%v", t.Key, t.Value, t.Effect)
+}
+
 func GetAvoidPodsFromNodeAnnotations(annotations map[string]string) (AvoidPods, error) {
 	var avoidPods AvoidPods
 	if len(annotations) > 0 && annotations[PreferAvoidPodsAnnotationKey] != "" {

pkg/api/helpers_test.go

@@ -296,6 +296,107 @@ func TestGetAffinityFromPod(t *testing.T) {
 	}
 }
 
+func TestTaintToString(t *testing.T) {
+	testCases := []struct {
+		taint          *Taint
+		expectedString string
+	}{
+		{
+			taint: &Taint{
+				Key:    "foo",
+				Value:  "bar",
+				Effect: TaintEffectNoSchedule,
+			},
+			expectedString: "foo=bar:NoSchedule",
+		},
+		{
+			taint: &Taint{
+				Key:    "foo",
+				Effect: TaintEffectNoSchedule,
+			},
+			expectedString: "foo:NoSchedule",
+		},
+	}
+
+	for i, tc := range testCases {
+		if tc.expectedString != tc.taint.ToString() {
+			t.Errorf("[%v] expected taint %v converted to %s, got %s", i, tc.taint, tc.expectedString, tc.taint.ToString())
+		}
+	}
+}
+
+func TestMatchTaint(t *testing.T) {
+	testCases := []struct {
+		description  string
+		taint        *Taint
+		taintToMatch Taint
+		expectMatch  bool
+	}{
+		{
+			description: "two taints with the same key,value,effect should match",
+			taint: &Taint{
+				Key:    "foo",
+				Value:  "bar",
+				Effect: TaintEffectNoSchedule,
+			},
+			taintToMatch: Taint{
+				Key:    "foo",
+				Value:  "bar",
+				Effect: TaintEffectNoSchedule,
+			},
+			expectMatch: true,
+		},
+		{
+			description: "two taints with the same key,effect but different value should match",
+			taint: &Taint{
+				Key:    "foo",
+				Value:  "bar",
+				Effect: TaintEffectNoSchedule,
+			},
+			taintToMatch: Taint{
+				Key:    "foo",
+				Value:  "different-value",
+				Effect: TaintEffectNoSchedule,
+			},
+			expectMatch: true,
+		},
+		{
+			description: "two taints with the different key cannot match",
+			taint: &Taint{
+				Key:    "foo",
+				Value:  "bar",
+				Effect: TaintEffectNoSchedule,
+			},
+			taintToMatch: Taint{
+				Key:    "different-key",
+				Value:  "bar",
+				Effect: TaintEffectNoSchedule,
+			},
+			expectMatch: false,
+		},
+		{
+			description: "two taints with the different effect cannot match",
+			taint: &Taint{
+				Key:    "foo",
+				Value:  "bar",
+				Effect: TaintEffectNoSchedule,
+			},
+			taintToMatch: Taint{
+				Key:    "foo",
+				Value:  "bar",
+				Effect: TaintEffectPreferNoSchedule,
+			},
+			expectMatch: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		if tc.expectMatch != tc.taint.MatchTaint(tc.taintToMatch) {
+			t.Errorf("[%s] expect taint %s match taint %s", tc.description, tc.taint.ToString(), tc.taintToMatch.ToString())
+		}
+	}
+}
+
 func TestGetAvoidPodsFromNode(t *testing.T) {
 	controllerFlag := true
 	testCases := []struct {

pkg/kubectl/cmd/taint.go

@@ -144,7 +144,7 @@ func reorganizeTaints(accessor meta.Object, overwrite bool, taintsToAdd []api.Ta
 	for _, oldTaint := range oldTaints {
 		existsInNew := false
 		for _, taint := range newTaints {
-			if taint.Key == oldTaint.Key && taint.Effect == oldTaint.Effect {
+			if taint.MatchTaint(oldTaint) {
 				existsInNew = true
 				break
 			}

pkg/kubectl/describe.go

@@ -2608,23 +2608,19 @@ func printTaintsMultilineWithIndent(out io.Writer, initialIndent, title, innerIn
 	// to print taints in the sorted order
 	keys := make([]string, 0, len(taints))
 	for _, taint := range taints {
-		keys = append(keys, taint.Key)
+		keys = append(keys, string(taint.Effect)+","+taint.Key)
 	}
 	sort.Strings(keys)
 
-	effects := []api.TaintEffect{api.TaintEffectNoSchedule, api.TaintEffectPreferNoSchedule}
-
 	for i, key := range keys {
-		for _, effect := range effects {
+		for _, taint := range taints {
-			for _, taint := range taints {
+			if string(taint.Effect)+","+taint.Key == key {
-				if taint.Key == key && taint.Effect == effect {
+				if i != 0 {
-					if i != 0 {
+					fmt.Fprint(out, initialIndent)
-						fmt.Fprint(out, initialIndent)
+					fmt.Fprint(out, innerIndent)
-						fmt.Fprint(out, innerIndent)
-					}
-					fmt.Fprintf(out, "%s=%s:%s\n", taint.Key, taint.Value, taint.Effect)
-					i++
 				}
+				fmt.Fprintf(out, "%s\n", taint.ToString())
+				i++
 			}
 		}
 	}

test/e2e/framework/util.go

@@ -3059,7 +3059,7 @@ func AddOrUpdateTaintOnNode(c *client.Client, nodeName string, taint api.Taint)
 		var newTaints []api.Taint
 		updated := false
 		for _, existingTaint := range nodeTaints {
-			if existingTaint.Key == taint.Key {
+			if taint.MatchTaint(existingTaint) {
 				newTaints = append(newTaints, taint)
 				updated = true
 				continue
@@ -3093,49 +3093,49 @@ func AddOrUpdateTaintOnNode(c *client.Client, nodeName string, taint api.Taint)
 	}
 }
 
-func taintExists(taints []api.Taint, taintKey string) bool {
+func taintExists(taints []api.Taint, taintToFind api.Taint) bool {
 	for _, taint := range taints {
-		if taint.Key == taintKey {
+		if taint.MatchTaint(taintToFind) {
 			return true
 		}
 	}
 	return false
 }
 
-func ExpectNodeHasTaint(c *client.Client, nodeName string, taintKey string) {
+func ExpectNodeHasTaint(c *client.Client, nodeName string, taint api.Taint) {
-	By("verifying the node has the taint " + taintKey)
+	By("verifying the node has the taint " + taint.ToString())
 	node, err := c.Nodes().Get(nodeName)
 	ExpectNoError(err)
 
 	nodeTaints, err := api.GetTaintsFromNodeAnnotations(node.Annotations)
 	ExpectNoError(err)
 
-	if len(nodeTaints) == 0 || !taintExists(nodeTaints, taintKey) {
+	if len(nodeTaints) == 0 || !taintExists(nodeTaints, taint) {
-		Failf("Failed to find taint %s on node %s", taintKey, nodeName)
+		Failf("Failed to find taint %s on node %s", taint.ToString(), nodeName)
 	}
 }
 
-func deleteTaintByKey(taints []api.Taint, taintKey string) ([]api.Taint, error) {
+func deleteTaint(oldTaints []api.Taint, taintToDelete api.Taint) ([]api.Taint, error) {
 	newTaints := []api.Taint{}
 	found := false
-	for _, taint := range taints {
+	for _, oldTaint := range oldTaints {
-		if taint.Key == taintKey {
+		if oldTaint.MatchTaint(taintToDelete) {
 			found = true
 			continue
 		}
-		newTaints = append(newTaints, taint)
+		newTaints = append(newTaints, taintToDelete)
 	}
 
 	if !found {
-		return nil, fmt.Errorf("taint key=\"%s\" not found.", taintKey)
+		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 *client.Client, nodeName string, taintKey string) {
+func RemoveTaintOffNode(c *client.Client, nodeName string, taint api.Taint) {
-	By("removing the taint " + taintKey + " off the node " + nodeName)
+	By("removing the taint " + taint.ToString() + " off the node " + nodeName)
 	for attempt := 0; attempt < UpdateRetries; attempt++ {
 		node, err := c.Nodes().Get(nodeName)
 		ExpectNoError(err)
@@ -3146,11 +3146,11 @@ func RemoveTaintOffNode(c *client.Client, nodeName string, taintKey string) {
 			return
 		}
 
-		if !taintExists(nodeTaints, taintKey) {
+		if !taintExists(nodeTaints, taint) {
 			return
 		}
 
-		newTaints, err := deleteTaintByKey(nodeTaints, taintKey)
+		newTaints, err := deleteTaint(nodeTaints, taint)
 		ExpectNoError(err)
 
 		taintsData, err := json.Marshal(newTaints)
@@ -3161,7 +3161,7 @@ func RemoveTaintOffNode(c *client.Client, nodeName string, taintKey string) {
 			if !apierrs.IsConflict(err) {
 				ExpectNoError(err)
 			} else {
-				Logf("Conflict when trying to add/update taint %v to %v", taintKey, nodeName)
+				Logf("Conflict when trying to add/update taint %s to node %v", taint.ToString(), nodeName)
 			}
 		} else {
 			break
@@ -3171,11 +3171,11 @@ func RemoveTaintOffNode(c *client.Client, nodeName string, taintKey string) {
 
 	nodeUpdated, err := c.Nodes().Get(nodeName)
 	ExpectNoError(err)
-	By("verifying the node doesn't have the taint " + taintKey)
+	By("verifying the node doesn't have the taint " + taint.ToString())
 	taintsGot, err := api.GetTaintsFromNodeAnnotations(nodeUpdated.Annotations)
 	ExpectNoError(err)
-	if taintExists(taintsGot, taintKey) {
+	if taintExists(taintsGot, taint) {
-		Failf("Failed removing taint " + taintKey + " of the node " + nodeName)
+		Failf("Failed removing taint " + taint.ToString() + " of the node " + nodeName)
 	}
 }
 

test/e2e/kubectl.go

@@ -1232,76 +1232,83 @@ var _ = framework.KubeDescribe("Kubectl client", func() {
 
 	framework.KubeDescribe("Kubectl taint", func() {
 		It("should update the taint on a node", func() {
-			taintName := fmt.Sprintf("kubernetes.io/e2e-taint-key-%s", string(uuid.NewUUID()))
+			testTaint := api.Taint{
-			taintValue := "testing-taint-value"
+				Key:    fmt.Sprintf("kubernetes.io/e2e-taint-key-%s", string(uuid.NewUUID())),
-			taintEffect := fmt.Sprintf("%s", api.TaintEffectNoSchedule)
+				Value:  "testing-taint-value",
+				Effect: api.TaintEffectNoSchedule,
+			}
 
 			nodes, err := c.Nodes().List(api.ListOptions{})
 			Expect(err).NotTo(HaveOccurred())
 			node := nodes.Items[0]
 			nodeName := node.Name
 
-			By("adding the taint " + taintName + " with value " + taintValue + " and taint effect " + taintEffect + " to a node")
+			By("adding the taint " + testTaint.ToString() + " to a node")
-			framework.RunKubectlOrDie("taint", "nodes", nodeName, taintName+"="+taintValue+":"+taintEffect)
+			framework.RunKubectlOrDie("taint", "nodes", nodeName, testTaint.ToString())
-			By("verifying the node has the taint " + taintName + " with the value " + taintValue)
+			By("verifying the node has the taint " + testTaint.ToString())
 			output := framework.RunKubectlOrDie("describe", "node", nodeName)
 			requiredStrings := [][]string{
 				{"Name:", nodeName},
 				{"Taints:"},
-				{taintName + "=" + taintValue + ":" + taintEffect},
+				{testTaint.ToString()},
 			}
 			checkOutput(output, requiredStrings)
 
-			By("removing the taint " + taintName + " of a node")
+			By("removing the taint " + testTaint.ToString() + " of a node")
-			framework.RunKubectlOrDie("taint", "nodes", nodeName, taintName+":"+taintEffect+"-")
+			framework.RunKubectlOrDie("taint", "nodes", nodeName, testTaint.Key+":"+string(testTaint.Effect)+"-")
-			By("verifying the node doesn't have the taint " + taintName)
+			By("verifying the node doesn't have the taint " + testTaint.Key)
 			output = framework.RunKubectlOrDie("describe", "node", nodeName)
-			if strings.Contains(output, taintName) {
+			if strings.Contains(output, testTaint.Key) {
-				framework.Failf("Failed removing taint " + taintName + " of the node " + nodeName)
+				framework.Failf("Failed removing taint " + testTaint.Key + " of the node " + nodeName)
 			}
 
 		})
 
 		It("should remove all the taints with the same key off a node", func() {
-			taintName := fmt.Sprintf("kubernetes.io/e2e-taint-key-%s", string(uuid.NewUUID()))
+			testTaint := api.Taint{
-			taintValue := "testing-taint-value"
+				Key:    fmt.Sprintf("kubernetes.io/e2e-taint-key-%s", string(uuid.NewUUID())),
-			taintEffect := fmt.Sprintf("%s", api.TaintEffectNoSchedule)
+				Value:  "testing-taint-value",
+				Effect: api.TaintEffectNoSchedule,
+			}
 
 			nodes, err := c.Nodes().List(api.ListOptions{})
 			Expect(err).NotTo(HaveOccurred())
 			node := nodes.Items[0]
 			nodeName := node.Name
 
-			By("adding the taint " + taintName + " with value " + taintValue + " and taint effect " + taintEffect + " to a node")
+			By("adding the taint " + testTaint.ToString() + " to a node")
-			framework.RunKubectlOrDie("taint", "nodes", nodeName, taintName+"="+taintValue+":"+taintEffect)
+			framework.RunKubectlOrDie("taint", "nodes", nodeName, testTaint.ToString())
-			By("verifying the node has the taint " + taintName + " with the value " + taintValue)
+			By("verifying the node has the taint " + testTaint.ToString())
 			output := framework.RunKubectlOrDie("describe", "node", nodeName)
 			requiredStrings := [][]string{
 				{"Name:", nodeName},
 				{"Taints:"},
-				{taintName + "=" + taintValue + ":" + taintEffect},
+				{testTaint.ToString()},
 			}
 			checkOutput(output, requiredStrings)
 
-			newTaintValue := "another-testing-taint-value"
+			newTestTaint := api.Taint{
-			newTaintEffect := fmt.Sprintf("%s", api.TaintEffectPreferNoSchedule)
+				Key:    testTaint.Key,
-			By("adding another taint " + taintName + " with value " + newTaintValue + " and taint effect " + newTaintEffect + " to the node")
+				Value:  "another-testing-taint-value",
-			framework.RunKubectlOrDie("taint", "nodes", nodeName, taintName+"="+newTaintValue+":"+newTaintEffect)
+				Effect: api.TaintEffectPreferNoSchedule,
-			By("verifying the node has the taint " + taintName + " with the value " + newTaintValue)
+			}
+			By("adding another taint " + newTestTaint.ToString() + " to the node")
+			framework.RunKubectlOrDie("taint", "nodes", nodeName, newTestTaint.ToString())
+			By("verifying the node has the taint " + newTestTaint.ToString())
 			output = framework.RunKubectlOrDie("describe", "node", nodeName)
 			requiredStrings = [][]string{
 				{"Name:", nodeName},
 				{"Taints:"},
-				{taintName + "=" + newTaintValue + ":" + newTaintEffect},
+				{newTestTaint.ToString()},
 			}
 			checkOutput(output, requiredStrings)
 
-			By("removing the taint " + taintName + " of a node")
+			By("removing all taints that have the same key " + testTaint.Key + " of the node")
-			framework.RunKubectlOrDie("taint", "nodes", nodeName, taintName+"-")
+			framework.RunKubectlOrDie("taint", "nodes", nodeName, testTaint.Key+"-")
-			By("verifying the node doesn't have the taints " + taintName)
+			By("verifying the node doesn't have the taints that have the same key " + testTaint.Key)
 			output = framework.RunKubectlOrDie("describe", "node", nodeName)
-			if strings.Contains(output, taintName) {
+			if strings.Contains(output, testTaint.Key) {
-				framework.Failf("Failed removing taint " + taintName + " of the node " + nodeName)
+				framework.Failf("Failed removing taints " + testTaint.Key + " of the node " + nodeName)
 			}
 		})
 	})

test/e2e/scheduler_predicates.go

@@ -1328,12 +1328,14 @@ var _ = framework.KubeDescribe("SchedulerPredicates [Serial]", func() {
 		framework.ExpectNoError(err)
 
 		By("Trying to apply a random taint on the found node.")
-		taintName := fmt.Sprintf("kubernetes.io/e2e-taint-key-%s", string(uuid.NewUUID()))
+		testTaint := api.Taint{
-		taintValue := "testing-taint-value"
+			Key:    fmt.Sprintf("kubernetes.io/e2e-taint-key-%s", string(uuid.NewUUID())),
-		taintEffect := api.TaintEffectNoSchedule
+			Value:  "testing-taint-value",
-		framework.AddOrUpdateTaintOnNode(c, nodeName, api.Taint{Key: taintName, Value: taintValue, Effect: taintEffect})
+			Effect: api.TaintEffectNoSchedule,
-		framework.ExpectNodeHasTaint(c, nodeName, taintName)
+		}
-		defer framework.RemoveTaintOffNode(c, nodeName, taintName)
+		framework.AddOrUpdateTaintOnNode(c, nodeName, testTaint)
+		framework.ExpectNodeHasTaint(c, nodeName, testTaint)
+		defer framework.RemoveTaintOffNode(c, nodeName, testTaint)
 
 		By("Trying to apply a random label on the found node.")
 		labelKey := fmt.Sprintf("kubernetes.io/e2e-label-key-%s", string(uuid.NewUUID()))
@@ -1354,9 +1356,9 @@ var _ = framework.KubeDescribe("SchedulerPredicates [Serial]", func() {
 					"scheduler.alpha.kubernetes.io/tolerations": `
 						[
 							{
-								"key": "` + taintName + `",
+								"key": "` + testTaint.Key + `",
-								"value": "` + taintValue + `",
+								"value": "` + testTaint.Value + `",
-								"effect": "` + string(taintEffect) + `"
+								"effect": "` + string(testTaint.Effect) + `"
 							}
 						]`,
 				},
@@ -1423,12 +1425,14 @@ var _ = framework.KubeDescribe("SchedulerPredicates [Serial]", func() {
 		framework.ExpectNoError(err)
 
 		By("Trying to apply a random taint on the found node.")
-		taintName := fmt.Sprintf("kubernetes.io/e2e-taint-key-%s", string(uuid.NewUUID()))
+		testTaint := api.Taint{
-		taintValue := "testing-taint-value"
+			Key:    fmt.Sprintf("kubernetes.io/e2e-taint-key-%s", string(uuid.NewUUID())),
-		taintEffect := api.TaintEffectNoSchedule
+			Value:  "testing-taint-value",
-		framework.AddOrUpdateTaintOnNode(c, nodeName, api.Taint{Key: taintName, Value: taintValue, Effect: taintEffect})
+			Effect: api.TaintEffectNoSchedule,
-		framework.ExpectNodeHasTaint(c, nodeName, taintName)
+		}
-		defer framework.RemoveTaintOffNode(c, nodeName, taintName)
+		framework.AddOrUpdateTaintOnNode(c, nodeName, testTaint)
+		framework.ExpectNodeHasTaint(c, nodeName, testTaint)
+		defer framework.RemoveTaintOffNode(c, nodeName, testTaint)
 
 		By("Trying to apply a random label on the found node.")
 		labelKey := fmt.Sprintf("kubernetes.io/e2e-label-key-%s", string(uuid.NewUUID()))
@@ -1466,7 +1470,7 @@ var _ = framework.KubeDescribe("SchedulerPredicates [Serial]", func() {
 		verifyResult(c, podNameNoTolerations, 0, 1, ns)
 
 		By("Removing taint off the node")
-		framework.RemoveTaintOffNode(c, nodeName, taintName)
+		framework.RemoveTaintOffNode(c, nodeName, testTaint)
 		// Wait a bit to allow scheduler to do its thing
 		// TODO: this is brittle; there's no guarantee the scheduler will have run in 10 seconds.
 		framework.Logf("Sleeping 10 seconds and crossing our fingers that scheduler will run in that time.")