From 18ae1ba813f6ba8ff7b80ede4f6471d4c8b46a3b Mon Sep 17 00:00:00 2001 From: Klaus Ma Date: Mon, 7 Aug 2017 19:29:39 +0800 Subject: [PATCH] Handled taints on node in batch. --- pkg/controller/BUILD | 3 + pkg/controller/controller_utils.go | 123 ++++---- pkg/controller/controller_utils_test.go | 358 ++++++++++++++++++++++++ pkg/controller/node/controller_utils.go | 2 +- pkg/controller/node/node_controller.go | 4 +- test/e2e/framework/util.go | 2 +- 6 files changed, 440 insertions(+), 52 deletions(-) diff --git a/pkg/controller/BUILD b/pkg/controller/BUILD index 4d2d889d8ee..dc6dbfbac2b 100644 --- a/pkg/controller/BUILD +++ b/pkg/controller/BUILD @@ -18,7 +18,9 @@ go_test( tags = ["automanaged"], deps = [ "//pkg/api:go_default_library", + "//pkg/api/install:go_default_library", "//pkg/api/testapi:go_default_library", + "//pkg/controller/node/testutil:go_default_library", "//pkg/securitycontext:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/api/extensions/v1beta1:go_default_library", @@ -32,6 +34,7 @@ go_test( "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//vendor/k8s.io/client-go/kubernetes:go_default_library", + "//vendor/k8s.io/client-go/kubernetes/fake:go_default_library", "//vendor/k8s.io/client-go/rest:go_default_library", "//vendor/k8s.io/client-go/tools/cache:go_default_library", "//vendor/k8s.io/client-go/tools/record:go_default_library", diff --git a/pkg/controller/controller_utils.go b/pkg/controller/controller_utils.go index 4a6d6863dab..4833b09cf1a 100644 --- a/pkg/controller/controller_utils.go +++ b/pkg/controller/controller_utils.go @@ -885,50 +885,11 @@ 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 := taintutils.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. -// If passed a node it'll check if there's anything to be done, if taint is not present it won't issue -// any API calls. -func RemoveTaintOffNode(c clientset.Interface, nodeName string, taint *v1.Taint, node *v1.Node) error { - // Short circuit for limiting amount of API calls. - if node != nil { - match := false - for i := range node.Spec.Taints { - if node.Spec.Taints[i].MatchTaint(taint) { - match = true - break - } - } - if !match { - return nil - } +// AddOrUpdateTaintOnNode add taints to the node. If taint was added into node, it'll issue API calls +// to update nodes; otherwise, no API calls. Return error if any. +func AddOrUpdateTaintOnNode(c clientset.Interface, nodeName string, taints ...*v1.Taint) error { + if len(taints) == 0 { + return nil } firstTry := true return clientretry.RetryOnConflict(UpdateTaintBackoff, func() error { @@ -945,11 +906,77 @@ func RemoveTaintOffNode(c clientset.Interface, nodeName string, taint *v1.Taint, if err != nil { return err } - newNode, ok, err := taintutils.RemoveTaint(oldNode, taint) - if err != nil { - return fmt.Errorf("Failed to update taint annotation!") + + var newNode *v1.Node + oldNodeCopy := oldNode + updated := false + for _, taint := range taints { + curNewNode, ok, err := taintutils.AddOrUpdateTaint(oldNodeCopy, taint) + if err != nil { + return fmt.Errorf("Failed to update taint of node!") + } + updated = updated || ok + newNode = curNewNode + oldNodeCopy = curNewNode } - if !ok { + if !updated { + 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. +// If passed a node it'll check if there's anything to be done, if taint is not present it won't issue +// any API calls. +func RemoveTaintOffNode(c clientset.Interface, nodeName string, node *v1.Node, taints ...*v1.Taint) error { + if len(taints) == 0 { + return nil + } + // Short circuit for limiting amount of API calls. + if node != nil { + match := false + for _, taint := range taints { + if taintutils.TaintExists(node.Spec.Taints, taint) { + match = true + break + } + } + if !match { + return nil + } + } + + 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 + } + + var newNode *v1.Node + oldNodeCopy := oldNode + updated := false + for _, taint := range taints { + curNewNode, ok, err := taintutils.RemoveTaint(oldNodeCopy, taint) + if err != nil { + return fmt.Errorf("Failed to remove taint of node!") + } + updated = updated || ok + newNode = curNewNode + oldNodeCopy = curNewNode + } + if !updated { return nil } return PatchNodeTaints(c, nodeName, oldNode, newNode) diff --git a/pkg/controller/controller_utils_test.go b/pkg/controller/controller_utils_test.go index 0e4fdd3fa21..7abc0af277e 100644 --- a/pkg/controller/controller_utils_test.go +++ b/pkg/controller/controller_utils_test.go @@ -37,12 +37,15 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/uuid" clientset "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" restclient "k8s.io/client-go/rest" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" utiltesting "k8s.io/client-go/util/testing" "k8s.io/kubernetes/pkg/api" + _ "k8s.io/kubernetes/pkg/api/install" "k8s.io/kubernetes/pkg/api/testapi" + "k8s.io/kubernetes/pkg/controller/node/testutil" "k8s.io/kubernetes/pkg/securitycontext" ) @@ -479,3 +482,358 @@ func TestComputeHash(t *testing.T) { } } } + +func TestRemoveTaintOffNode(t *testing.T) { + tests := []struct { + name string + nodeHandler *testutil.FakeNodeHandler + nodeName string + taintsToRemove []*v1.Taint + expectedTaints []v1.Taint + requestCount int + }{ + { + name: "remove one taint from node", + nodeHandler: &testutil.FakeNodeHandler{ + Existing: []*v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + Taints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + }, + }, + }, + Clientset: fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testutil.NewPod("pod0", "node0")}}), + }, + nodeName: "node1", + taintsToRemove: []*v1.Taint{ + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + expectedTaints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + }, + requestCount: 4, + }, + { + name: "remove multiple taints from node", + nodeHandler: &testutil.FakeNodeHandler{ + Existing: []*v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + Taints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + {Key: "key3", Value: "value3", Effect: "NoSchedule"}, + {Key: "key4", Value: "value4", Effect: "NoExecute"}, + }, + }, + }, + }, + Clientset: fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testutil.NewPod("pod0", "node0")}}), + }, + nodeName: "node1", + taintsToRemove: []*v1.Taint{ + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + {Key: "key3", Value: "value3", Effect: "NoSchedule"}, + }, + expectedTaints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key4", Value: "value4", Effect: "NoExecute"}, + }, + requestCount: 4, + }, + { + name: "remove no-exist taints from node", + nodeHandler: &testutil.FakeNodeHandler{ + Existing: []*v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + Taints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + }, + }, + }, + Clientset: fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testutil.NewPod("pod0", "node0")}}), + }, + nodeName: "node1", + taintsToRemove: []*v1.Taint{ + {Key: "key3", Value: "value3", Effect: "NoSchedule"}, + }, + expectedTaints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + requestCount: 2, + }, + { + name: "remove taint from node without taints", + nodeHandler: &testutil.FakeNodeHandler{ + Existing: []*v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + }, + }, + Clientset: fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testutil.NewPod("pod0", "node0")}}), + }, + nodeName: "node1", + taintsToRemove: []*v1.Taint{ + {Key: "key3", Value: "value3", Effect: "NoSchedule"}, + }, + expectedTaints: nil, + requestCount: 2, + }, + { + name: "remove empty taint list from node without taints", + nodeHandler: &testutil.FakeNodeHandler{ + Existing: []*v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + }, + }, + Clientset: fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testutil.NewPod("pod0", "node0")}}), + }, + nodeName: "node1", + taintsToRemove: []*v1.Taint{}, + expectedTaints: nil, + requestCount: 2, + }, + { + name: "remove empty taint list from node", + nodeHandler: &testutil.FakeNodeHandler{ + Existing: []*v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + Taints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + }, + }, + }, + Clientset: fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testutil.NewPod("pod0", "node0")}}), + }, + nodeName: "node1", + taintsToRemove: []*v1.Taint{}, + expectedTaints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + requestCount: 2, + }, + } + for _, test := range tests { + node, _ := test.nodeHandler.Get(test.nodeName, metav1.GetOptions{}) + if err := RemoveTaintOffNode(test.nodeHandler, test.nodeName, node, test.taintsToRemove...); err != nil { + t.Errorf("%s: RemoveTaintOffNode() error = %v", test.name, err) + } + + node, _ = test.nodeHandler.Get(test.nodeName, metav1.GetOptions{}) + if !reflect.DeepEqual(node.Spec.Taints, test.expectedTaints) { + t.Errorf("%s: failed to remove taint off node: expected %+v, got %+v", + test.name, test.expectedTaints, node.Spec.Taints) + } + + if test.nodeHandler.RequestCount != test.requestCount { + t.Errorf("%s: unexpected request count: expected %+v, got %+v", + test.name, test.requestCount, test.nodeHandler.RequestCount) + } + } +} + +func TestAddOrUpdateTaintOnNode(t *testing.T) { + tests := []struct { + name string + nodeHandler *testutil.FakeNodeHandler + nodeName string + taintsToAdd []*v1.Taint + expectedTaints []v1.Taint + requestCount int + }{ + { + name: "add one taint on node", + nodeHandler: &testutil.FakeNodeHandler{ + Existing: []*v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + Taints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + }, + }, + }, + }, + Clientset: fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testutil.NewPod("pod0", "node0")}}), + }, + nodeName: "node1", + taintsToAdd: []*v1.Taint{ + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + expectedTaints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + requestCount: 3, + }, + { + name: "add multiple taints to node", + nodeHandler: &testutil.FakeNodeHandler{ + Existing: []*v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + Taints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + }, + }, + }, + Clientset: fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testutil.NewPod("pod0", "node0")}}), + }, + nodeName: "node1", + taintsToAdd: []*v1.Taint{ + {Key: "key3", Value: "value3", Effect: "NoSchedule"}, + {Key: "key4", Value: "value4", Effect: "NoExecute"}, + }, + expectedTaints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + {Key: "key3", Value: "value3", Effect: "NoSchedule"}, + {Key: "key4", Value: "value4", Effect: "NoExecute"}, + }, + requestCount: 3, + }, + { + name: "add exist taints to node", + nodeHandler: &testutil.FakeNodeHandler{ + Existing: []*v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + Taints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + }, + }, + }, + Clientset: fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testutil.NewPod("pod0", "node0")}}), + }, + nodeName: "node1", + taintsToAdd: []*v1.Taint{ + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + expectedTaints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + requestCount: 3, + }, + { + name: "add taint to node without taints", + nodeHandler: &testutil.FakeNodeHandler{ + Existing: []*v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + }, + }, + Clientset: fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testutil.NewPod("pod0", "node0")}}), + }, + nodeName: "node1", + taintsToAdd: []*v1.Taint{ + {Key: "key3", Value: "value3", Effect: "NoSchedule"}, + }, + expectedTaints: []v1.Taint{ + {Key: "key3", Value: "value3", Effect: "NoSchedule"}, + }, + requestCount: 3, + }, + { + name: "add empty taint list to node without taints", + nodeHandler: &testutil.FakeNodeHandler{ + Existing: []*v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + }, + }, + Clientset: fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testutil.NewPod("pod0", "node0")}}), + }, + nodeName: "node1", + taintsToAdd: []*v1.Taint{}, + expectedTaints: nil, + requestCount: 1, + }, + { + name: "add empty taint list to node", + nodeHandler: &testutil.FakeNodeHandler{ + Existing: []*v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + Taints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + }, + }, + }, + Clientset: fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testutil.NewPod("pod0", "node0")}}), + }, + nodeName: "node1", + taintsToAdd: []*v1.Taint{}, + expectedTaints: []v1.Taint{ + {Key: "key1", Value: "value1", Effect: "NoSchedule"}, + {Key: "key2", Value: "value2", Effect: "NoExecute"}, + }, + requestCount: 1, + }, + } + for _, test := range tests { + if err := AddOrUpdateTaintOnNode(test.nodeHandler, test.nodeName, test.taintsToAdd...); err != nil { + t.Errorf("%s: AddOrUpdateTaintOnNode() error = %v", test.name, err) + } + + node, _ := test.nodeHandler.Get(test.nodeName, metav1.GetOptions{}) + if !reflect.DeepEqual(node.Spec.Taints, test.expectedTaints) { + t.Errorf("%s: failed to add taint to node: expected %+v, got %+v", + test.name, test.expectedTaints, node.Spec.Taints) + } + + if test.nodeHandler.RequestCount != test.requestCount { + t.Errorf("%s: unexpected request count: expected %+v, got %+v", + test.name, test.requestCount, test.nodeHandler.RequestCount) + } + } +} diff --git a/pkg/controller/node/controller_utils.go b/pkg/controller/node/controller_utils.go index 035dfdc6f9a..9b5666d5861 100644 --- a/pkg/controller/node/controller_utils.go +++ b/pkg/controller/node/controller_utils.go @@ -303,7 +303,7 @@ func swapNodeControllerTaint(kubeClient clientset.Interface, taintToAdd, taintTo } glog.V(4).Infof("Added %v Taint to Node %v", taintToAdd, node.Name) - err = controller.RemoveTaintOffNode(kubeClient, node.Name, taintToRemove, node) + err = controller.RemoveTaintOffNode(kubeClient, node.Name, node, taintToRemove) if err != nil { utilruntime.HandleError( fmt.Errorf( diff --git a/pkg/controller/node/node_controller.go b/pkg/controller/node/node_controller.go index 1df7d2d2c3e..5a2fa6f89f0 100644 --- a/pkg/controller/node/node_controller.go +++ b/pkg/controller/node/node_controller.go @@ -1072,12 +1072,12 @@ func (nc *NodeController) markNodeForTainting(node *v1.Node) bool { func (nc *NodeController) markNodeAsHealthy(node *v1.Node) (bool, error) { nc.evictorLock.Lock() defer nc.evictorLock.Unlock() - err := controller.RemoveTaintOffNode(nc.kubeClient, node.Name, UnreachableTaintTemplate, node) + err := controller.RemoveTaintOffNode(nc.kubeClient, node.Name, node, UnreachableTaintTemplate) if err != nil { glog.Errorf("Failed to remove taint from node %v: %v", node.Name, err) return false, err } - err = controller.RemoveTaintOffNode(nc.kubeClient, node.Name, NotReadyTaintTemplate, node) + err = controller.RemoveTaintOffNode(nc.kubeClient, node.Name, node, NotReadyTaintTemplate) if err != nil { glog.Errorf("Failed to remove taint from node %v: %v", node.Name, err) return false, err diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index 1a9130414b0..0b6455f83c3 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -2551,7 +2551,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, nil)) + ExpectNoError(controller.RemoveTaintOffNode(c, nodeName, nil, &taint)) VerifyThatTaintIsGone(c, nodeName, &taint) }