github/kubernetes
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix a data race in TopologyCache

Browse Source 
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>
This commit is contained in:
Quan Tian
2023-04-12 23:51:23 +08:00
parent 2f1db33dd5
commit d567d61aa5
2 changed files with 66 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
pkg/controller/endpointslice/topologycache/topologycache.go
View File

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

63
pkg/controller/endpointslice/topologycache/topologycache_test.go
View File

@@ -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: pointer.String("zone-a"),
Conditions: discovery.EndpointConditions{Ready: pointer.Bool(true)},
}, {
Addresses: []string{"10.1.2.4"},
Zone: pointer.String("zone-b"),
Conditions: discovery.EndpointConditions{Ready: pointer.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 // Test Helpers
func expectEquivalentSlices(t *testing.T, actualSlices, expectedSlices []*discovery.EndpointSlice) { func expectEquivalentSlices(t *testing.T, actualSlices, expectedSlices []*discovery.EndpointSlice) {