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 179b2d4e8f050..ef9259a032551 100644 --- a/staging/src/k8s.io/cloud-provider/controllers/service/controller.go +++ b/staging/src/k8s.io/cloud-provider/controllers/service/controller.go @@ -693,8 +693,8 @@ func shouldSyncUpdatedNode(oldNode, newNode *v1.Node) bool { if respectsPredicates(oldNode, nodeIncludedPredicate) != respectsPredicates(newNode, nodeIncludedPredicate) { return true } - // For the same reason as above, also check for changes to the providerID - if respectsPredicates(oldNode, nodeHasProviderIDPredicate) != respectsPredicates(newNode, nodeHasProviderIDPredicate) { + // For the same reason as above, also check for any change to the providerID + if oldNode.Spec.ProviderID != newNode.Spec.ProviderID { return true } if !utilfeature.DefaultFeatureGate.Enabled(features.StableLoadBalancerNodeSet) { @@ -958,17 +958,14 @@ var ( nodeIncludedPredicate, nodeUnTaintedPredicate, nodeReadyPredicate, - nodeHasProviderIDPredicate, } etpLocalNodePredicates []NodeConditionPredicate = []NodeConditionPredicate{ nodeIncludedPredicate, nodeUnTaintedPredicate, - nodeHasProviderIDPredicate, } stableNodeSetPredicates []NodeConditionPredicate = []NodeConditionPredicate{ nodeNotDeletedPredicate, nodeIncludedPredicate, - nodeHasProviderIDPredicate, // This is not perfect, but probably good enough. We won't update the // LBs just because the taint was added (see shouldSyncUpdatedNode) but // if any other situation causes an LB sync, tainted nodes will be @@ -994,10 +991,6 @@ func nodeIncludedPredicate(node *v1.Node) bool { return !hasExcludeBalancerLabel } -func nodeHasProviderIDPredicate(node *v1.Node) bool { - return node.Spec.ProviderID != "" -} - // We consider the node for load balancing only when its not tainted for deletion by the cluster autoscaler. func nodeUnTaintedPredicate(node *v1.Node) bool { for _, taint := range node.Spec.Taints { 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 76596edf9f74a..b37b023ab9916 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 @@ -1756,7 +1756,7 @@ func Test_respectsPredicates(t *testing.T) { }{ {want: false, input: &v1.Node{}}, {want: true, input: &v1.Node{Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}}, - {want: false, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}}, + {want: true, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}}, {want: false, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionFalse}}}}}, {want: true, input: &v1.Node{Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}, ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}}, {want: false, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}, ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{v1.LabelNodeExcludeBalancers: ""}}}}, @@ -1934,9 +1934,9 @@ func tweakDeleted() nodeTweak { } } -func tweakUnsetProviderID() nodeTweak { +func tweakProviderID(id string) nodeTweak { return func(n *v1.Node) { - n.Spec.ProviderID = "" + n.Spec.ProviderID = id } } @@ -2099,15 +2099,37 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) { stableNodeSetEnabled: true, }, { name: "providerID set F -> T", - oldNode: makeNode(tweakUnsetProviderID()), + oldNode: makeNode(tweakProviderID("")), newNode: makeNode(), shouldSync: true, }, { name: "providerID set F -> T", - oldNode: makeNode(tweakUnsetProviderID()), + oldNode: makeNode(tweakProviderID("")), newNode: makeNode(), shouldSync: true, stableNodeSetEnabled: true, + }, { + name: "providerID set T-> F", + oldNode: makeNode(), + newNode: makeNode(tweakProviderID("")), + shouldSync: true, + }, { + name: "providerID set T-> F", + oldNode: makeNode(), + newNode: makeNode(tweakProviderID("")), + shouldSync: true, + stableNodeSetEnabled: true, + }, { + name: "providerID change", + oldNode: makeNode(), + newNode: makeNode(tweakProviderID(providerID + "-2")), + shouldSync: true, + }, { + name: "providerID change", + oldNode: makeNode(), + newNode: makeNode(tweakProviderID(providerID + "-2")), + shouldSync: true, + stableNodeSetEnabled: true, }} for _, testcase := range testcases { t.Run(fmt.Sprintf("%s - StableLoadBalancerNodeSet: %v", testcase.name, testcase.stableNodeSetEnabled), func(t *testing.T) { @@ -2121,143 +2143,214 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) { } func Test_shouldSyncUpdatedNode_compoundedPredicates(t *testing.T) { + type testCase struct { + name string + oldNode *v1.Node + newNode *v1.Node + shouldSync bool + fgEnabled bool + } + testcases := []testCase{} for _, fgEnabled := range []bool{true, false} { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StableLoadBalancerNodeSet, fgEnabled)() - testcases := []struct { - name string - oldNode *v1.Node - newNode *v1.Node - shouldSync bool - }{{ - 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, providerID set F->T", - oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint), tweakUnsetProviderID()), - newNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), - shouldSync: true, - }, { - name: "tainted T, providerID set T->F", - oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), - newNode: makeNode(tweakAddTaint(ToBeDeletedTaint), tweakUnsetProviderID()), - 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)), + testcases = append(testcases, []testCase{ + { + name: "tainted T, excluded F->T", + oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(tweakAddTaint(ToBeDeletedTaint), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "tainted T, excluded T->F", + oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "tainted T, providerID set F->T", + oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint), tweakProviderID("")), + newNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "tainted T, providerID set T->F", + oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(tweakAddTaint(ToBeDeletedTaint), tweakProviderID("")), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "tainted T, providerID change", + oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(tweakAddTaint(ToBeDeletedTaint), tweakProviderID(providerID+"-2")), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "tainted T, ready F->T", + oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint), tweakSetReady(false)), + newNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + shouldSync: false, + fgEnabled: fgEnabled, + }, { + name: "tainted T, ready T->F", + oldNode: makeNode(tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(tweakAddTaint(ToBeDeletedTaint), tweakSetReady(false)), + shouldSync: false, + fgEnabled: fgEnabled, + }, { + name: "excluded T, tainted F->T", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakAddTaint(ToBeDeletedTaint)), + shouldSync: false, + fgEnabled: fgEnabled, + }, { + name: "excluded T, tainted T->F", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: false, + fgEnabled: fgEnabled, + }, { + name: "excluded T, ready F->T", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakSetReady(false)), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: false, + fgEnabled: fgEnabled, + }, { + name: "excluded T, ready T->F", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakSetReady(false)), + shouldSync: false, + fgEnabled: fgEnabled, + }, { + name: "excluded T, providerID set F->T", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakProviderID("")), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "excluded T, providerID set T->F", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakProviderID("")), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "excluded T, providerID change", + oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakProviderID(providerID+"-2")), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "ready F, tainted F->T", + oldNode: makeNode(tweakSetReady(false)), + newNode: makeNode(tweakSetReady(false), tweakAddTaint(ToBeDeletedTaint)), + shouldSync: false, + fgEnabled: fgEnabled, + }, { + name: "ready F, tainted T->F", + oldNode: makeNode(tweakSetReady(false), tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(tweakSetReady(false)), + shouldSync: false, + fgEnabled: fgEnabled, + }, { + name: "ready F, excluded F->T", + oldNode: makeNode(tweakSetReady(false)), + newNode: makeNode(tweakSetReady(false), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "ready F, excluded T->F", + oldNode: makeNode(tweakSetReady(false), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(tweakSetReady(false)), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "ready F, providerID set F->T", + oldNode: makeNode(tweakSetReady(false), tweakProviderID("")), + newNode: makeNode(tweakSetReady(false)), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "ready F, providerID set T->F", + oldNode: makeNode(tweakSetReady(false)), + newNode: makeNode(tweakSetReady(false), tweakProviderID("")), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "ready F, providerID change", + oldNode: makeNode(tweakSetReady(false)), + newNode: makeNode(tweakSetReady(false), tweakProviderID(providerID+"-2")), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "providerID unset, excluded F->T", + oldNode: makeNode(tweakProviderID("")), + newNode: makeNode(tweakProviderID(""), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "providerID unset, excluded T->F", + oldNode: makeNode(tweakProviderID(""), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), + newNode: makeNode(tweakProviderID("")), + shouldSync: true, + fgEnabled: fgEnabled, + }, { + name: "providerID unset, ready T->F", + oldNode: makeNode(tweakProviderID("")), + newNode: makeNode(tweakProviderID(""), tweakSetReady(true)), + shouldSync: false, + fgEnabled: fgEnabled, + }, + }...) + } + testcases = append(testcases, []testCase{ + { + name: "providerID unset, ready F->T", + oldNode: makeNode(tweakProviderID("")), + newNode: makeNode(tweakProviderID(""), tweakSetReady(false)), shouldSync: false, - }, { - name: "excluded T, providerID set F->T", - oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakUnsetProviderID()), - newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), - shouldSync: true, - }, { - name: "excluded T, providerID set T->F", - oldNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), - newNode: makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), tweakUnsetProviderID()), + fgEnabled: true, + }, + { + name: "providerID unset, ready F->T", + oldNode: makeNode(tweakProviderID("")), + newNode: makeNode(tweakProviderID(""), tweakSetReady(false)), shouldSync: true, + fgEnabled: 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)), + name: "providerID unset, tainted T->F", + oldNode: makeNode(tweakProviderID(""), tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(tweakProviderID("")), 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, - }, { - name: "ready F, providerID set F->T", - oldNode: makeNode(tweakSetReady(false), tweakUnsetProviderID()), - newNode: makeNode(tweakSetReady(false)), - shouldSync: true, - }, { - name: "ready F, providerID set T->F", - oldNode: makeNode(tweakSetReady(false)), - newNode: makeNode(tweakSetReady(false), tweakUnsetProviderID()), + fgEnabled: true, + }, + { + name: "providerID unset, tainted T->F", + oldNode: makeNode(tweakProviderID(""), tweakAddTaint(ToBeDeletedTaint)), + newNode: makeNode(tweakProviderID("")), shouldSync: true, + fgEnabled: false, }, { name: "providerID unset, tainted F->T", - oldNode: makeNode(tweakUnsetProviderID()), - newNode: makeNode(tweakUnsetProviderID(), tweakAddTaint(ToBeDeletedTaint)), - shouldSync: false, - }, { - name: "providerID unset, tainted T->F", - oldNode: makeNode(tweakUnsetProviderID(), tweakAddTaint(ToBeDeletedTaint)), - newNode: makeNode(tweakUnsetProviderID()), + oldNode: makeNode(tweakProviderID("")), + newNode: makeNode(tweakProviderID(""), tweakAddTaint(ToBeDeletedTaint)), shouldSync: false, + fgEnabled: true, }, { - name: "providerID unset, excluded F->T", - oldNode: makeNode(tweakUnsetProviderID()), - newNode: makeNode(tweakUnsetProviderID(), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), - shouldSync: true, - }, { - name: "providerID unset, excluded T->F", - oldNode: makeNode(tweakUnsetProviderID(), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")), - newNode: makeNode(tweakUnsetProviderID()), + name: "providerID unset, tainted F->T", + oldNode: makeNode(tweakProviderID("")), + newNode: makeNode(tweakProviderID(""), tweakAddTaint(ToBeDeletedTaint)), shouldSync: true, - }, { - name: "providerID unset, ready F->T", - oldNode: makeNode(tweakUnsetProviderID()), - newNode: makeNode(tweakUnsetProviderID(), tweakSetReady(false)), - shouldSync: false, - }, { - name: "providerID unset, ready T->F", - oldNode: makeNode(tweakUnsetProviderID()), - newNode: makeNode(tweakUnsetProviderID(), tweakSetReady(true)), - shouldSync: false, - }} - for _, testcase := range testcases { - t.Run(fmt.Sprintf("%s - StableLoadBalancerNodeSet: %v", testcase.name, fgEnabled), func(t *testing.T) { - shouldSync := shouldSyncUpdatedNode(testcase.oldNode, testcase.newNode) - if shouldSync != testcase.shouldSync { - t.Errorf("unexpected result from shouldSyncNode, expected: %v, actual: %v", testcase.shouldSync, shouldSync) - } - }) - } + fgEnabled: false, + }, + }...) + for _, testcase := range testcases { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StableLoadBalancerNodeSet, testcase.fgEnabled)() + t.Run(fmt.Sprintf("%s - StableLoadBalancerNodeSet: %v", testcase.name, testcase.fgEnabled), func(t *testing.T) { + shouldSync := shouldSyncUpdatedNode(testcase.oldNode, testcase.newNode) + if shouldSync != testcase.shouldSync { + t.Errorf("unexpected result from shouldSyncNode, expected: %v, actual: %v", testcase.shouldSync, shouldSync) + } + }) } + } func TestServiceQueueDelay(t *testing.T) {