From 1e2d560253b0b397d4fe9b50a94406a38f6e5293 Mon Sep 17 00:00:00 2001 From: Kevin Date: Fri, 12 Aug 2016 16:46:40 +0800 Subject: [PATCH] make taints unique by on a node --- pkg/api/validation/validation.go | 17 +++++ pkg/api/validation/validation_test.go | 22 +++++- pkg/kubectl/cmd/taint.go | 98 +++++++++++++++++---------- pkg/kubectl/cmd/taint_test.go | 48 +++++++++---- pkg/kubectl/describe.go | 18 +++-- test/e2e/kubectl.go | 45 +++++++++++- 6 files changed, 191 insertions(+), 57 deletions(-) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index ce206aad215..43e83511ad3 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -2405,6 +2405,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 @@ -2415,6 +2418,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 + 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 } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index c50f7654859..f6657c2a9e0 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -5329,7 +5329,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 @@ -5441,6 +5440,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", diff --git a/pkg/kubectl/cmd/taint.go b/pkg/kubectl/cmd/taint.go index 7a7b6bda2cb..97a409975d6 100644 --- a/pkg/kubectl/cmd/taint.go +++ b/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 - taintsToAdd []api.Taint - removeTaintKeys []string - builder *resource.Builder - selector string - overwrite bool - all bool - f *cmdutil.Factory - out io.Writer - cmd *cobra.Command + resources []string + taintsToAdd []api.Taint + taintsToRemove []api.Taint + builder *resource.Builder + selector string + overwrite bool + all bool + f *cmdutil.Factory + out io.Writer + 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 { - newTaints, err = deleteTaintByKey(newTaints, taintToRemove) + for _, taintToRemove := range taintsToRemove { + 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) { - var taints []api.Taint - var remove []string +func parseTaints(spec []string) ([]api.Taint, []api.Taint, error) { + var taints, taintsToRemove []api.Taint + 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 + 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{} - removeTaintKeysSet := sets.NewString(o.removeTaintKeys...) - for _, taint := range o.taintsToAdd { - if removeTaintKeysSet.Has(taint.Key) { - conflictKeys = append(conflictKeys, taint.Key) + var conflictTaints []string + for _, taintAdd := range o.taintsToAdd { + for _, taintRemove := range o.taintsToRemove { + if taintAdd.Key != taintRemove.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 { - return fmt.Errorf("can not both modify and remove the following taint(s) in the same command: %s", strings.Join(conflictKeys, ", ")) + if len(conflictTaints) > 0 { + 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 { - allErrs = append(allErrs, fmt.Errorf("Node '%s' already has a taint (%+v), and --overwrite is false", accessor.GetName(), taint)) + if taint.Key == oldTaint.Key && taint.Effect == oldTaint.Effect { + 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 } diff --git a/pkg/kubectl/cmd/taint_test.go b/pkg/kubectl/cmd/taint_test.go index 9d722ee125e..0c9427b3823 100644 --- a/pkg/kubectl/cmd/taint_test.go +++ b/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", - Effect: "PreferNoSchedule", + Value: "barz", + 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", - Value: "bar", + Key: "dedicated", + Value: "namespaceA", Effect: "PreferNoSchedule", }}, newTaints: []api.Taint{{ - Key: "foo", - Value: "bar", + Key: "dedicated", + 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", - Effect: "NoSchedule", + Value: "barz", + 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, }, diff --git a/pkg/kubectl/describe.go b/pkg/kubectl/describe.go index 5b6aebb4118..ea089c50d53 100644 --- a/pkg/kubectl/describe.go +++ b/pkg/kubectl/describe.go @@ -2462,15 +2462,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 { - if taint.Key == key { - if i != 0 { - fmt.Fprint(out, initialIndent) - fmt.Fprint(out, innerIndent) + for _, effect := range effects { + for _, taint := range taints { + if taint.Key == key && taint.Effect == effect { + if i != 0 { + 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++ } } } diff --git a/test/e2e/kubectl.go b/test/e2e/kubectl.go index 4baab2020fd..c4bc0b23c31 100644 --- a/test/e2e/kubectl.go +++ b/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() {