From f56e62ca14d87efea6522b49cdf0dd913acbdee8 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Fri, 3 Mar 2023 11:52:53 -0800 Subject: [PATCH 1/3] Add helper funcs to clean up svc controller tests --- .../controllers/service/controller_test.go | 1771 +++-------------- 1 file changed, 295 insertions(+), 1476 deletions(-) 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 77f7050cf59..dc3c931e0c5 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 @@ -2058,6 +2058,90 @@ func TestListWithPredicate(t *testing.T) { } } +type nodeTweak func(n *v1.Node) + +// TODO: use this pattern in all the tests above. +func makeNode(tweaks ...nodeTweak) *v1.Node { + n := &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node", + Labels: map[string]string{}, + }, + Spec: v1.NodeSpec{ + Taints: []v1.Taint{}, + }, + Status: v1.NodeStatus{ + Conditions: []v1.NodeCondition{{ + Type: v1.NodeReady, + Status: v1.ConditionTrue, + }}, + }, + } + + for _, tw := range tweaks { + tw(n) + } + return n +} + +func tweakAddTaint(key string) nodeTweak { + return func(n *v1.Node) { + n.Spec.Taints = append(n.Spec.Taints, v1.Taint{Key: key}) + } +} + +func tweakSetLabel(key, val string) nodeTweak { + return func(n *v1.Node) { + n.Labels[key] = val + } +} + +func tweakSetCondition(condType v1.NodeConditionType, condStatus v1.ConditionStatus) nodeTweak { + return func(n *v1.Node) { + var cond *v1.NodeCondition + for i := range n.Status.Conditions { + c := &n.Status.Conditions[i] + if c.Type == condType { + cond = c + break + } + } + if cond == nil { + n.Status.Conditions = append(n.Status.Conditions, v1.NodeCondition{}) + cond = &n.Status.Conditions[len(n.Status.Conditions)-1] + } + *cond = v1.NodeCondition{ + Type: condType, + Status: condStatus, + } + } +} + +func tweakSetReady(val bool) nodeTweak { + var condStatus v1.ConditionStatus + + if val { + condStatus = v1.ConditionTrue + } else { + condStatus = v1.ConditionFalse + } + + return tweakSetCondition(v1.NodeReady, condStatus) +} + +func tweakUnsetCondition(condType v1.NodeConditionType) nodeTweak { + return func(n *v1.Node) { + for i := range n.Status.Conditions { + c := &n.Status.Conditions[i] + if c.Type == condType { + // Hacky but easy. + c.Type = "SomethingElse" + break + } + } + } +} + func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) { testcases := []struct { name string @@ -2065,984 +2149,156 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) { newNode *v1.Node shouldSync bool stableNodeSetEnabled bool - }{ - { - name: "taint F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: true, - }, - { - name: "taint T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: true, - }, - { - name: "taint F->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "taint T->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "other taint F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: "other", - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "other taint T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: "other", - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "excluded F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: true, - }, - { - name: "excluded changed T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: true, - }, - { - name: "excluded changed T->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "excluded changed F->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "excluded F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: true, - stableNodeSetEnabled: true, - }, - { - name: "excluded changed T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: true, - stableNodeSetEnabled: true, - }, - { - name: "excluded changed T->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - stableNodeSetEnabled: true, - }, - { - name: "excluded changed F->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - stableNodeSetEnabled: true, - }, - { - name: "other label changed F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - "other": "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "other label changed T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - "other": "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "readiness changed F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: true, - }, - { - name: "readiness changed T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - shouldSync: true, - }, - { - name: "readiness changed T->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "readiness changed F->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "readiness changed F->unset", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{}, - }, - }, - shouldSync: false, - }, - { - name: "readiness changed T->unset", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{}, - }, - }, - shouldSync: true, - }, - { - name: "readiness changed unset->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{}, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "readiness changed unset->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{}, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: true, - }, - { - name: "readiness changed unset->unset", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{}, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{}, - }, - }, - shouldSync: false, - }, - { - name: "ready F, other condition changed F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeDiskPressure, - Status: v1.ConditionFalse, - }, - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeDiskPressure, - Status: v1.ConditionTrue, - }, - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "ready F, other condition changed T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeDiskPressure, - Status: v1.ConditionTrue, - }, - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeDiskPressure, - Status: v1.ConditionFalse, - }, - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "ready T, other condition changed F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeDiskPressure, - Status: v1.ConditionFalse, - }, - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeDiskPressure, - Status: v1.ConditionTrue, - }, - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "ready T, other condition changed T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeDiskPressure, - Status: v1.ConditionTrue, - }, - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeDiskPressure, - Status: v1.ConditionFalse, - }, - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - } + }{{ + name: "taint F->T", + oldNode: makeNode(), + newNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + shouldSync: true, + }, { + name: "taint T->F", + oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(), + shouldSync: true, + }, { + name: "taint F->F", + oldNode: makeNode(), + newNode: makeNode(), + shouldSync: false, + }, { + name: "taint T->T", + oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + shouldSync: false, + }, { + name: "other taint F->T", + oldNode: makeNode(), + newNode: makeNode(tweakAddTaint("other")), + shouldSync: false, + }, { + name: "other taint T->F", + oldNode: makeNode(tweakAddTaint("other")), + newNode: makeNode(), + shouldSync: false, + }, { + name: "excluded F->T", + oldNode: makeNode(), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: true, + }, { + name: "excluded changed T->F", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(), + shouldSync: true, + }, { + name: "excluded changed T->T", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: false, + }, { + name: "excluded changed F->F", + oldNode: makeNode(), + newNode: makeNode(), + shouldSync: false, + }, { + name: "excluded F->T", + oldNode: makeNode(), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: true, + stableNodeSetEnabled: true, + }, { + name: "excluded changed T->F", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(), + shouldSync: true, + stableNodeSetEnabled: true, + }, { + name: "excluded changed T->T", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: false, + stableNodeSetEnabled: true, + }, { + name: "excluded changed F->F", + oldNode: makeNode(), + newNode: makeNode(), + shouldSync: false, + stableNodeSetEnabled: true, + }, { + name: "other label changed F->T", + oldNode: makeNode(), + newNode: makeNode(tweakSetLabel("other", "")), + shouldSync: false, + }, { + name: "other label changed T->F", + oldNode: makeNode(tweakSetLabel("other", "")), + newNode: makeNode(), + shouldSync: false, + }, { + name: "readiness changed F->T", + oldNode: makeNode(tweakSetReady(false)), + newNode: makeNode(), + shouldSync: true, + }, { + name: "readiness changed T->F", + oldNode: makeNode(), + newNode: makeNode(tweakSetReady(false)), + shouldSync: true, + }, { + name: "readiness changed T->T", + oldNode: makeNode(), + newNode: makeNode(), + shouldSync: false, + }, { + name: "readiness changed F->F", + oldNode: makeNode(tweakSetReady(false)), + newNode: makeNode(tweakSetReady(false)), + shouldSync: false, + }, { + name: "readiness changed F->unset", + oldNode: makeNode(tweakSetReady(false)), + newNode: makeNode(tweakUnsetCondition(v1.NodeReady)), + shouldSync: false, + }, { + name: "readiness changed T->unset", + oldNode: makeNode(), + newNode: makeNode(tweakUnsetCondition(v1.NodeReady)), + shouldSync: true, + }, { + name: "readiness changed unset->F", + oldNode: makeNode(tweakUnsetCondition(v1.NodeReady)), + newNode: makeNode(tweakSetReady(false)), + shouldSync: false, + }, { + name: "readiness changed unset->T", + oldNode: makeNode(tweakUnsetCondition(v1.NodeReady)), + newNode: makeNode(), + shouldSync: true, + }, { + name: "readiness changed unset->unset", + oldNode: makeNode(tweakUnsetCondition(v1.NodeReady)), + newNode: makeNode(tweakUnsetCondition(v1.NodeReady)), + shouldSync: false, + }, { + name: "ready F, other condition changed F->T", + oldNode: makeNode(tweakSetReady(false), tweakSetCondition(v1.NodeDiskPressure, v1.ConditionFalse)), + newNode: makeNode(tweakSetReady(false), tweakSetCondition(v1.NodeDiskPressure, v1.ConditionTrue)), + shouldSync: false, + }, { + name: "ready F, other condition changed T->F", + oldNode: makeNode(tweakSetReady(false), tweakSetCondition(v1.NodeDiskPressure, v1.ConditionTrue)), + newNode: makeNode(tweakSetReady(false), tweakSetCondition(v1.NodeDiskPressure, v1.ConditionFalse)), + shouldSync: false, + }, { + name: "ready T, other condition changed F->T", + oldNode: makeNode(tweakSetCondition("Other", v1.ConditionFalse)), + newNode: makeNode(tweakSetCondition("Other", v1.ConditionTrue)), + shouldSync: false, + }, { + name: "ready T, other condition changed T->F", + oldNode: makeNode(tweakSetCondition("Other", v1.ConditionTrue)), + newNode: makeNode(tweakSetCondition("Other", v1.ConditionFalse)), + shouldSync: false, + }} for _, testcase := range testcases { t.Run(fmt.Sprintf("%s - StableLoadBalancerNodeSet: %v", testcase.name, testcase.stableNodeSetEnabled), func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StableLoadBalancerNodeSet, testcase.stableNodeSetEnabled)() @@ -3062,504 +2318,67 @@ func Test_shouldSyncUpdatedNode_compoundedPredicates(t *testing.T) { oldNode *v1.Node newNode *v1.Node shouldSync bool - }{ - { - name: "tainted T, excluded F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: true, - }, - { - name: "tainted T, excluded T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: true, - }, - { - name: "tainted T, ready F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "tainted T, ready T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "excluded T, tainted F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "excluded T, tainted T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "excluded T, ready F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "excluded T, ready T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionTrue, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "ready F, tainted F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "ready F, tainted T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: ToBeDeletedTaint, - }, - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - shouldSync: false, - }, - { - name: "ready F, excluded F->T", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - shouldSync: true, - }, - { - name: "ready F, excluded T->F", - oldNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{ - v1.LabelNodeExcludeBalancers: "", - }, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - newNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node", - Labels: map[string]string{}, - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionFalse, - }, - }, - }, - }, - shouldSync: true, - }, - } + }{{ + name: "tainted T, excluded F->T", + oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(tweakAddTaint(ToBeDeletedTaint), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: true, + }, { + name: "tainted T, excluded T->F", + oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + shouldSync: true, + }, { + name: "tainted T, ready F->T", + oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint), tweakSetReady(false)), + newNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + shouldSync: false, + }, { + name: "tainted T, ready T->F", + oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(tweakAddTaint(ToBeDeletedTaint), tweakSetReady(false)), + shouldSync: false, + }, { + name: "excluded T, tainted F->T", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakAddTaint(ToBeDeletedTaint)), + shouldSync: false, + }, { + name: "excluded T, tainted T->F", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: false, + }, { + name: "excluded T, ready F->T", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakSetReady(false)), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: false, + }, { + name: "excluded T, ready T->F", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakSetReady(false)), + shouldSync: false, + }, { + name: "ready F, tainted F->T", + oldNode: makeNode(tweakSetReady(false)), + newNode: makeNode(tweakSetReady(false), tweakAddTaint(ToBeDeletedTaint)), + shouldSync: false, + }, { + name: "ready F, tainted T->F", + oldNode: makeNode(tweakSetReady(false), tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(tweakSetReady(false)), + shouldSync: false, + }, { + name: "ready F, excluded F->T", + oldNode: makeNode(tweakSetReady(false)), + newNode: makeNode(tweakSetReady(false), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: true, + }, { + name: "ready F, excluded T->F", + oldNode: makeNode(tweakSetReady(false), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(tweakSetReady(false)), + shouldSync: true, + }} for _, testcase := range testcases { t.Run(fmt.Sprintf("%s - StableLoadBalancerNodeSet: %v", testcase.name, fgEnabled), func(t *testing.T) { shouldSync := shouldSyncUpdatedNode(testcase.oldNode, testcase.newNode) From 0ccab9804a6d47386c27581b7a37bcaff3d418c4 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Fri, 3 Mar 2023 11:56:25 -0800 Subject: [PATCH 2/3] Remove obviously redundant tests --- .../controllers/service/controller_test.go | 32 +++++++------------ 1 file changed, 11 insertions(+), 21 deletions(-) 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 dc3c931e0c5..465c92b1de9 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 @@ -2150,6 +2150,17 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) { shouldSync bool stableNodeSetEnabled bool }{{ + name: "nothing changed", + oldNode: makeNode(), + newNode: makeNode(), + shouldSync: false, + }, { + name: "nothing changed", + oldNode: makeNode(), + newNode: makeNode(), + shouldSync: false, + stableNodeSetEnabled: true, + }, { name: "taint F->T", oldNode: makeNode(), newNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), @@ -2159,11 +2170,6 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) { oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), newNode: makeNode(), shouldSync: true, - }, { - name: "taint F->F", - oldNode: makeNode(), - newNode: makeNode(), - shouldSync: false, }, { name: "taint T->T", oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), @@ -2194,11 +2200,6 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) { oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), shouldSync: false, - }, { - name: "excluded changed F->F", - oldNode: makeNode(), - newNode: makeNode(), - shouldSync: false, }, { name: "excluded F->T", oldNode: makeNode(), @@ -2217,12 +2218,6 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) { newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), shouldSync: false, stableNodeSetEnabled: true, - }, { - name: "excluded changed F->F", - oldNode: makeNode(), - newNode: makeNode(), - shouldSync: false, - stableNodeSetEnabled: true, }, { name: "other label changed F->T", oldNode: makeNode(), @@ -2243,11 +2238,6 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) { oldNode: makeNode(), newNode: makeNode(tweakSetReady(false)), shouldSync: true, - }, { - name: "readiness changed T->T", - oldNode: makeNode(), - newNode: makeNode(), - shouldSync: false, }, { name: "readiness changed F->F", oldNode: makeNode(tweakSetReady(false)), From 95df201d368ca3abfc18c42802ef15806d422ade Mon Sep 17 00:00:00 2001 From: Alexander Constantinescu Date: Mon, 13 Mar 2023 12:01:06 +0100 Subject: [PATCH 3/3] [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) {