Fix a data race in TopologyCache

The member variable `cpuRatiosByZone` should be accessed with the lock acquired as it could be be updated by `SetNodes` concurrently. Signed-off-by: Quan Tian <qtian@vmware.com> Co-authored-by: Antonio Ojea <aojea@google.com>
kubernetes · Apr 13, 2023 · 12b7029 · 12b7029
1 parent b121565
commit 12b7029
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 3 deletions.
diff --git a/pkg/controller/endpointslice/topologycache/topologycache.go b/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{}

diff --git a/pkg/controller/endpointslice/topologycache/topologycache_test.go b/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) {