From 95df201d368ca3abfc18c42802ef15806d422ade Mon Sep 17 00:00:00 2001 From: Alexander Constantinescu Date: Mon, 13 Mar 2023 12:01:06 +0100 Subject: [PATCH] [KCCM]: service controller - Add node.deletionTimestamp predicate --- .../controllers/service/controller.go | 5 ++ .../controllers/service/controller_test.go | 68 +++++++++++++++---- 2 files changed, 59 insertions(+), 14 deletions(-) diff --git a/staging/src/k8s.io/cloud-provider/controllers/service/controller.go b/staging/src/k8s.io/cloud-provider/controllers/service/controller.go index d470fef7de8..7aa76bf19c8 100644 --- a/staging/src/k8s.io/cloud-provider/controllers/service/controller.go +++ b/staging/src/k8s.io/cloud-provider/controllers/service/controller.go @@ -943,6 +943,7 @@ var ( nodeUnTaintedPredicate, } stableNodeSetPredicates []NodeConditionPredicate = []NodeConditionPredicate{ + nodeNotDeletedPredicate, nodeIncludedPredicate, // This is not perfect, but probably good enough. We won't update the // LBs just because the taint was added (see shouldSyncUpdatedNode) but @@ -989,6 +990,10 @@ func nodeReadyPredicate(node *v1.Node) bool { return false } +func nodeNotDeletedPredicate(node *v1.Node) bool { + return node.DeletionTimestamp.IsZero() +} + // listWithPredicate gets nodes that matches all predicate functions. func listWithPredicates(nodeLister corelisters.NodeLister, predicates ...NodeConditionPredicate) ([]*v1.Node, error) { nodes, err := nodeLister.List(labels.Everything()) diff --git a/staging/src/k8s.io/cloud-provider/controllers/service/controller_test.go b/staging/src/k8s.io/cloud-provider/controllers/service/controller_test.go index 465c92b1de9..dc99ad2c1ec 100644 --- a/staging/src/k8s.io/cloud-provider/controllers/service/controller_test.go +++ b/staging/src/k8s.io/cloud-provider/controllers/service/controller_test.go @@ -578,13 +578,13 @@ func TestUpdateNodesInExternalLoadBalancer(t *testing.T) { func TestNodeChangesForExternalTrafficPolicyLocalServices(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StableLoadBalancerNodeSet, false)() - node1 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}} - node2 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node2"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}} - node2NotReady := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node2"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionFalse}}}} - node2Tainted := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node2"}, Spec: v1.NodeSpec{Taints: []v1.Taint{{Key: ToBeDeletedTaint}}}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionFalse}}}} - node2SpuriousChange := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node2"}, Status: v1.NodeStatus{Phase: v1.NodeTerminated, Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}} - node2Exclude := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node2", Labels: map[string]string{v1.LabelNodeExcludeBalancers: ""}}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}} - node3 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node3"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}} + node1 := makeNode(tweakName("node1"), tweakSetCondition(v1.NodeReady, v1.ConditionTrue)) + node2 := makeNode(tweakName("node2"), tweakSetCondition(v1.NodeReady, v1.ConditionTrue)) + node3 := makeNode(tweakName("node3"), tweakSetCondition(v1.NodeReady, v1.ConditionTrue)) + node2NotReady := makeNode(tweakName("node2"), tweakSetCondition(v1.NodeReady, v1.ConditionFalse)) + node2Tainted := makeNode(tweakName("node2"), tweakAddTaint(ToBeDeletedTaint), tweakSetCondition(v1.NodeReady, v1.ConditionTrue)) + node2SpuriousChange := makeNode(tweakName("node2"), tweakAddTaint("Other"), tweakSetCondition(v1.NodeReady, v1.ConditionTrue)) + node2Exclude := makeNode(tweakName("node2"), tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakSetCondition(v1.NodeReady, v1.ConditionTrue)) type stateChanges struct { nodes []*v1.Node @@ -750,13 +750,14 @@ func TestNodeChangesForExternalTrafficPolicyLocalServices(t *testing.T) { } func TestNodeChangesForStableNodeSetEnabled(t *testing.T) { - node1 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node0"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}} - node2 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}} - node2NotReady := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionFalse}}}} - node2Tainted := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Spec: v1.NodeSpec{Taints: []v1.Taint{{Key: ToBeDeletedTaint}}}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionFalse}}}} - node2SpuriousChange := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: v1.NodeStatus{Phase: v1.NodeTerminated, Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}} - node2Exclude := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1", Labels: map[string]string{v1.LabelNodeExcludeBalancers: ""}}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}} - node3 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node73"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}} + node1 := makeNode(tweakName("node1"), tweakSetCondition(v1.NodeReady, v1.ConditionTrue)) + node2 := makeNode(tweakName("node2"), tweakSetCondition(v1.NodeReady, v1.ConditionTrue)) + node3 := makeNode(tweakName("node3"), tweakSetCondition(v1.NodeReady, v1.ConditionTrue)) + node2NotReady := makeNode(tweakName("node2"), tweakSetCondition(v1.NodeReady, v1.ConditionFalse)) + node2Tainted := makeNode(tweakName("node2"), tweakAddTaint(ToBeDeletedTaint), tweakSetCondition(v1.NodeReady, v1.ConditionTrue)) + node2SpuriousChange := makeNode(tweakName("node2"), tweakAddTaint("Other"), tweakSetCondition(v1.NodeReady, v1.ConditionTrue)) + node2Exclude := makeNode(tweakName("node2"), tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakSetCondition(v1.NodeReady, v1.ConditionTrue)) + node2Deleted := makeNode(tweakName("node2"), tweakDeleted()) type stateChanges struct { nodes []*v1.Node @@ -879,6 +880,20 @@ func TestNodeChangesForStableNodeSetEnabled(t *testing.T) { {Service: service3, Hosts: []*v1.Node{node1, node2}}, }, }, + { + desc: "1 node marked for deletion", + initialState: []*v1.Node{node1, node2, node3}, + stateChanges: []stateChanges{ + { + nodes: []*v1.Node{node1, node2Deleted, node3}, + }, + }, + expectedUpdateCalls: []fakecloud.UpdateBalancerCall{ + {Service: etpLocalservice1, Hosts: []*v1.Node{node1, node3}}, + {Service: etpLocalservice2, Hosts: []*v1.Node{node1, node3}}, + {Service: service3, Hosts: []*v1.Node{node1, node3}}, + }, + }, { desc: "1 spurious node update", initialState: []*v1.Node{node1, node2, node3}, @@ -2084,6 +2099,12 @@ func makeNode(tweaks ...nodeTweak) *v1.Node { return n } +func tweakName(name string) nodeTweak { + return func(n *v1.Node) { + n.Name = name + } +} + func tweakAddTaint(key string) nodeTweak { return func(n *v1.Node) { n.Spec.Taints = append(n.Spec.Taints, v1.Taint{Key: key}) @@ -2142,6 +2163,14 @@ func tweakUnsetCondition(condType v1.NodeConditionType) nodeTweak { } } +func tweakDeleted() nodeTweak { + return func(n *v1.Node) { + n.DeletionTimestamp = &metav1.Time{ + Time: time.Now(), + } + } +} + func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) { testcases := []struct { name string @@ -2288,6 +2317,17 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) { oldNode: makeNode(tweakSetCondition("Other", v1.ConditionTrue)), newNode: makeNode(tweakSetCondition("Other", v1.ConditionFalse)), shouldSync: false, + }, { + name: "deletionTimestamp F -> T", + oldNode: makeNode(), + newNode: makeNode(tweakDeleted()), + shouldSync: false, + }, { + name: "deletionTimestamp F -> T", + oldNode: makeNode(), + newNode: makeNode(tweakDeleted()), + shouldSync: false, + stableNodeSetEnabled: true, }} for _, testcase := range testcases { t.Run(fmt.Sprintf("%s - StableLoadBalancerNodeSet: %v", testcase.name, testcase.stableNodeSetEnabled), func(t *testing.T) {