From a62563f13057df0d58b3f2d9b6178eb9df584fa9 Mon Sep 17 00:00:00 2001
From: Quan Tian <qtian@vmware.com>
Date: Wed, 12 Apr 2023 20:59:14 +0800
Subject: [PATCH] Fix TopologyAwareHint not working when zone label is added
 after Node creation

The topology.kubernetes.io/zone label may be added by could provider
asynchronously after the Node is created. The previous code didn't
update the topology cache after receiving the Node update event, causing
TopologyAwareHint to not work until kube-controller-manager restarts or
other Node events trigger the update.

Signed-off-by: Quan Tian <qtian@vmware.com>
---
 .../endpointslice/endpointslice_controller.go |  5 +-
 .../endpointslice_controller_test.go          | 75 +++++++++++++++++++
 2 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/pkg/controller/endpointslice/endpointslice_controller.go b/pkg/controller/endpointslice/endpointslice_controller.go
index 5910d6cd3bd..425f8fb57a1 100644
--- a/pkg/controller/endpointslice/endpointslice_controller.go
+++ b/pkg/controller/endpointslice/endpointslice_controller.go
@@ -520,7 +520,10 @@ func (c *Controller) updateNode(old, cur interface{}) {
 	oldNode := old.(*v1.Node)
 	curNode := cur.(*v1.Node)
 
-	if topologycache.NodeReady(oldNode.Status) != topologycache.NodeReady(curNode.Status) {
+	// LabelTopologyZone may be added by cloud provider asynchronously after the Node is created.
+	// The topology cache should be updated in this case.
+	if topologycache.NodeReady(oldNode.Status) != topologycache.NodeReady(curNode.Status) ||
+		oldNode.Labels[v1.LabelTopologyZone] != curNode.Labels[v1.LabelTopologyZone] {
 		c.checkNodeTopologyDistribution()
 	}
 }
diff --git a/pkg/controller/endpointslice/endpointslice_controller_test.go b/pkg/controller/endpointslice/endpointslice_controller_test.go
index f1b14b193dc..5d6a98ef49d 100644
--- a/pkg/controller/endpointslice/endpointslice_controller_test.go
+++ b/pkg/controller/endpointslice/endpointslice_controller_test.go
@@ -25,6 +25,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	v1 "k8s.io/api/core/v1"
 	discovery "k8s.io/api/discovery/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -1877,6 +1878,80 @@ func Test_checkNodeTopologyDistribution(t *testing.T) {
 	}
 }
 
+func TestUpdateNode(t *testing.T) {
+	nodeReadyStatus := v1.NodeStatus{
+		Allocatable: map[v1.ResourceName]resource.Quantity{
+			v1.ResourceCPU: resource.MustParse("100m"),
+		},
+		Conditions: []v1.NodeCondition{
+			{
+				Type:   v1.NodeReady,
+				Status: v1.ConditionTrue,
+			},
+		},
+	}
+	_, esController := newController(nil, time.Duration(0))
+	sliceInfo := &topologycache.SliceInfo{
+		ServiceKey:  "ns/svc",
+		AddressType: discovery.AddressTypeIPv4,
+		ToCreate: []*discovery.EndpointSlice{
+			{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "svc-abc",
+					Namespace: "ns",
+					Labels: map[string]string{
+						discovery.LabelServiceName: "svc",
+						discovery.LabelManagedBy:   controllerName,
+					},
+				},
+				Endpoints: []discovery.Endpoint{
+					{
+						Addresses:  []string{"172.18.0.2"},
+						Zone:       pointer.String("zone-a"),
+						Conditions: discovery.EndpointConditions{Ready: pointer.Bool(true)},
+					},
+					{
+						Addresses:  []string{"172.18.1.2"},
+						Zone:       pointer.String("zone-b"),
+						Conditions: discovery.EndpointConditions{Ready: pointer.Bool(true)},
+					},
+				},
+				AddressType: discovery.AddressTypeIPv4,
+			},
+		},
+	}
+	node1 := &v1.Node{
+		ObjectMeta: metav1.ObjectMeta{Name: "node-1"},
+		Status:     nodeReadyStatus,
+	}
+	node2 := &v1.Node{
+		ObjectMeta: metav1.ObjectMeta{Name: "node-2"},
+		Status:     nodeReadyStatus,
+	}
+	esController.nodeStore.Add(node1)
+	esController.nodeStore.Add(node2)
+	esController.addNode(node1)
+	esController.addNode(node2)
+	// The Nodes don't have the zone label, AddHints should fail.
+	_, _, eventsBuilders := esController.topologyCache.AddHints(sliceInfo)
+	require.Len(t, eventsBuilders, 1)
+	assert.Contains(t, eventsBuilders[0].Message, topologycache.InsufficientNodeInfo)
+
+	updateNode1 := node1.DeepCopy()
+	updateNode1.Labels = map[string]string{v1.LabelTopologyZone: "zone-a"}
+	updateNode2 := node2.DeepCopy()
+	updateNode2.Labels = map[string]string{v1.LabelTopologyZone: "zone-b"}
+
+	// After adding the zone label to the Nodes and calling the event handler updateNode, AddHints should succeed.
+	esController.nodeStore.Update(updateNode1)
+	esController.nodeStore.Update(updateNode2)
+	esController.updateNode(node1, updateNode1)
+	esController.updateNode(node2, updateNode2)
+	_, _, eventsBuilders = esController.topologyCache.AddHints(sliceInfo)
+	require.Len(t, eventsBuilders, 1)
+	assert.Contains(t, eventsBuilders[0].Message, topologycache.TopologyAwareHintsEnabled)
+}
+
 // Test helpers
 func addPods(t *testing.T, esController *endpointSliceController, namespace string, podsCount int) {
 	t.Helper()