Merge pull request #30590 from kevin-wangzefeng/taint-unique-by-key-effect

Automatic merge from submit-queue

make taints unique by <key, effect> on a node

closes #29362

pkg/api/validation/validation.go

@@ -2465,6 +2465,9 @@ func ValidateReadOnlyPersistentDisks(volumes []api.Volume, fldPath *field.Path)
 // validateTaints tests if given taints have valid data.
 func validateTaints(taints []api.Taint, fldPath *field.Path) field.ErrorList {
 	allErrors := field.ErrorList{}
+
+	uniqueTaints := map[api.TaintEffect]sets.String{}
+
 	for i, currTaint := range taints {
 		idxPath := fldPath.Index(i)
 		// validate the taint key
@@ -2475,6 +2478,20 @@ func validateTaints(taints []api.Taint, fldPath *field.Path) field.ErrorList {
 		}
 		// validate the taint effect
 		allErrors = append(allErrors, validateTaintEffect(&currTaint.Effect, false, idxPath.Child("effect"))...)
+
+		// validate if taint is unique by <key, effect>
+		if len(uniqueTaints[currTaint.Effect]) > 0 && uniqueTaints[currTaint.Effect].Has(currTaint.Key) {
+			duplicatedError := field.Duplicate(idxPath, currTaint)
+			duplicatedError.Detail = "taints must be unique by key and effect pair"
+			allErrors = append(allErrors, duplicatedError)
+			continue
+		}
+
+		// add taint to existingTaints for uniqueness check
+		if len(uniqueTaints[currTaint.Effect]) == 0 {
+			uniqueTaints[currTaint.Effect] = sets.String{}
+		}
+		uniqueTaints[currTaint.Effect].Insert(currTaint.Key)
 	}
 	return allErrors
 }

pkg/api/validation/validation_test.go

@@ -5592,7 +5592,6 @@ func TestValidateNode(t *testing.T) {
 			},
 		},
 		"missing-taint-key": {
-
 			ObjectMeta: api.ObjectMeta{
 				Name: "dedicated-node1",
 				// Add a taint with an empty key to a node
@@ -5704,6 +5703,27 @@ func TestValidateNode(t *testing.T) {
 				ExternalID: "external",
 			},
 		},
+		"duplicated-taints-with-same-key-effect": {
+			ObjectMeta: api.ObjectMeta{
+				Name: "dedicated-node1",
+				// Add two taints to the node with the same key and effect; should be rejected.
+				Annotations: map[string]string{
+					api.TaintsAnnotationKey: `
+					[{
+						"key": "dedicated",
+						"value": "special-user-1",
+						"effect": "NoSchedule"
+					}, {
+						"key": "dedicated",
+						"value": "special-user-2",
+						"effect": "NoSchedule"
+					}]`,
+				},
+			},
+			Spec: api.NodeSpec{
+				ExternalID: "external",
+			},
+		},
 		"missing-podSignature": {
 			ObjectMeta: api.ObjectMeta{
 				Name: "abc-123",

pkg/kubectl/cmd/taint.go

@@ -40,16 +40,16 @@ import (
 
 // TaintOptions have the data required to perform the taint operation
 type TaintOptions struct {
-	resources       []string
+	resources      []string
-	taintsToAdd     []api.Taint
+	taintsToAdd    []api.Taint
-	removeTaintKeys []string
+	taintsToRemove []api.Taint
-	builder         *resource.Builder
+	builder        *resource.Builder
-	selector        string
+	selector       string
-	overwrite       bool
+	overwrite      bool
-	all             bool
+	all            bool
-	f               *cmdutil.Factory
+	f              *cmdutil.Factory
-	out             io.Writer
+	out            io.Writer
-	cmd             *cobra.Command
+	cmd            *cobra.Command
 }
 
 var (
@@ -63,9 +63,13 @@ var (
 		Currently taint can only apply to node.`)
 	taint_example = dedent.Dedent(`
 		# Update node 'foo' with a taint with key 'dedicated' and value 'special-user' and effect 'NoSchedule'.
-		# If a taint with that key already exists, its value and effect are replaced as specified.
+		# If a taint with that key and effect already exists, its value is replaced as specified.
 		kubectl taint nodes foo dedicated=special-user:NoSchedule
-		# Remove from node 'foo' the taint with key 'dedicated' if one exists.
+
+		# Remove from node 'foo' the taint with key 'dedicated' and effect 'NoSchedule' if one exists.
+		kubectl taint nodes foo dedicated:NoSchedule-
+
+		# Remove from node 'foo' all the taints with key 'dedicated'
 		kubectl taint nodes foo dedicated-`)
 )
 
@@ -110,11 +114,12 @@ func NewCmdTaint(f *cmdutil.Factory, out io.Writer) *cobra.Command {
 	return cmd
 }
 
-func deleteTaintByKey(taints []api.Taint, key string) ([]api.Taint, error) {
+func deleteTaint(taints []api.Taint, taintToDelete api.Taint) ([]api.Taint, error) {
 	newTaints := []api.Taint{}
 	found := false
 	for _, taint := range taints {
-		if taint.Key == key {
+		if taint.Key == taintToDelete.Key &&
+			(len(taintToDelete.Effect) == 0 || taint.Effect == taintToDelete.Effect) {
 			found = true
 			continue
 		}
@@ -122,14 +127,14 @@ func deleteTaintByKey(taints []api.Taint, key string) ([]api.Taint, error) {
 	}
 
 	if !found {
-		return nil, fmt.Errorf("taint key=\"%s\" not found.", key)
+		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 meta.Object, overwrite bool, taintsToAdd []api.Taint, removeKeys []string) ([]api.Taint, error) {
+func reorganizeTaints(accessor meta.Object, overwrite bool, taintsToAdd []api.Taint, taintsToRemove []api.Taint) ([]api.Taint, error) {
 	newTaints := append([]api.Taint{}, taintsToAdd...)
 
 	var oldTaints []api.Taint
@@ -145,7 +150,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 {
+			if taint.Key == oldTaint.Key && taint.Effect == oldTaint.Effect {
 				existsInNew = true
 				break
 			}
@@ -156,8 +161,8 @@ func reorganizeTaints(accessor meta.Object, overwrite bool, taintsToAdd []api.Ta
 	}
 
 	allErrs := []error{}
-	for _, taintToRemove := range removeKeys {
+	for _, taintToRemove := range taintsToRemove {
-		newTaints, err = deleteTaintByKey(newTaints, taintToRemove)
+		newTaints, err = deleteTaint(newTaints, taintToRemove)
 		if err != nil {
 			allErrs = append(allErrs, err)
 		}
@@ -165,9 +170,10 @@ func reorganizeTaints(accessor meta.Object, overwrite bool, taintsToAdd []api.Ta
 	return newTaints, utilerrors.NewAggregate(allErrs)
 }
 
-func parseTaints(spec []string) ([]api.Taint, []string, error) {
+func parseTaints(spec []string) ([]api.Taint, []api.Taint, error) {
-	var taints []api.Taint
+	var taints, taintsToRemove []api.Taint
-	var remove []string
+	uniqueTaints := map[api.TaintEffect]sets.String{}
+
 	for _, taintSpec := range spec {
 		if strings.Index(taintSpec, "=") != -1 && strings.Index(taintSpec, ":") != -1 {
 			parts := strings.Split(taintSpec, "=")
@@ -192,14 +198,31 @@ func parseTaints(spec []string) ([]api.Taint, []string, error) {
 				Effect: effect,
 			}
 
+			// validate if taint is unique by <key, effect>
+			if len(uniqueTaints[newTaint.Effect]) > 0 && uniqueTaints[newTaint.Effect].Has(newTaint.Key) {
+				return nil, nil, fmt.Errorf("duplicated taints with the same key and effect: %v", newTaint)
+			}
+			// add taint to existingTaints for uniqueness check
+			if len(uniqueTaints[newTaint.Effect]) == 0 {
+				uniqueTaints[newTaint.Effect] = sets.String{}
+			}
+			uniqueTaints[newTaint.Effect].Insert(newTaint.Key)
+
 			taints = append(taints, newTaint)
 		} else if strings.HasSuffix(taintSpec, "-") {
-			remove = append(remove, taintSpec[:len(taintSpec)-1])
+			taintKey := taintSpec[:len(taintSpec)-1]
+			var effect api.TaintEffect
+			if strings.Index(taintKey, ":") != -1 {
+				parts := strings.Split(taintKey, ":")
+				taintKey = parts[0]
+				effect = api.TaintEffect(parts[1])
+			}
+			taintsToRemove = append(taintsToRemove, api.Taint{Key: taintKey, Effect: effect})
 		} else {
 			return nil, nil, fmt.Errorf("unknown taint spec: %v", taintSpec)
 		}
 	}
-	return taints, remove, nil
+	return taints, taintsToRemove, nil
 }
 
 // Complete adapts from the command line args and factory to the data required.
@@ -235,7 +258,7 @@ func (o *TaintOptions) Complete(f *cmdutil.Factory, out io.Writer, cmd *cobra.Co
 		return fmt.Errorf("at least one taint update is required")
 	}
 
-	if o.taintsToAdd, o.removeTaintKeys, err = parseTaints(taintArgs); err != nil {
+	if o.taintsToAdd, o.taintsToRemove, err = parseTaints(taintArgs); err != nil {
 		return cmdutil.UsageError(cmd, err.Error())
 	}
 
@@ -270,15 +293,20 @@ func (o TaintOptions) Validate(args []string) error {
 	}
 
 	// check the format of taint args and checks removed taints aren't in the new taints list
-	conflictKeys := []string{}
+	var conflictTaints []string
-	removeTaintKeysSet := sets.NewString(o.removeTaintKeys...)
+	for _, taintAdd := range o.taintsToAdd {
-	for _, taint := range o.taintsToAdd {
+		for _, taintRemove := range o.taintsToRemove {
-		if removeTaintKeysSet.Has(taint.Key) {
+			if taintAdd.Key != taintRemove.Key {
-			conflictKeys = append(conflictKeys, taint.Key)
+				continue
+			}
+			if len(taintRemove.Effect) == 0 || taintAdd.Effect == taintRemove.Effect {
+				conflictTaint := fmt.Sprintf("{\"%s\":\"%s\"}", taintRemove.Key, taintRemove.Effect)
+				conflictTaints = append(conflictTaints, conflictTaint)
+			}
 		}
 	}
-	if len(conflictKeys) > 0 {
+	if len(conflictTaints) > 0 {
-		return fmt.Errorf("can not both modify and remove the following taint(s) in the same command: %s", strings.Join(conflictKeys, ", "))
+		return fmt.Errorf("can not both modify and remove the following taint(s) in the same command: %s", strings.Join(conflictTaints, ", "))
 	}
 
 	return nil
@@ -363,8 +391,8 @@ func validateNoTaintOverwrites(accessor meta.Object, taints []api.Taint) error {
 
 	for _, taint := range taints {
 		for _, oldTaint := range oldTaints {
-			if taint.Key == oldTaint.Key {
+			if taint.Key == oldTaint.Key && taint.Effect == oldTaint.Effect {
-				allErrs = append(allErrs, fmt.Errorf("Node '%s' already has a taint (%+v), and --overwrite is false", accessor.GetName(), taint))
+				allErrs = append(allErrs, fmt.Errorf("Node '%s' already has a taint with key (%s) and effect (%v), and --overwrite is false", accessor.GetName(), taint.Key, taint.Effect))
 				break
 			}
 		}
@@ -389,7 +417,7 @@ func (o TaintOptions) updateTaints(obj runtime.Object) error {
 		annotations = make(map[string]string)
 	}
 
-	newTaints, err := reorganizeTaints(accessor, o.overwrite, o.taintsToAdd, o.removeTaintKeys)
+	newTaints, err := reorganizeTaints(accessor, o.overwrite, o.taintsToAdd, o.taintsToRemove)
 	if err != nil {
 		return err
 	}

pkg/kubectl/cmd/taint_test.go

@@ -125,7 +125,7 @@ func TestTaint(t *testing.T) {
 			expectTaint: true,
 		},
 		{
-			description: "update an existing taint on the node, change the effect from NoSchedule to PreferNoSchedule",
+			description: "update an existing taint on the node, change the value from bar to barz",
 			oldTaints: []api.Taint{{
 				Key:    "foo",
 				Value:  "bar",
@@ -133,10 +133,10 @@ func TestTaint(t *testing.T) {
 			}},
 			newTaints: []api.Taint{{
 				Key:    "foo",
-				Value:  "bar",
+				Value:  "barz",
-				Effect: "PreferNoSchedule",
+				Effect: "NoSchedule",
 			}},
-			args:        []string{"node", "node-name", "foo=bar:PreferNoSchedule", "--overwrite"},
+			args:        []string{"node", "node-name", "foo=barz:NoSchedule", "--overwrite"},
 			expectFatal: false,
 			expectTaint: true,
 		},
@@ -156,21 +156,37 @@ func TestTaint(t *testing.T) {
 			expectTaint: true,
 		},
 		{
-			description: "node has two taints, remove one of them",
+			description: "node has two taints with the same key but different effect, remove one of them by indicating exact key and effect",
 			oldTaints: []api.Taint{{
 				Key:    "dedicated",
 				Value:  "namespaceA",
 				Effect: "NoSchedule",
 			}, {
-				Key:    "foo",
+				Key:    "dedicated",
-				Value:  "bar",
+				Value:  "namespaceA",
 				Effect: "PreferNoSchedule",
 			}},
 			newTaints: []api.Taint{{
-				Key:    "foo",
+				Key:    "dedicated",
-				Value:  "bar",
+				Value:  "namespaceA",
 				Effect: "PreferNoSchedule",
 			}},
+			args:        []string{"node", "node-name", "dedicated:NoSchedule-"},
+			expectFatal: false,
+			expectTaint: true,
+		},
+		{
+			description: "node has two taints with the same key but different effect, remove all of them with wildcard",
+			oldTaints: []api.Taint{{
+				Key:    "dedicated",
+				Value:  "namespaceA",
+				Effect: "NoSchedule",
+			}, {
+				Key:    "dedicated",
+				Value:  "namespaceA",
+				Effect: "PreferNoSchedule",
+			}},
+			newTaints:   []api.Taint{},
 			args:        []string{"node", "node-name", "dedicated-"},
 			expectFatal: false,
 			expectTaint: true,
@@ -188,10 +204,10 @@ func TestTaint(t *testing.T) {
 			}},
 			newTaints: []api.Taint{{
 				Key:    "foo",
-				Value:  "bar",
+				Value:  "barz",
-				Effect: "NoSchedule",
+				Effect: "PreferNoSchedule",
 			}},
-			args:        []string{"node", "node-name", "dedicated-", "foo=bar:NoSchedule", "--overwrite"},
+			args:        []string{"node", "node-name", "dedicated:NoSchedule-", "foo=barz:PreferNoSchedule", "--overwrite"},
 			expectFatal: false,
 			expectTaint: true,
 		},
@@ -209,6 +225,12 @@ func TestTaint(t *testing.T) {
 			expectFatal: true,
 			expectTaint: false,
 		},
+		{
+			description: "duplicated taints with the same key and effect should be rejected",
+			args:        []string{"node", "node-name", "foo=bar:NoExcute", "foo=barz:NoExcute"},
+			expectFatal: true,
+			expectTaint: false,
+		},
 		{
 			description: "can't update existing taint on the node, since 'overwrite' flag is not set",
 			oldTaints: []api.Taint{{
@@ -221,7 +243,7 @@ func TestTaint(t *testing.T) {
 				Value:  "bar",
 				Effect: "NoSchedule",
 			}},
-			args:        []string{"node", "node-name", "foo=bar:PreferNoSchedule"},
+			args:        []string{"node", "node-name", "foo=bar:NoSchedule"},
 			expectFatal: true,
 			expectTaint: false,
 		},

pkg/kubectl/describe.go

@@ -2546,15 +2546,19 @@ func printTaintsMultilineWithIndent(out io.Writer, initialIndent, title, innerIn
 	}
 	sort.Strings(keys)
 
+	effects := []api.TaintEffect{api.TaintEffectNoSchedule, api.TaintEffectPreferNoSchedule}
+
 	for i, key := range keys {
-		for _, taint := range taints {
+		for _, effect := range effects {
-			if taint.Key == key {
+			for _, taint := range taints {
-				if i != 0 {
+				if taint.Key == key && taint.Effect == effect {
-					fmt.Fprint(out, initialIndent)
+					if i != 0 {
-					fmt.Fprint(out, innerIndent)
+						fmt.Fprint(out, initialIndent)
+						fmt.Fprint(out, innerIndent)
+					}
+					fmt.Fprintf(out, "%s=%s:%s\n", taint.Key, taint.Value, taint.Effect)
+					i++
 				}
-				fmt.Fprintf(out, "%s=%s:%s\n", taint.Key, taint.Value, taint.Effect)
-				i++
 			}
 		}
 	}

test/e2e/kubectl.go

@@ -1190,7 +1190,7 @@ var _ = framework.KubeDescribe("Kubectl client", func() {
 			checkOutput(output, requiredStrings)
 
 			By("removing the taint " + taintName + " of a node")
-			framework.RunKubectlOrDie("taint", "nodes", nodeName, taintName+"-")
+			framework.RunKubectlOrDie("taint", "nodes", nodeName, taintName+":"+taintEffect+"-")
 			By("verifying the node doesn't have the taint " + taintName)
 			output = framework.RunKubectlOrDie("describe", "node", nodeName)
 			if strings.Contains(output, taintName) {
@@ -1198,6 +1198,49 @@ var _ = framework.KubeDescribe("Kubectl client", func() {
 			}
 
 		})
+
+		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()))
+			taintValue := "testing-taint-value"
+			taintEffect := fmt.Sprintf("%s", 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")
+			framework.RunKubectlOrDie("taint", "nodes", nodeName, taintName+"="+taintValue+":"+taintEffect)
+			By("verifying the node has the taint " + taintName + " with the value " + taintValue)
+			output := framework.RunKubectlOrDie("describe", "node", nodeName)
+			requiredStrings := [][]string{
+				{"Name:", nodeName},
+				{"Taints:"},
+				{taintName + "=" + taintValue + ":" + taintEffect},
+			}
+			checkOutput(output, requiredStrings)
+
+			newTaintValue := "another-testing-taint-value"
+			newTaintEffect := fmt.Sprintf("%s", api.TaintEffectPreferNoSchedule)
+			By("adding another taint " + taintName + " with value " + newTaintValue + " and taint effect " + newTaintEffect + " to the node")
+			framework.RunKubectlOrDie("taint", "nodes", nodeName, taintName+"="+newTaintValue+":"+newTaintEffect)
+			By("verifying the node has the taint " + taintName + " with the value " + newTaintValue)
+			output = framework.RunKubectlOrDie("describe", "node", nodeName)
+			requiredStrings = [][]string{
+				{"Name:", nodeName},
+				{"Taints:"},
+				{taintName + "=" + newTaintValue + ":" + newTaintEffect},
+			}
+			checkOutput(output, requiredStrings)
+
+			By("removing the taint " + taintName + " of a node")
+			framework.RunKubectlOrDie("taint", "nodes", nodeName, taintName+"-")
+			By("verifying the node doesn't have the taints " + taintName)
+			output = framework.RunKubectlOrDie("describe", "node", nodeName)
+			if strings.Contains(output, taintName) {
+				framework.Failf("Failed removing taint " + taintName + " of the node " + nodeName)
+			}
+		})
 	})
 
 	framework.KubeDescribe("Kubectl create quota", func() {