Merge pull request #117602 from alexanderConstantinescu/revise-providerid

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

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
+	// For the same reason as above, also check for any change to the providerID
-	if respectsPredicates(oldNode, nodeHasProviderIDPredicate) != respectsPredicates(newNode, nodeHasProviderIDPredicate) {
+	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 {

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) {
-	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()),
+				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), tweakUnsetProviderID()),
+				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, ""), tweakUnsetProviderID()),
+				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, ""), tweakUnsetProviderID()),
+				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), tweakUnsetProviderID()),
+				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), tweakUnsetProviderID()),
+				newNode:    makeNode(tweakSetReady(false), tweakProviderID("")),
 				shouldSync: true,
+				fgEnabled:  fgEnabled,
 			}, {
-			name:       "providerID unset, tainted F->T",
+				name:       "ready F, providerID change",
-			oldNode:    makeNode(tweakUnsetProviderID()),
+				oldNode:    makeNode(tweakSetReady(false)),
-			newNode:    makeNode(tweakUnsetProviderID(), tweakAddTaint(ToBeDeletedTaint)),
+				newNode:    makeNode(tweakSetReady(false), tweakProviderID(providerID+"-2")),
-			shouldSync: false,
+				shouldSync: true,
-		}, {
+				fgEnabled:  fgEnabled,
-			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()),
+				oldNode:    makeNode(tweakProviderID("")),
-			newNode:    makeNode(tweakUnsetProviderID(), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")),
+				newNode:    makeNode(tweakProviderID(""), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")),
 				shouldSync: true,
+				fgEnabled:  fgEnabled,
 			}, {
 				name:       "providerID unset, excluded T->F",
-			oldNode:    makeNode(tweakUnsetProviderID(), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")),
+				oldNode:    makeNode(tweakProviderID(""), tweakSetLabel(v1.LabelNodeExcludeBalancers, "")),
-			newNode:    makeNode(tweakUnsetProviderID()),
+				newNode:    makeNode(tweakProviderID("")),
 				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()),
+				oldNode:    makeNode(tweakProviderID("")),
-			newNode:    makeNode(tweakUnsetProviderID(), tweakSetReady(true)),
+				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,
+			fgEnabled:  true,
+		},
+		{
+			name:       "providerID unset, ready F->T",
+			oldNode:    makeNode(tweakProviderID("")),
+			newNode:    makeNode(tweakProviderID(""), tweakSetReady(false)),
+			shouldSync: true,
+			fgEnabled:  false,
+		}, {
+			name:       "providerID unset, tainted T->F",
+			oldNode:    makeNode(tweakProviderID(""), tweakAddTaint(ToBeDeletedTaint)),
+			newNode:    makeNode(tweakProviderID("")),
+			shouldSync: false,
+			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(tweakProviderID("")),
+			newNode:    makeNode(tweakProviderID(""), tweakAddTaint(ToBeDeletedTaint)),
+			shouldSync: false,
+			fgEnabled:  true,
+		}, {
+			name:       "providerID unset, tainted F->T",
+			oldNode:    makeNode(tweakProviderID("")),
+			newNode:    makeNode(tweakProviderID(""), 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)
 			}
 		})
 	}
-	}
+
 }
 
 func TestServiceQueueDelay(t *testing.T) {