Automated cherry pick of #117245: Fix TopologyAwareHint not working when zone label is added #117249: Fix a data race in TopologyCache

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()
 	}
 }

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"
@@ -1805,6 +1806,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:       utilpointer.String("zone-a"),
+						Conditions: discovery.EndpointConditions{Ready: utilpointer.Bool(true)},
+					},
+					{
+						Addresses:  []string{"172.18.1.2"},
+						Zone:       utilpointer.String("zone-b"),
+						Conditions: discovery.EndpointConditions{Ready: utilpointer.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()

pkg/controller/endpointslice/topologycache/topologycache.go

@@ -270,6 +270,9 @@ func (t *TopologyCache) HasPopulatedHints(serviceKey string) bool {
 // it is not possible to provide allocations that are below the overload
 // threshold, a nil value will be returned.
 func (t *TopologyCache) getAllocations(numEndpoints int) (map[string]Allocation, *EventBuilder) {
+	t.lock.Lock()
+	defer t.lock.Unlock()
+
 	// it is similar to checking !t.sufficientNodeInfo
 	if t.cpuRatiosByZone == nil {
 		return nil, &EventBuilder{
@@ -293,9 +296,6 @@ func (t *TopologyCache) getAllocations(numEndpoints int) (map[string]Allocation,
 		}
 	}
 
-	t.lock.Lock()
-	defer t.lock.Unlock()
-
 	remainingMinEndpoints := numEndpoints
 	minTotal := 0
 	allocations := map[string]Allocation{}

pkg/controller/endpointslice/topologycache/topologycache_test.go

@@ -625,6 +625,69 @@ func TestSetNodes(t *testing.T) {
 	}
 }
 
+func TestTopologyCacheRace(t *testing.T) {
+	sliceInfo := &SliceInfo{
+		ServiceKey:  "ns/svc",
+		AddressType: discovery.AddressTypeIPv4,
+		ToCreate: []*discovery.EndpointSlice{{
+			Endpoints: []discovery.Endpoint{{
+				Addresses:  []string{"10.1.2.3"},
+				Zone:       utilpointer.String("zone-a"),
+				Conditions: discovery.EndpointConditions{Ready: utilpointer.Bool(true)},
+			}, {
+				Addresses:  []string{"10.1.2.4"},
+				Zone:       utilpointer.String("zone-b"),
+				Conditions: discovery.EndpointConditions{Ready: utilpointer.Bool(true)},
+			}},
+		}}}
+	type nodeInfo struct {
+		zone  string
+		cpu   resource.Quantity
+		ready v1.ConditionStatus
+	}
+	nodeInfos := []nodeInfo{
+		{zone: "zone-a", cpu: resource.MustParse("1000m"), ready: v1.ConditionTrue},
+		{zone: "zone-a", cpu: resource.MustParse("1000m"), ready: v1.ConditionTrue},
+		{zone: "zone-a", cpu: resource.MustParse("1000m"), ready: v1.ConditionTrue},
+		{zone: "zone-a", cpu: resource.MustParse("2000m"), ready: v1.ConditionTrue},
+		{zone: "zone-b", cpu: resource.MustParse("3000m"), ready: v1.ConditionTrue},
+		{zone: "zone-b", cpu: resource.MustParse("1500m"), ready: v1.ConditionTrue},
+		{zone: "zone-c", cpu: resource.MustParse("500m"), ready: v1.ConditionTrue},
+	}
+
+	cache := NewTopologyCache()
+	nodes := []*v1.Node{}
+	for _, node := range nodeInfos {
+		labels := map[string]string{}
+		if node.zone != "" {
+			labels[v1.LabelTopologyZone] = node.zone
+		}
+		conditions := []v1.NodeCondition{{
+			Type:   v1.NodeReady,
+			Status: node.ready,
+		}}
+		allocatable := v1.ResourceList{
+			v1.ResourceCPU: node.cpu,
+		}
+		nodes = append(nodes, &v1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: labels,
+			},
+			Status: v1.NodeStatus{
+				Allocatable: allocatable,
+				Conditions:  conditions,
+			},
+		})
+	}
+
+	go func() {
+		cache.SetNodes(nodes)
+	}()
+	go func() {
+		cache.AddHints(sliceInfo)
+	}()
+}
+
 // Test Helpers
 
 func expectEquivalentSlices(t *testing.T, actualSlices, expectedSlices []*discovery.EndpointSlice) {