service controller: improve node lifecycle updates - update nodes if providerID has changed

Signed-off-by: czawadka <czawadka@google.com>

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

@@ -735,9 +735,11 @@ func (c *Controller) nodeSyncService(svc *v1.Service, oldNodes, newNodes []*v1.N
 	}
 	newNodes = filterWithPredicates(newNodes, getNodePredicatesForService(svc)...)
 	oldNodes = filterWithPredicates(oldNodes, getNodePredicatesForService(svc)...)
-	if nodeNames(newNodes).Equal(nodeNames(oldNodes)) {
+
+	if nodesSufficientlyEqual(oldNodes, newNodes) {
 		return retSuccess
 	}
+
 	klog.V(4).Infof("nodeSyncService started for service %s/%s", svc.Namespace, svc.Name)
 	if err := c.lockedUpdateLoadBalancerHosts(svc, newNodes); err != nil {
 		runtime.HandleError(fmt.Errorf("failed to update load balancer hosts for service %s/%s: %v", svc.Namespace, svc.Name, err))
@@ -748,6 +750,34 @@ func (c *Controller) nodeSyncService(svc *v1.Service, oldNodes, newNodes []*v1.N
 	return retSuccess
 }
 
+func nodesSufficientlyEqual(oldNodes, newNodes []*v1.Node) bool {
+	if len(oldNodes) != len(newNodes) {
+		return false
+	}
+
+	// This holds the Node fields which trigger a sync when changed.
+	type protoNode struct {
+		providerID string
+	}
+	distill := func(n *v1.Node) protoNode {
+		return protoNode{
+			providerID: n.Spec.ProviderID,
+		}
+	}
+
+	mOld := map[string]protoNode{}
+	for _, n := range oldNodes {
+		mOld[n.Name] = distill(n)
+	}
+
+	mNew := map[string]protoNode{}
+	for _, n := range newNodes {
+		mNew[n.Name] = distill(n)
+	}
+
+	return reflect.DeepEqual(mOld, mNew)
+}
+
 // updateLoadBalancerHosts updates all existing load balancers so that
 // they will match the latest list of nodes with input number of workers.
 // Returns the list of services that couldn't be updated.

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

@@ -897,6 +897,65 @@ func compareUpdateCalls(t *testing.T, left, right []fakecloud.UpdateBalancerCall
 	}
 }
 
+func TestNodesNotEqual(t *testing.T) {
+	controller, cloud, _ := newController()
+
+	services := []*v1.Service{
+		newService("s0", v1.ServiceTypeLoadBalancer),
+		newService("s1", v1.ServiceTypeLoadBalancer),
+	}
+
+	node1 := makeNode(tweakName("node1"))
+	node2 := makeNode(tweakName("node2"))
+	node3 := makeNode(tweakName("node3"))
+	node1WithProviderID := makeNode(tweakName("node1"), tweakProviderID("cumulus/1"))
+	node2WithProviderID := makeNode(tweakName("node2"), tweakProviderID("cumulus/2"))
+
+	testCases := []struct {
+		desc                string
+		lastSyncNodes       []*v1.Node
+		newNodes            []*v1.Node
+		expectedUpdateCalls []fakecloud.UpdateBalancerCall
+	}{
+		{
+			desc:          "Nodes with updated providerID",
+			lastSyncNodes: []*v1.Node{node1, node2},
+			newNodes:      []*v1.Node{node1WithProviderID, node2WithProviderID},
+			expectedUpdateCalls: []fakecloud.UpdateBalancerCall{
+				{Service: newService("s0", v1.ServiceTypeLoadBalancer), Hosts: []*v1.Node{node1WithProviderID, node2WithProviderID}},
+				{Service: newService("s1", v1.ServiceTypeLoadBalancer), Hosts: []*v1.Node{node1WithProviderID, node2WithProviderID}},
+			},
+		},
+		{
+			desc:                "Nodes unchanged",
+			lastSyncNodes:       []*v1.Node{node1WithProviderID, node2},
+			newNodes:            []*v1.Node{node1WithProviderID, node2},
+			expectedUpdateCalls: []fakecloud.UpdateBalancerCall{},
+		},
+		{
+			desc:          "Change node with empty providerID",
+			lastSyncNodes: []*v1.Node{node1WithProviderID, node2},
+			newNodes:      []*v1.Node{node1WithProviderID, node3},
+			expectedUpdateCalls: []fakecloud.UpdateBalancerCall{
+				{Service: newService("s0", v1.ServiceTypeLoadBalancer), Hosts: []*v1.Node{node1WithProviderID, node3}},
+				{Service: newService("s1", v1.ServiceTypeLoadBalancer), Hosts: []*v1.Node{node1WithProviderID, node3}},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+			controller.nodeLister = newFakeNodeLister(nil, tc.newNodes...)
+			controller.lastSyncedNodes = tc.lastSyncNodes
+			controller.updateLoadBalancerHosts(ctx, services, 5)
+			compareUpdateCalls(t, tc.expectedUpdateCalls, cloud.UpdateCalls)
+			cloud.UpdateCalls = []fakecloud.UpdateBalancerCall{}
+		})
+	}
+}
+
 func TestProcessServiceCreateOrUpdate(t *testing.T) {
 	controller, _, client := newController()