Skip to content

Commit 67ec4b3

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#92838 from tnqn/endpointslicetrack-leak
Fix memory leak in endpointSliceTracker
2 parents 76e3b25 + 0876825 commit 67ec4b3

File tree

5 files changed

+129
-21
lines changed

5 files changed

+129
-21
lines changed

‎pkg/controller/endpointslice/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ go_test(
6666
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
6767
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
6868
"//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
69+
"//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
6970
"//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
7071
"//staging/src/k8s.io/apimachinery/pkg/util/rand:go_default_library",
7172
"//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library",

‎pkg/controller/endpointslice/endpointslice_controller.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,7 @@ func (c *Controller) syncService(key string) error {
307307
if apierrors.IsNotFound(err) {
308308
c.triggerTimeTracker.DeleteService(namespace, name)
309309
c.reconciler.deleteService(namespace, name)
310+
c.endpointSliceTracker.DeleteService(namespace, name)
310311
// The service has been deleted, return nil so that it won't be retried.
311312
return nil
312313
}

‎pkg/controller/endpointslice/endpointslice_tracker.go‎

Lines changed: 31 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,11 @@ func (est *endpointSliceTracker) Has(endpointSlice *discovery.EndpointSlice) boo
5151
est.lock.Lock()
5252
defer est.lock.Unlock()
5353

54-
rrv := est.relatedResourceVersions(endpointSlice)
55-
_, ok := rrv[endpointSlice.Name]
54+
rrv, ok := est.relatedResourceVersions(endpointSlice)
55+
if !ok {
56+
return false
57+
}
58+
_, ok = rrv[endpointSlice.Name]
5659
return ok
5760
}
5861

@@ -63,7 +66,10 @@ func (est *endpointSliceTracker) Stale(endpointSlice *discovery.EndpointSlice) b
6366
est.lock.Lock()
6467
defer est.lock.Unlock()
6568

66-
rrv := est.relatedResourceVersions(endpointSlice)
69+
rrv, ok := est.relatedResourceVersions(endpointSlice)
70+
if !ok {
71+
return true
72+
}
6773
return rrv[endpointSlice.Name] != endpointSlice.ResourceVersion
6874
}
6975

@@ -73,33 +79,42 @@ func (est *endpointSliceTracker) Update(endpointSlice *discovery.EndpointSlice)
7379
est.lock.Lock()
7480
defer est.lock.Unlock()
7581

76-
rrv := est.relatedResourceVersions(endpointSlice)
82+
rrv, ok := est.relatedResourceVersions(endpointSlice)
83+
if !ok {
84+
rrv = endpointSliceResourceVersions{}
85+
est.resourceVersionsByService[getServiceNN(endpointSlice)] = rrv
86+
}
7787
rrv[endpointSlice.Name] = endpointSlice.ResourceVersion
7888
}
7989

90+
// DeleteService removes the set of resource versions tracked for the Service.
91+
func (est *endpointSliceTracker) DeleteService(namespace, name string) {
92+
est.lock.Lock()
93+
defer est.lock.Unlock()
94+
95+
serviceNN := types.NamespacedName{Name: name, Namespace: namespace}
96+
delete(est.resourceVersionsByService, serviceNN)
97+
}
98+
8099
// Delete removes the resource version in this endpointSliceTracker for the
81100
// provided EndpointSlice.
82101
func (est *endpointSliceTracker) Delete(endpointSlice *discovery.EndpointSlice) {
83102
est.lock.Lock()
84103
defer est.lock.Unlock()
85104

86-
rrv := est.relatedResourceVersions(endpointSlice)
87-
delete(rrv, endpointSlice.Name)
105+
rrv, ok := est.relatedResourceVersions(endpointSlice)
106+
if ok {
107+
delete(rrv, endpointSlice.Name)
108+
}
88109
}
89110

90111
// relatedResourceVersions returns the set of resource versions tracked for the
91-
// Service corresponding to the provided EndpointSlice. If no resource versions
92-
// are currently tracked for this service, an empty set is initialized.
93-
func (est *endpointSliceTracker) relatedResourceVersions(endpointSlice *discovery.EndpointSlice) endpointSliceResourceVersions {
112+
// Service corresponding to the provided EndpointSlice, and a bool to indicate
113+
// if it exists.
114+
func (est *endpointSliceTracker) relatedResourceVersions(endpointSlice *discovery.EndpointSlice) (endpointSliceResourceVersions, bool) {
94115
serviceNN := getServiceNN(endpointSlice)
95116
vers, ok := est.resourceVersionsByService[serviceNN]
96-
97-
if !ok {
98-
vers = endpointSliceResourceVersions{}
99-
est.resourceVersionsByService[serviceNN] = vers
100-
}
101-
102-
return vers
117+
return vers, ok
103118
}
104119

105120
// getServiceNN returns a namespaced name for the Service corresponding to the

‎pkg/controller/endpointslice/endpointslice_tracker_test.go‎

Lines changed: 95 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,11 @@ package endpointslice
1919
import (
2020
"testing"
2121

22+
"github.com/stretchr/testify/assert"
23+
2224
discovery "k8s.io/api/discovery/v1beta1"
2325
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
26+
"k8s.io/apimachinery/pkg/types"
2427
)
2528

2629
func TestEndpointSliceTrackerUpdate(t *testing.T) {
@@ -43,34 +46,55 @@ func TestEndpointSliceTrackerUpdate(t *testing.T) {
4346
epSlice1DifferentRV.ResourceVersion = "rv2"
4447

4548
testCases := map[string]struct {
46-
updateParam *discovery.EndpointSlice
47-
checksParam *discovery.EndpointSlice
48-
expectHas bool
49-
expectStale bool
49+
updateParam *discovery.EndpointSlice
50+
checksParam *discovery.EndpointSlice
51+
expectHas bool
52+
expectStale bool
53+
expectResourceVersionsByService map[types.NamespacedName]endpointSliceResourceVersions
5054
}{
5155
"same slice": {
5256
updateParam: epSlice1,
5357
checksParam: epSlice1,
5458
expectHas: true,
5559
expectStale: false,
60+
expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{
61+
{Namespace: epSlice1.Namespace, Name: "svc1"}: {
62+
epSlice1.Name: epSlice1.ResourceVersion,
63+
},
64+
},
5665
},
5766
"different namespace": {
5867
updateParam: epSlice1,
5968
checksParam: epSlice1DifferentNS,
6069
expectHas: false,
6170
expectStale: true,
71+
expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{
72+
{Namespace: epSlice1.Namespace, Name: "svc1"}: {
73+
epSlice1.Name: epSlice1.ResourceVersion,
74+
},
75+
},
6276
},
6377
"different service": {
6478
updateParam: epSlice1,
6579
checksParam: epSlice1DifferentService,
6680
expectHas: false,
6781
expectStale: true,
82+
expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{
83+
{Namespace: epSlice1.Namespace, Name: "svc1"}: {
84+
epSlice1.Name: epSlice1.ResourceVersion,
85+
},
86+
},
6887
},
6988
"different resource version": {
7089
updateParam: epSlice1,
7190
checksParam: epSlice1DifferentRV,
7291
expectHas: true,
7392
expectStale: true,
93+
expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{
94+
{Namespace: epSlice1.Namespace, Name: "svc1"}: {
95+
epSlice1.Name: epSlice1.ResourceVersion,
96+
},
97+
},
7498
},
7599
}
76100

@@ -84,6 +108,7 @@ func TestEndpointSliceTrackerUpdate(t *testing.T) {
84108
if esTracker.Stale(tc.checksParam) != tc.expectStale {
85109
t.Errorf("tc.tracker.Stale(%+v) == %t, expected %t", tc.checksParam, esTracker.Stale(tc.checksParam), tc.expectStale)
86110
}
111+
assert.Equal(t, tc.expectResourceVersionsByService, esTracker.resourceVersionsByService)
87112
})
88113
}
89114
}
@@ -172,3 +197,69 @@ func TestEndpointSliceTrackerDelete(t *testing.T) {
172197
})
173198
}
174199
}
200+
201+
func TestEndpointSliceTrackerDeleteService(t *testing.T) {
202+
svcName1, svcNS1 := "svc1", "ns1"
203+
svcName2, svcNS2 := "svc2", "ns2"
204+
epSlice1 := &discovery.EndpointSlice{
205+
ObjectMeta: metav1.ObjectMeta{
206+
Name: "example-1",
207+
Namespace: svcNS1,
208+
ResourceVersion: "rv1",
209+
Labels: map[string]string{discovery.LabelServiceName: svcName1},
210+
},
211+
}
212+
213+
testCases := map[string]struct {
214+
updateParam *discovery.EndpointSlice
215+
deleteServiceParam *types.NamespacedName
216+
expectHas bool
217+
expectStale bool
218+
expectResourceVersionsByService map[types.NamespacedName]endpointSliceResourceVersions
219+
}{
220+
"same service": {
221+
updateParam: epSlice1,
222+
deleteServiceParam: &types.NamespacedName{Namespace: svcNS1, Name: svcName1},
223+
expectHas: false,
224+
expectStale: true,
225+
expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{},
226+
},
227+
"different namespace": {
228+
updateParam: epSlice1,
229+
deleteServiceParam: &types.NamespacedName{Namespace: svcNS2, Name: svcName1},
230+
expectHas: true,
231+
expectStale: false,
232+
expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{
233+
{Namespace: epSlice1.Namespace, Name: "svc1"}: {
234+
epSlice1.Name: epSlice1.ResourceVersion,
235+
},
236+
},
237+
},
238+
"different service": {
239+
updateParam: epSlice1,
240+
deleteServiceParam: &types.NamespacedName{Namespace: svcNS1, Name: svcName2},
241+
expectHas: true,
242+
expectStale: false,
243+
expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{
244+
{Namespace: epSlice1.Namespace, Name: "svc1"}: {
245+
epSlice1.Name: epSlice1.ResourceVersion,
246+
},
247+
},
248+
},
249+
}
250+
251+
for name, tc := range testCases {
252+
t.Run(name, func(t *testing.T) {
253+
esTracker := newEndpointSliceTracker()
254+
esTracker.Update(tc.updateParam)
255+
esTracker.DeleteService(tc.deleteServiceParam.Namespace, tc.deleteServiceParam.Name)
256+
if esTracker.Has(tc.updateParam) != tc.expectHas {
257+
t.Errorf("tc.tracker.Has(%+v) == %t, expected %t", tc.updateParam, esTracker.Has(tc.updateParam), tc.expectHas)
258+
}
259+
if esTracker.Stale(tc.updateParam) != tc.expectStale {
260+
t.Errorf("tc.tracker.Stale(%+v) == %t, expected %t", tc.updateParam, esTracker.Stale(tc.updateParam), tc.expectStale)
261+
}
262+
assert.Equal(t, tc.expectResourceVersionsByService, esTracker.resourceVersionsByService)
263+
})
264+
}
265+
}

‎pkg/controller/endpointslice/reconciler_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -983,7 +983,7 @@ func expectActions(t *testing.T, actions []k8stesting.Action, num int, verb, res
983983
}
984984

985985
func expectTrackedResourceVersion(t *testing.T, tracker *endpointSliceTracker, slice *discovery.EndpointSlice, expectedRV string) {
986-
rrv := tracker.relatedResourceVersions(slice)
986+
rrv, _ := tracker.relatedResourceVersions(slice)
987987
rv, tracked := rrv[slice.Name]
988988
if !tracked {
989989
t.Fatalf("Expected EndpointSlice %s to be tracked", slice.Name)

0 commit comments

Comments
 (0)