Skip to content

Commit 0876825

Browse files
tnqntnqn
committed
Fix memory leak in endpointSliceTracker
endpointSliceTracker creates a set of resource versions for each service, the resource versions in the set could be deleted when endpointslices are deleted, but the set and its key in the map is never deleted, leading to memory leak. This patch deletes the set if the service is deleted, and stops initializing an empty set when "read-only" methods "Has" and "Stale" are called.
1 parent 865cbf0 commit 0876825

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)