Merge pull request #117388 from alexanderConstantinescu/117375

[KCCM] service controller: add `providerID` Node predicate

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

@@ -660,15 +660,10 @@ func nodeNames(nodes []*v1.Node) sets.String {
 }
 
 func shouldSyncUpdatedNode(oldNode, newNode *v1.Node) bool {
-	if utilfeature.DefaultFeatureGate.Enabled(features.StableLoadBalancerNodeSet) {
-		// Only Nodes with changes to the label
-		// "node.kubernetes.io/exclude-from-external-load-balancers" will
-		// trigger a load balancer re-sync.
-		return respectsPredicates(oldNode, nodeIncludedPredicate) != respectsPredicates(newNode, nodeIncludedPredicate)
-	}
 	// Evaluate the individual node exclusion predicate before evaluating the
-	// compounded result of all predicates. We don't sync ETP=local services
+	// compounded result of all predicates. We don't sync changes on the
-	// for changes on the readiness condition, hence if a node remains NotReady
+	// readiness condition for eTP:Local services or when
+	// StableLoadBalancerNodeSet is enabled, hence if a node remains NotReady
 	// and a user adds the exclusion label we will need to sync as to make sure
 	// this change is reflected correctly on ETP=local services. The sync
 	// function compares lastSyncedNodes with the new (existing) set of nodes
@@ -679,7 +674,14 @@ func shouldSyncUpdatedNode(oldNode, newNode *v1.Node) bool {
 	if respectsPredicates(oldNode, nodeIncludedPredicate) != respectsPredicates(newNode, nodeIncludedPredicate) {
 		return true
 	}
-	return respectsPredicates(oldNode, allNodePredicates...) != respectsPredicates(newNode, allNodePredicates...)
+	// For the same reason as above, also check for changes to the providerID
+	if respectsPredicates(oldNode, nodeHasProviderIDPredicate) != respectsPredicates(newNode, nodeHasProviderIDPredicate) {
+		return true
+	}
+	if !utilfeature.DefaultFeatureGate.Enabled(features.StableLoadBalancerNodeSet) {
+		return respectsPredicates(oldNode, allNodePredicates...) != respectsPredicates(newNode, allNodePredicates...)
+	}
+	return false
 }
 
 // syncNodes handles updating the hosts pointed to by all load
@@ -937,14 +939,17 @@ 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
@@ -970,6 +975,10 @@ 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 {

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

@@ -471,9 +471,9 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) {
 // TODO: Finish converting and update comments
 func TestUpdateNodesInExternalLoadBalancer(t *testing.T) {
 	nodes := []*v1.Node{
-		{ObjectMeta: metav1.ObjectMeta{Name: "node0"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}},
+		makeNode(tweakName("node1")),
-		{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}},
+		makeNode(tweakName("node2")),
-		{ObjectMeta: metav1.ObjectMeta{Name: "node73"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}},
+		makeNode(tweakName("node3")),
 	}
 	table := []struct {
 		desc                string
@@ -933,10 +933,10 @@ func TestNodeChangesForStableNodeSetEnabled(t *testing.T) {
 }
 
 func TestNodeChangesInExternalLoadBalancer(t *testing.T) {
-	node1 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
+	node1 := makeNode(tweakName("node1"))
-	node2 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node2"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
+	node2 := makeNode(tweakName("node2"))
-	node3 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node3"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
+	node3 := makeNode(tweakName("node3"))
-	node4 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node4"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
+	node4 := makeNode(tweakName("node4"))
 
 	services := []*v1.Service{
 		newService("s0", "777", v1.ServiceTypeLoadBalancer),
@@ -1992,9 +1992,10 @@ func Test_respectsPredicates(t *testing.T) {
 		want  bool
 	}{
 		{want: false, input: &v1.Node{}},
-		{want: true, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}},
+		{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: false, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionFalse}}}}},
-		{want: true, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}, ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}},
+		{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: ""}}}},
 
 		{want: false, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}},
@@ -2073,6 +2074,8 @@ func TestListWithPredicate(t *testing.T) {
 	}
 }
 
+var providerID = "providerID"
+
 type nodeTweak func(n *v1.Node)
 
 // TODO: use this pattern in all the tests above.
@@ -2083,7 +2086,8 @@ func makeNode(tweaks ...nodeTweak) *v1.Node {
 			Labels: map[string]string{},
 		},
 		Spec: v1.NodeSpec{
-			Taints: []v1.Taint{},
+			Taints:     []v1.Taint{},
+			ProviderID: providerID,
 		},
 		Status: v1.NodeStatus{
 			Conditions: []v1.NodeCondition{{
@@ -2171,6 +2175,12 @@ func tweakDeleted() nodeTweak {
 	}
 }
 
+func tweakUnsetProviderID() nodeTweak {
+	return func(n *v1.Node) {
+		n.Spec.ProviderID = ""
+	}
+}
+
 func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
 	testcases := []struct {
 		name                 string
@@ -2328,6 +2338,17 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
 		newNode:              makeNode(tweakDeleted()),
 		shouldSync:           false,
 		stableNodeSetEnabled: true,
+	}, {
+		name:       "providerID set F -> T",
+		oldNode:    makeNode(tweakUnsetProviderID()),
+		newNode:    makeNode(),
+		shouldSync: true,
+	}, {
+		name:                 "providerID set F -> T",
+		oldNode:              makeNode(tweakUnsetProviderID()),
+		newNode:              makeNode(),
+		shouldSync:           true,
+		stableNodeSetEnabled: true,
 	}}
 	for _, testcase := range testcases {
 		t.Run(fmt.Sprintf("%s - StableLoadBalancerNodeSet: %v", testcase.name, testcase.stableNodeSetEnabled), func(t *testing.T) {
@@ -2358,6 +2379,16 @@ func Test_shouldSyncUpdatedNode_compoundedPredicates(t *testing.T) {
 			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)),
@@ -2388,6 +2419,16 @@ func Test_shouldSyncUpdatedNode_compoundedPredicates(t *testing.T) {
 			oldNode:    makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, "")),
 			newNode:    makeNode(tweakSetLabel(v1.LabelNodeExcludeBalancers, ""), 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()),
+			shouldSync: true,
 		}, {
 			name:       "ready F, tainted F->T",
 			oldNode:    makeNode(tweakSetReady(false)),
@@ -2408,6 +2449,46 @@ func Test_shouldSyncUpdatedNode_compoundedPredicates(t *testing.T) {
 			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()),
+			shouldSync: true,
+		}, {
+			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,
+		}, {
+			name:       "providerID unset, excluded T->F",
+			oldNode:    makeNode(tweakUnsetProviderID(), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")),
+			newNode:    makeNode(tweakUnsetProviderID()),
+			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) {