[KCCM]: have providerID trigger re-sync, but not be required for nodes

staging/src/k8s.io/cloud-provider/controllers/service/controller.go

@@ -948,17 +948,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
@@ -978,16 +975,19 @@ func getNodePredicatesForService(service *v1.Service) []NodeConditionPredicate {
 	return allNodePredicates
 }
 
+// This predicate just validates if the providerID has been set and triggers a
+// node sync. It is _not_ used when determining which nodes to use when
+// configuring the load balancer's backend pool.
+func nodeHasProviderIDPredicate(node *v1.Node) bool {
+	return node.Spec.ProviderID != ""
+}
+
 // We consider the node for load balancing only when the node is not labelled for exclusion.
 func nodeIncludedPredicate(node *v1.Node) bool {
 	_, hasExcludeBalancerLabel := node.Labels[v1.LabelNodeExcludeBalancers]
 	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 {

staging/src/k8s.io/cloud-provider/controllers/service/controller_test.go

@@ -1779,7 +1779,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: ""}}}},
@@ -2144,143 +2144,196 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
 }
 
 func Test_shouldSyncUpdatedNode_compoundedPredicates(t *testing.T) {
-	for _, fgEnabled := range []bool{true, false} {
+	type testCase struct {
-		defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StableLoadBalancerNodeSet, fgEnabled)()
-		testcases := []struct {
 		name       string
 		oldNode    *v1.Node
 		newNode    *v1.Node
 		shouldSync bool
-		}{{
+		fgEnabled  bool
+	}
+	testcases := []testCase{}
+	for _, fgEnabled := range []bool{true, 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), tweakUnsetProviderID()),
 				newNode:    makeNode(tweakAddTaint(ToBeDeletedTaint)),
 				shouldSync: true,
+				fgEnabled:  fgEnabled,
 			}, {
 				name:       "tainted T, providerID set T->F",
 				oldNode:    makeNode(tweakAddTaint(ToBeDeletedTaint)),
 				newNode:    makeNode(tweakAddTaint(ToBeDeletedTaint), tweakUnsetProviderID()),
 				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, ""), tweakUnsetProviderID()),
 				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, ""), tweakUnsetProviderID()),
 				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), tweakUnsetProviderID()),
 				newNode:    makeNode(tweakSetReady(false)),
 				shouldSync: true,
+				fgEnabled:  fgEnabled,
 			}, {
 				name:       "ready F, providerID set T->F",
 				oldNode:    makeNode(tweakSetReady(false)),
 				newNode:    makeNode(tweakSetReady(false), tweakUnsetProviderID()),
 				shouldSync: true,
-		}, {
+				fgEnabled:  fgEnabled,
-			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()),
-			shouldSync: false,
 			}, {
 				name:       "providerID unset, excluded F->T",
 				oldNode:    makeNode(tweakUnsetProviderID()),
 				newNode:    makeNode(tweakUnsetProviderID(), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")),
 				shouldSync: true,
+				fgEnabled:  fgEnabled,
 			}, {
 				name:       "providerID unset, excluded T->F",
 				oldNode:    makeNode(tweakUnsetProviderID(), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")),
 				newNode:    makeNode(tweakUnsetProviderID()),
 				shouldSync: true,
-		}, {
+				fgEnabled:  fgEnabled,
-			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,
-		}}
+				fgEnabled:  fgEnabled,
+			},
+		}...)
+	}
+	testcases = append(testcases, []testCase{
+		{
+			name:       "providerID unset, ready F->T",
+			oldNode:    makeNode(tweakUnsetProviderID()),
+			newNode:    makeNode(tweakUnsetProviderID(), tweakSetReady(false)),
+			shouldSync: false,
+			fgEnabled:  true,
+		},
+		{
+			name:       "providerID unset, ready F->T",
+			oldNode:    makeNode(tweakUnsetProviderID()),
+			newNode:    makeNode(tweakUnsetProviderID(), tweakSetReady(false)),
+			shouldSync: true,
+			fgEnabled:  false,
+		}, {
+			name:       "providerID unset, tainted T->F",
+			oldNode:    makeNode(tweakUnsetProviderID(), tweakAddTaint(ToBeDeletedTaint)),
+			newNode:    makeNode(tweakUnsetProviderID()),
+			shouldSync: false,
+			fgEnabled:  true,
+		},
+		{
+			name:       "providerID unset, tainted T->F",
+			oldNode:    makeNode(tweakUnsetProviderID(), tweakAddTaint(ToBeDeletedTaint)),
+			newNode:    makeNode(tweakUnsetProviderID()),
+			shouldSync: true,
+			fgEnabled:  false,
+		}, {
+			name:       "providerID unset, tainted F->T",
+			oldNode:    makeNode(tweakUnsetProviderID()),
+			newNode:    makeNode(tweakUnsetProviderID(), tweakAddTaint(ToBeDeletedTaint)),
+			shouldSync: false,
+			fgEnabled:  true,
+		}, {
+			name:       "providerID unset, tainted F->T",
+			oldNode:    makeNode(tweakUnsetProviderID()),
+			newNode:    makeNode(tweakUnsetProviderID(), tweakAddTaint(ToBeDeletedTaint)),
+			shouldSync: true,
+			fgEnabled:  false,
+		},
+	}...)
 	for _, testcase := range testcases {
-			t.Run(fmt.Sprintf("%s - StableLoadBalancerNodeSet: %v", testcase.name, fgEnabled), func(t *testing.T) {
+		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)
 			}
 		})
 	}
-	}
+
 }
 
 type fakeNodeLister struct {