From e1e4370ecd90b2a8353127d35ee498ee6f04a6c6 Mon Sep 17 00:00:00 2001 From: gmarek Date: Mon, 6 Feb 2017 13:59:50 +0100 Subject: [PATCH 1/2] Promote taint addition/removal to api/v1/helpers.go --- pkg/api/v1/helpers.go | 91 ++++++++++++++++++++- pkg/api/v1/helpers_test.go | 2 +- pkg/controller/controller_utils.go | 102 ++++++++++++++++++++++- pkg/kubectl/cmd/taint.go | 2 +- pkg/util/taints/taints.go | 1 + test/e2e/framework/util.go | 126 +++++------------------------ test/e2e/scheduler_predicates.go | 4 +- test/e2e/taints_test.go | 8 +- 8 files changed, 218 insertions(+), 118 deletions(-) diff --git a/pkg/api/v1/helpers.go b/pkg/api/v1/helpers.go index 2f5587e908a..110ab3b5852 100644 --- a/pkg/api/v1/helpers.go +++ b/pkg/api/v1/helpers.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/kubernetes/pkg/api" ) @@ -392,7 +393,7 @@ func DeleteTaint(taints []Taint, taintToDelete *Taint) ([]Taint, bool) { newTaints := []Taint{} deleted := false for i := range taints { - if taintToDelete.MatchTaint(taints[i]) { + if taintToDelete.MatchTaint(&taints[i]) { deleted = true continue } @@ -428,7 +429,7 @@ func GetMatchingTolerations(taints []Taint, tolerations []Toleration) (bool, []T // 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 { +func (t *Taint) MatchTaint(taintToMatch *Taint) bool { return t.Key == taintToMatch.Key && t.Effect == taintToMatch.Effect } @@ -510,3 +511,89 @@ type NodeResources struct { // Capacity represents the available resources of a node Capacity ResourceList `protobuf:"bytes,1,rep,name=capacity,casttype=ResourceList,castkey=ResourceName"` } + +// Tries to add a taint to annotations list. Returns a new copy of updated Node and true if something was updated +// false otherwise. +func AddOrUpdateTaint(node *Node, taint *Taint) (*Node, bool, error) { + objCopy, err := api.Scheme.DeepCopy(node) + if err != nil { + return nil, false, err + } + newNode := objCopy.(*Node) + nodeTaints, err := GetTaintsFromNodeAnnotations(newNode.Annotations) + if err != nil { + return newNode, false, err + } + + var newTaints []*Taint + updated := false + for i := range nodeTaints { + if taint.MatchTaint(&nodeTaints[i]) { + if api.Semantic.DeepEqual(taint, nodeTaints[i]) { + return newNode, false, nil + } + newTaints = append(newTaints, taint) + updated = true + continue + } + + newTaints = append(newTaints, &nodeTaints[i]) + } + + if !updated { + newTaints = append(newTaints, taint) + } + + taintsData, err := json.Marshal(newTaints) + if err != nil { + return nil, false, err + } + + if newNode.Annotations == nil { + newNode.Annotations = make(map[string]string) + } + newNode.Annotations[TaintsAnnotationKey] = string(taintsData) + return newNode, true, nil +} + +func TaintExists(taints []Taint, taintToFind *Taint) bool { + for _, taint := range taints { + if taint.MatchTaint(taintToFind) { + return true + } + } + return false +} + +// Tries to remove a taint from annotations list. Returns a new copy of updated Node and true if something was updated +// false otherwise. +func RemoveTaint(node *Node, taint *Taint) (*Node, bool, error) { + objCopy, err := api.Scheme.DeepCopy(node) + if err != nil { + return nil, false, err + } + newNode := objCopy.(*Node) + nodeTaints, err := GetTaintsFromNodeAnnotations(newNode.Annotations) + if err != nil { + return newNode, false, err + } + if len(nodeTaints) == 0 { + return newNode, false, nil + } + + if !TaintExists(nodeTaints, taint) { + return newNode, false, nil + } + + newTaints, _ := DeleteTaint(nodeTaints, taint) + if len(newTaints) == 0 { + delete(newNode.Annotations, TaintsAnnotationKey) + } else { + taintsData, err := json.Marshal(newTaints) + if err != nil { + return newNode, false, err + } + newNode.Annotations[TaintsAnnotationKey] = string(taintsData) + } + return newNode, true, nil +} diff --git a/pkg/api/v1/helpers_test.go b/pkg/api/v1/helpers_test.go index 62acc6285b7..fd51eb18184 100644 --- a/pkg/api/v1/helpers_test.go +++ b/pkg/api/v1/helpers_test.go @@ -274,7 +274,7 @@ func TestMatchTaint(t *testing.T) { } for _, tc := range testCases { - if tc.expectMatch != tc.taint.MatchTaint(tc.taintToMatch) { + 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()) } } diff --git a/pkg/controller/controller_utils.go b/pkg/controller/controller_utils.go index 5b827a78ce8..e18d7332102 100644 --- a/pkg/controller/controller_utils.go +++ b/pkg/controller/controller_utils.go @@ -17,12 +17,12 @@ limitations under the License. package controller import ( + "encoding/json" "fmt" "sync" "sync/atomic" "time" - "github.com/golang/glog" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -30,15 +30,22 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/strategicpatch" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" "k8s.io/client-go/util/clock" "k8s.io/client-go/util/integer" + "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/v1" "k8s.io/kubernetes/pkg/api/validation" extensions "k8s.io/kubernetes/pkg/apis/extensions/v1beta1" "k8s.io/kubernetes/pkg/client/clientset_generated/clientset" + clientretry "k8s.io/kubernetes/pkg/client/retry" + + "github.com/golang/glog" ) const ( @@ -55,6 +62,12 @@ const ( ExpectationsTimeout = 5 * time.Minute ) +var UpdateTaintBackoff = wait.Backoff{ + Steps: 5, + Duration: 100 * time.Millisecond, + Jitter: 1.0, +} + var ( KeyFunc = cache.DeletionHandlingMetaNamespaceKeyFunc ) @@ -822,3 +835,90 @@ func (o ReplicaSetsBySizeNewer) Less(i, j int) bool { } return *(o[i].Spec.Replicas) > *(o[j].Spec.Replicas) } + +func AddOrUpdateTaintOnNode(c clientset.Interface, nodeName string, taint *v1.Taint) error { + firstTry := true + return clientretry.RetryOnConflict(UpdateTaintBackoff, func() error { + var err error + var oldNode *v1.Node + // First we try getting node from the API server cache, as it's cheaper. If it fails + // we get it from etcd to be sure to have fresh data. + if firstTry { + oldNode, err = c.Core().Nodes().Get(nodeName, metav1.GetOptions{ResourceVersion: "0"}) + firstTry = false + } else { + oldNode, err = c.Core().Nodes().Get(nodeName, metav1.GetOptions{}) + } + if err != nil { + return err + } + newNode, ok, err := v1.AddOrUpdateTaint(oldNode, taint) + if err != nil { + return fmt.Errorf("Failed to update taint annotation!") + } + if !ok { + return nil + } + return PatchNodeTaints(c, nodeName, oldNode, newNode) + }) +} + +// 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) error { + firstTry := true + return clientretry.RetryOnConflict(UpdateTaintBackoff, func() error { + var err error + var oldNode *v1.Node + // First we try getting node from the API server cache, as it's cheaper. If it fails + // we get it from etcd to be sure to have fresh data. + if firstTry { + oldNode, err = c.Core().Nodes().Get(nodeName, metav1.GetOptions{ResourceVersion: "0"}) + firstTry = false + } else { + oldNode, err = c.Core().Nodes().Get(nodeName, metav1.GetOptions{}) + } + if err != nil { + return err + } + newNode, ok, err := v1.RemoveTaint(oldNode, taint) + if err != nil { + return fmt.Errorf("Failed to update taint annotation!") + } + if !ok { + return nil + } + return PatchNodeTaints(c, nodeName, oldNode, newNode) + }) +} + +// PatchNodeTaints patches node's taints. +func PatchNodeTaints(c clientset.Interface, nodeName string, oldNode *v1.Node, newNode *v1.Node) error { + oldData, err := json.Marshal(oldNode) + if err != nil { + return fmt.Errorf("failed to marshal old node %#v for node %q: %v", oldNode, nodeName, err) + } + + newAnnotations := newNode.Annotations + objCopy, err := api.Scheme.DeepCopy(oldNode) + if err != nil { + return fmt.Errorf("failed to copy node object %#v: %v", oldNode, err) + } + newNode, ok := (objCopy).(*v1.Node) + if !ok { + return fmt.Errorf("failed to cast copy onto node object %#v: %v", newNode, err) + } + newNode.Annotations = newAnnotations + newData, err := json.Marshal(newNode) + if err != nil { + return fmt.Errorf("failed to marshal new node %#v for node %q: %v", newNode, nodeName, err) + } + + patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, v1.Node{}) + if err != nil { + return fmt.Errorf("failed to create patch for node %q: %v", nodeName, err) + } + + _, err = c.Core().Nodes().Patch(string(nodeName), types.StrategicMergePatchType, patchBytes) + return err +} diff --git a/pkg/kubectl/cmd/taint.go b/pkg/kubectl/cmd/taint.go index e56bf9d36fa..0008b1de929 100644 --- a/pkg/kubectl/cmd/taint.go +++ b/pkg/kubectl/cmd/taint.go @@ -131,7 +131,7 @@ func reorganizeTaints(accessor metav1.Object, overwrite bool, taintsToAdd []v1.T for _, oldTaint := range oldTaints { existsInNew := false for _, taint := range newTaints { - if taint.MatchTaint(oldTaint) { + if taint.MatchTaint(&oldTaint) { existsInNew = true break } diff --git a/pkg/util/taints/taints.go b/pkg/util/taints/taints.go index 5c0e8c17899..2ea3471ef1a 100644 --- a/pkg/util/taints/taints.go +++ b/pkg/util/taints/taints.go @@ -22,6 +22,7 @@ import ( "strings" "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/v1" ) diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index 9f7b5068d58..8d85008bfee 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -19,7 +19,6 @@ package framework import ( "bytes" "context" - "encoding/json" "errors" "fmt" "io" @@ -63,11 +62,13 @@ import ( "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" + "k8s.io/client-go/discovery" "k8s.io/client-go/dynamic" restclient "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" + "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/v1" apps "k8s.io/kubernetes/pkg/apis/apps/v1beta1" @@ -164,9 +165,6 @@ const ( // restart before test is considered failed. RestartPodReadyAgainTimeout = 5 * time.Minute - // Number of times we want to retry Updates in case of conflict - UpdateRetries = 5 - // Number of objects that gc can delete in a second. // GC issues 2 requestes for single delete. gcThroughput = 10 @@ -2491,6 +2489,14 @@ func ExpectNodeHasLabel(c clientset.Interface, nodeName string, labelKey string, Expect(node.Labels[labelKey]).To(Equal(labelValue)) } +func RemoveTaintOffNode(c clientset.Interface, nodeName string, taint v1.Taint) { + ExpectNoError(controller.RemoveTaintOffNode(c, nodeName, &taint)) +} + +func AddOrUpdateTaintOnNode(c clientset.Interface, nodeName string, taint v1.Taint) { + ExpectNoError(controller.AddOrUpdateTaintOnNode(c, nodeName, &taint)) +} + // RemoveLabelOffNode is for cleaning up labels temporarily added to node, // won't fail if target label doesn't exist or has been removed. func RemoveLabelOffNode(c clientset.Interface, nodeName string, labelKey string) { @@ -2501,61 +2507,17 @@ func RemoveLabelOffNode(c clientset.Interface, nodeName string, labelKey string) ExpectNoError(testutils.VerifyLabelsRemoved(c, nodeName, []string{labelKey})) } -func AddOrUpdateTaintOnNode(c clientset.Interface, nodeName string, taint v1.Taint) { - for attempt := 0; attempt < UpdateRetries; attempt++ { - node, err := c.Core().Nodes().Get(nodeName, metav1.GetOptions{}) - ExpectNoError(err) - - nodeTaints, err := v1.GetTaintsFromNodeAnnotations(node.Annotations) - ExpectNoError(err) - - var newTaints []v1.Taint - updated := false - for _, existingTaint := range nodeTaints { - if taint.MatchTaint(existingTaint) { - newTaints = append(newTaints, taint) - updated = true - continue - } - - newTaints = append(newTaints, existingTaint) - } - - if !updated { - newTaints = append(newTaints, taint) - } - - taintsData, err := json.Marshal(newTaints) - ExpectNoError(err) - - if node.Annotations == nil { - node.Annotations = make(map[string]string) - } - node.Annotations[v1.TaintsAnnotationKey] = string(taintsData) - _, err = c.Core().Nodes().Update(node) - if err != nil { - if !apierrs.IsConflict(err) { - ExpectNoError(err) - } else { - Logf("Conflict when trying to add/update taint %v to %v", taint, nodeName) - } - } else { - break - } - time.Sleep(100 * time.Millisecond) +func VerifyThatTaintIsGone(c clientset.Interface, nodeName string, taint *v1.Taint) { + By("verifying the node doesn't have the taint " + taint.ToString()) + nodeUpdated, err := c.Core().Nodes().Get(nodeName, metav1.GetOptions{}) + taintsGot, err := v1.GetTaintsFromNodeAnnotations(nodeUpdated.Annotations) + ExpectNoError(err) + if v1.TaintExists(taintsGot, taint) { + Failf("Failed removing taint " + taint.ToString() + " of the node " + nodeName) } } -func taintExists(taints []v1.Taint, taintToFind v1.Taint) bool { - for _, taint := range taints { - if taint.MatchTaint(taintToFind) { - return true - } - } - return false -} - -func ExpectNodeHasTaint(c clientset.Interface, nodeName string, taint v1.Taint) { +func ExpectNodeHasTaint(c clientset.Interface, nodeName string, taint *v1.Taint) { By("verifying the node has the taint " + taint.ToString()) node, err := c.Core().Nodes().Get(nodeName, metav1.GetOptions{}) ExpectNoError(err) @@ -2563,61 +2525,11 @@ func ExpectNodeHasTaint(c clientset.Interface, nodeName string, taint v1.Taint) nodeTaints, err := v1.GetTaintsFromNodeAnnotations(node.Annotations) ExpectNoError(err) - if len(nodeTaints) == 0 || !taintExists(nodeTaints, taint) { + if len(nodeTaints) == 0 || !v1.TaintExists(nodeTaints, taint) { Failf("Failed to find taint %s on node %s", taint.ToString(), nodeName) } } -// 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) { - By("removing the taint " + taint.ToString() + " off the node " + nodeName) - for attempt := 0; attempt < UpdateRetries; attempt++ { - node, err := c.Core().Nodes().Get(nodeName, metav1.GetOptions{}) - ExpectNoError(err) - - nodeTaints, err := v1.GetTaintsFromNodeAnnotations(node.Annotations) - ExpectNoError(err) - if len(nodeTaints) == 0 { - return - } - - if !taintExists(nodeTaints, taint) { - return - } - - newTaints, _ := v1.DeleteTaint(nodeTaints, &taint) - if len(newTaints) == 0 { - delete(node.Annotations, v1.TaintsAnnotationKey) - } else { - taintsData, err := json.Marshal(newTaints) - ExpectNoError(err) - node.Annotations[v1.TaintsAnnotationKey] = string(taintsData) - } - - _, err = c.Core().Nodes().Update(node) - if err != nil { - if !apierrs.IsConflict(err) { - ExpectNoError(err) - } else { - Logf("Conflict when trying to add/update taint %s to node %v", taint.ToString(), nodeName) - } - } else { - break - } - time.Sleep(100 * time.Millisecond) - } - - nodeUpdated, err := c.Core().Nodes().Get(nodeName, metav1.GetOptions{}) - ExpectNoError(err) - By("verifying the node doesn't have the taint " + taint.ToString()) - taintsGot, err := v1.GetTaintsFromNodeAnnotations(nodeUpdated.Annotations) - ExpectNoError(err) - if taintExists(taintsGot, taint) { - Failf("Failed removing taint " + taint.ToString() + " of the node " + nodeName) - } -} - func getScalerForKind(internalClientset internalclientset.Interface, kind schema.GroupKind) (kubectl.Scaler, error) { return kubectl.ScalerFor(kind, internalClientset) } diff --git a/test/e2e/scheduler_predicates.go b/test/e2e/scheduler_predicates.go index 15b0303f4a9..c13b1ae1a43 100644 --- a/test/e2e/scheduler_predicates.go +++ b/test/e2e/scheduler_predicates.go @@ -676,7 +676,7 @@ var _ = framework.KubeDescribe("SchedulerPredicates [Serial]", func() { Effect: v1.TaintEffectNoSchedule, } framework.AddOrUpdateTaintOnNode(cs, nodeName, testTaint) - framework.ExpectNodeHasTaint(cs, nodeName, testTaint) + framework.ExpectNodeHasTaint(cs, nodeName, &testTaint) defer framework.RemoveTaintOffNode(cs, nodeName, testTaint) By("Trying to apply a random label on the found node.") @@ -728,7 +728,7 @@ var _ = framework.KubeDescribe("SchedulerPredicates [Serial]", func() { Effect: v1.TaintEffectNoSchedule, } framework.AddOrUpdateTaintOnNode(cs, nodeName, testTaint) - framework.ExpectNodeHasTaint(cs, nodeName, testTaint) + framework.ExpectNodeHasTaint(cs, nodeName, &testTaint) defer framework.RemoveTaintOffNode(cs, nodeName, testTaint) By("Trying to apply a random label on the found node.") diff --git a/test/e2e/taints_test.go b/test/e2e/taints_test.go index b9d84235bbc..9d9eeee6d55 100644 --- a/test/e2e/taints_test.go +++ b/test/e2e/taints_test.go @@ -193,7 +193,7 @@ var _ = framework.KubeDescribe("NoExecuteTaintManager [Serial]", func() { By("Trying to apply a taint on the Node") testTaint := getTestTaint() framework.AddOrUpdateTaintOnNode(cs, nodeName, testTaint) - framework.ExpectNodeHasTaint(cs, nodeName, testTaint) + framework.ExpectNodeHasTaint(cs, nodeName, &testTaint) defer framework.RemoveTaintOffNode(cs, nodeName, testTaint) // Wait a bit @@ -225,7 +225,7 @@ var _ = framework.KubeDescribe("NoExecuteTaintManager [Serial]", func() { By("Trying to apply a taint on the Node") testTaint := getTestTaint() framework.AddOrUpdateTaintOnNode(cs, nodeName, testTaint) - framework.ExpectNodeHasTaint(cs, nodeName, testTaint) + framework.ExpectNodeHasTaint(cs, nodeName, &testTaint) defer framework.RemoveTaintOffNode(cs, nodeName, testTaint) // Wait a bit @@ -258,7 +258,7 @@ var _ = framework.KubeDescribe("NoExecuteTaintManager [Serial]", func() { By("Trying to apply a taint on the Node") testTaint := getTestTaint() framework.AddOrUpdateTaintOnNode(cs, nodeName, testTaint) - framework.ExpectNodeHasTaint(cs, nodeName, testTaint) + framework.ExpectNodeHasTaint(cs, nodeName, &testTaint) defer framework.RemoveTaintOffNode(cs, nodeName, testTaint) // Wait a bit @@ -302,7 +302,7 @@ var _ = framework.KubeDescribe("NoExecuteTaintManager [Serial]", func() { By("Trying to apply a taint on the Node") testTaint := getTestTaint() framework.AddOrUpdateTaintOnNode(cs, nodeName, testTaint) - framework.ExpectNodeHasTaint(cs, nodeName, testTaint) + framework.ExpectNodeHasTaint(cs, nodeName, &testTaint) taintRemoved := false defer func() { if !taintRemoved { From 6b20bb790f05ef5a53ae1de8f711fe0b31d50078 Mon Sep 17 00:00:00 2001 From: gmarek Date: Mon, 6 Feb 2017 16:18:17 +0100 Subject: [PATCH 2/2] generated --- pkg/controller/BUILD | 3 +++ pkg/util/taints/taints.go | 1 - test/e2e/framework/util.go | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/controller/BUILD b/pkg/controller/BUILD index 78daa3da1c6..15f54086d26 100644 --- a/pkg/controller/BUILD +++ b/pkg/controller/BUILD @@ -25,6 +25,7 @@ go_library( "//pkg/apis/extensions/v1beta1:go_default_library", "//pkg/client/clientset_generated/clientset:go_default_library", "//pkg/client/clientset_generated/clientset/typed/core/v1:go_default_library", + "//pkg/client/retry:go_default_library", "//pkg/serviceaccount:go_default_library", "//pkg/util/hash:go_default_library", "//vendor:github.com/golang/glog", @@ -38,6 +39,8 @@ go_library( "//vendor:k8s.io/apimachinery/pkg/runtime/schema", "//vendor:k8s.io/apimachinery/pkg/types", "//vendor:k8s.io/apimachinery/pkg/util/sets", + "//vendor:k8s.io/apimachinery/pkg/util/strategicpatch", + "//vendor:k8s.io/apimachinery/pkg/util/wait", "//vendor:k8s.io/apimachinery/pkg/watch", "//vendor:k8s.io/apiserver/pkg/authentication/serviceaccount", "//vendor:k8s.io/client-go/kubernetes", diff --git a/pkg/util/taints/taints.go b/pkg/util/taints/taints.go index 2ea3471ef1a..5c0e8c17899 100644 --- a/pkg/util/taints/taints.go +++ b/pkg/util/taints/taints.go @@ -22,7 +22,6 @@ import ( "strings" "k8s.io/apimachinery/pkg/util/validation" - "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/v1" ) diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index 8d85008bfee..a8fee4e0896 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -2491,6 +2491,7 @@ func ExpectNodeHasLabel(c clientset.Interface, nodeName string, labelKey string, func RemoveTaintOffNode(c clientset.Interface, nodeName string, taint v1.Taint) { ExpectNoError(controller.RemoveTaintOffNode(c, nodeName, &taint)) + VerifyThatTaintIsGone(c, nodeName, &taint) } func AddOrUpdateTaintOnNode(c clientset.Interface, nodeName string, taint v1.Taint) {