service controller: only sync LB node pools when relevant fields change

Signed-off-by: Andrew Sy Kim <[email protected]>

Author: andrewsykim

pkg/controller/service/controller.go

@@ -176,6 +176,20 @@ func New(
 				s.nodeSyncLoop()
 			},
 			UpdateFunc: func(old, cur interface{}) {
+				oldNode, ok := old.(*v1.Node)
+				if !ok {
+					return
+				}
+
+				curNode, ok := cur.(*v1.Node)
+				if !ok {
+					return
+				}
+
+				if !shouldSyncNode(oldNode, curNode) {
+					return
+				}
+
 				s.nodeSyncLoop()
 			},
 			DeleteFunc: func(old interface{}) {
@@ -649,6 +663,30 @@ func getNodeConditionPredicate() NodeConditionPredicate {
 	}
 }
 
+func shouldSyncNode(oldNode, newNode *v1.Node) bool {
+	if oldNode.Spec.Unschedulable != newNode.Spec.Unschedulable {
+		return true
+	}
+
+	if !reflect.DeepEqual(oldNode.Labels, newNode.Labels) {
+		return true
+	}
+
+	return nodeReadyConditionStatus(oldNode) != nodeReadyConditionStatus(newNode)
+}
+
+func nodeReadyConditionStatus(node *v1.Node) v1.ConditionStatus {
+	for _, condition := range node.Status.Conditions {
+		if condition.Type != v1.NodeReady {
+			continue
+		}
+
+		return condition.Status
+	}
+
+	return ""
+}
+
 // nodeSyncLoop handles updating the hosts pointed to by all load
 // balancers whenever the set of nodes in the cluster changes.
 func (s *Controller) nodeSyncLoop() {

pkg/controller/service/controller_test.go

@@ -1486,3 +1486,122 @@ func TestListWithPredicate(t *testing.T) {
 		})
 	}
 }
+
+func Test_shouldSyncNode(t *testing.T) {
+	testcases := []struct {
+		name       string
+		oldNode    *v1.Node
+		newNode    *v1.Node
+		shouldSync bool
+	}{
+		{
+			name: "spec.unschedable field changed",
+			oldNode: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "node",
+				},
+				Spec: v1.NodeSpec{
+					Unschedulable: false,
+				},
+			},
+			newNode: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "node",
+				},
+				Spec: v1.NodeSpec{
+					Unschedulable: true,
+				},
+			},
+			shouldSync: true,
+		},
+		{
+			name: "labels changed",
+			oldNode: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:   "node",
+					Labels: map[string]string{},
+				},
+			},
+			newNode: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "node",
+					Labels: map[string]string{
+						labelNodeRoleExcludeBalancer: "",
+					},
+				},
+			},
+			shouldSync: true,
+		},
+		{
+			name: "ready condition changed",
+			oldNode: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "node",
+				},
+				Status: v1.NodeStatus{
+					Conditions: []v1.NodeCondition{
+						{
+							Type:   v1.NodeReady,
+							Status: v1.ConditionTrue,
+						},
+					},
+				},
+			},
+			newNode: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "node",
+				},
+				Status: v1.NodeStatus{
+					Conditions: []v1.NodeCondition{
+						{
+							Type:   v1.NodeReady,
+							Status: v1.ConditionFalse,
+						},
+					},
+				},
+			},
+			shouldSync: true,
+		},
+		{
+			name: "not relevant condition changed and no ready condition",
+			oldNode: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "node",
+				},
+				Status: v1.NodeStatus{
+					Conditions: []v1.NodeCondition{
+						{
+							Type:   v1.NodeNetworkUnavailable,
+							Status: v1.ConditionTrue,
+						},
+					},
+				},
+			},
+			newNode: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "node",
+				},
+				Status: v1.NodeStatus{
+					Conditions: []v1.NodeCondition{
+						{
+							Type:   v1.NodeNetworkUnavailable,
+							Status: v1.ConditionFalse,
+						},
+					},
+				},
+			},
+			shouldSync: false,
+		},
+	}
+
+	for _, testcase := range testcases {
+		t.Run(testcase.name, func(t *testing.T) {
+			shouldSync := shouldSyncNode(testcase.oldNode, testcase.newNode)
+			if shouldSync != testcase.shouldSync {
+				t.Logf("actual shouldSyncNode: %v", shouldSync)
+				t.Logf("expected shouldSyncNode: %v", testcase.shouldSync)
+				t.Errorf("unexpected result from shouldSyncNode")
+			}
+		})
+	}
+}