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 2003a9f7cdc..b51eee68c72 100644 --- a/staging/src/k8s.io/cloud-provider/controllers/service/controller.go +++ b/staging/src/k8s.io/cloud-provider/controllers/service/controller.go @@ -745,9 +745,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)) @@ -758,6 +760,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. 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 62909f88230..47851447c6e 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 @@ -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()