From 087682584d5e61883853f8eafc3b6339e8606c52 Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Mon, 6 Jul 2020 21:51:00 +0800 Subject: [PATCH] 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. --- pkg/controller/endpointslice/BUILD | 1 + .../endpointslice/endpointslice_controller.go | 1 + .../endpointslice/endpointslice_tracker.go | 47 ++++++--- .../endpointslice_tracker_test.go | 99 ++++++++++++++++++- .../endpointslice/reconciler_test.go | 2 +- 5 files changed, 129 insertions(+), 21 deletions(-) diff --git a/pkg/controller/endpointslice/BUILD b/pkg/controller/endpointslice/BUILD index 67833854a78..4d83a1d7e60 100644 --- a/pkg/controller/endpointslice/BUILD +++ b/pkg/controller/endpointslice/BUILD @@ -66,6 +66,7 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/rand:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", diff --git a/pkg/controller/endpointslice/endpointslice_controller.go b/pkg/controller/endpointslice/endpointslice_controller.go index 2a7d37ac451..63098565a49 100644 --- a/pkg/controller/endpointslice/endpointslice_controller.go +++ b/pkg/controller/endpointslice/endpointslice_controller.go @@ -307,6 +307,7 @@ func (c *Controller) syncService(key string) error { if apierrors.IsNotFound(err) { c.triggerTimeTracker.DeleteService(namespace, name) c.reconciler.deleteService(namespace, name) + c.endpointSliceTracker.DeleteService(namespace, name) // The service has been deleted, return nil so that it won't be retried. return nil } diff --git a/pkg/controller/endpointslice/endpointslice_tracker.go b/pkg/controller/endpointslice/endpointslice_tracker.go index 7a0680ab739..d404edd353a 100644 --- a/pkg/controller/endpointslice/endpointslice_tracker.go +++ b/pkg/controller/endpointslice/endpointslice_tracker.go @@ -51,8 +51,11 @@ func (est *endpointSliceTracker) Has(endpointSlice *discovery.EndpointSlice) boo est.lock.Lock() defer est.lock.Unlock() - rrv := est.relatedResourceVersions(endpointSlice) - _, ok := rrv[endpointSlice.Name] + rrv, ok := est.relatedResourceVersions(endpointSlice) + if !ok { + return false + } + _, ok = rrv[endpointSlice.Name] return ok } @@ -63,7 +66,10 @@ func (est *endpointSliceTracker) Stale(endpointSlice *discovery.EndpointSlice) b est.lock.Lock() defer est.lock.Unlock() - rrv := est.relatedResourceVersions(endpointSlice) + rrv, ok := est.relatedResourceVersions(endpointSlice) + if !ok { + return true + } return rrv[endpointSlice.Name] != endpointSlice.ResourceVersion } @@ -73,33 +79,42 @@ func (est *endpointSliceTracker) Update(endpointSlice *discovery.EndpointSlice) est.lock.Lock() defer est.lock.Unlock() - rrv := est.relatedResourceVersions(endpointSlice) + rrv, ok := est.relatedResourceVersions(endpointSlice) + if !ok { + rrv = endpointSliceResourceVersions{} + est.resourceVersionsByService[getServiceNN(endpointSlice)] = rrv + } rrv[endpointSlice.Name] = endpointSlice.ResourceVersion } +// DeleteService removes the set of resource versions tracked for the Service. +func (est *endpointSliceTracker) DeleteService(namespace, name string) { + est.lock.Lock() + defer est.lock.Unlock() + + serviceNN := types.NamespacedName{Name: name, Namespace: namespace} + delete(est.resourceVersionsByService, serviceNN) +} + // Delete removes the resource version in this endpointSliceTracker for the // provided EndpointSlice. func (est *endpointSliceTracker) Delete(endpointSlice *discovery.EndpointSlice) { est.lock.Lock() defer est.lock.Unlock() - rrv := est.relatedResourceVersions(endpointSlice) - delete(rrv, endpointSlice.Name) + rrv, ok := est.relatedResourceVersions(endpointSlice) + if ok { + delete(rrv, endpointSlice.Name) + } } // relatedResourceVersions returns the set of resource versions tracked for the -// Service corresponding to the provided EndpointSlice. If no resource versions -// are currently tracked for this service, an empty set is initialized. -func (est *endpointSliceTracker) relatedResourceVersions(endpointSlice *discovery.EndpointSlice) endpointSliceResourceVersions { +// Service corresponding to the provided EndpointSlice, and a bool to indicate +// if it exists. +func (est *endpointSliceTracker) relatedResourceVersions(endpointSlice *discovery.EndpointSlice) (endpointSliceResourceVersions, bool) { serviceNN := getServiceNN(endpointSlice) vers, ok := est.resourceVersionsByService[serviceNN] - - if !ok { - vers = endpointSliceResourceVersions{} - est.resourceVersionsByService[serviceNN] = vers - } - - return vers + return vers, ok } // getServiceNN returns a namespaced name for the Service corresponding to the diff --git a/pkg/controller/endpointslice/endpointslice_tracker_test.go b/pkg/controller/endpointslice/endpointslice_tracker_test.go index 0ad704e2d11..500fa847556 100644 --- a/pkg/controller/endpointslice/endpointslice_tracker_test.go +++ b/pkg/controller/endpointslice/endpointslice_tracker_test.go @@ -19,8 +19,11 @@ package endpointslice import ( "testing" + "github.com/stretchr/testify/assert" + discovery "k8s.io/api/discovery/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" ) func TestEndpointSliceTrackerUpdate(t *testing.T) { @@ -43,34 +46,55 @@ func TestEndpointSliceTrackerUpdate(t *testing.T) { epSlice1DifferentRV.ResourceVersion = "rv2" testCases := map[string]struct { - updateParam *discovery.EndpointSlice - checksParam *discovery.EndpointSlice - expectHas bool - expectStale bool + updateParam *discovery.EndpointSlice + checksParam *discovery.EndpointSlice + expectHas bool + expectStale bool + expectResourceVersionsByService map[types.NamespacedName]endpointSliceResourceVersions }{ "same slice": { updateParam: epSlice1, checksParam: epSlice1, expectHas: true, expectStale: false, + expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{ + {Namespace: epSlice1.Namespace, Name: "svc1"}: { + epSlice1.Name: epSlice1.ResourceVersion, + }, + }, }, "different namespace": { updateParam: epSlice1, checksParam: epSlice1DifferentNS, expectHas: false, expectStale: true, + expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{ + {Namespace: epSlice1.Namespace, Name: "svc1"}: { + epSlice1.Name: epSlice1.ResourceVersion, + }, + }, }, "different service": { updateParam: epSlice1, checksParam: epSlice1DifferentService, expectHas: false, expectStale: true, + expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{ + {Namespace: epSlice1.Namespace, Name: "svc1"}: { + epSlice1.Name: epSlice1.ResourceVersion, + }, + }, }, "different resource version": { updateParam: epSlice1, checksParam: epSlice1DifferentRV, expectHas: true, expectStale: true, + expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{ + {Namespace: epSlice1.Namespace, Name: "svc1"}: { + epSlice1.Name: epSlice1.ResourceVersion, + }, + }, }, } @@ -84,6 +108,7 @@ func TestEndpointSliceTrackerUpdate(t *testing.T) { if esTracker.Stale(tc.checksParam) != tc.expectStale { t.Errorf("tc.tracker.Stale(%+v) == %t, expected %t", tc.checksParam, esTracker.Stale(tc.checksParam), tc.expectStale) } + assert.Equal(t, tc.expectResourceVersionsByService, esTracker.resourceVersionsByService) }) } } @@ -172,3 +197,69 @@ func TestEndpointSliceTrackerDelete(t *testing.T) { }) } } + +func TestEndpointSliceTrackerDeleteService(t *testing.T) { + svcName1, svcNS1 := "svc1", "ns1" + svcName2, svcNS2 := "svc2", "ns2" + epSlice1 := &discovery.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example-1", + Namespace: svcNS1, + ResourceVersion: "rv1", + Labels: map[string]string{discovery.LabelServiceName: svcName1}, + }, + } + + testCases := map[string]struct { + updateParam *discovery.EndpointSlice + deleteServiceParam *types.NamespacedName + expectHas bool + expectStale bool + expectResourceVersionsByService map[types.NamespacedName]endpointSliceResourceVersions + }{ + "same service": { + updateParam: epSlice1, + deleteServiceParam: &types.NamespacedName{Namespace: svcNS1, Name: svcName1}, + expectHas: false, + expectStale: true, + expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{}, + }, + "different namespace": { + updateParam: epSlice1, + deleteServiceParam: &types.NamespacedName{Namespace: svcNS2, Name: svcName1}, + expectHas: true, + expectStale: false, + expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{ + {Namespace: epSlice1.Namespace, Name: "svc1"}: { + epSlice1.Name: epSlice1.ResourceVersion, + }, + }, + }, + "different service": { + updateParam: epSlice1, + deleteServiceParam: &types.NamespacedName{Namespace: svcNS1, Name: svcName2}, + expectHas: true, + expectStale: false, + expectResourceVersionsByService: map[types.NamespacedName]endpointSliceResourceVersions{ + {Namespace: epSlice1.Namespace, Name: "svc1"}: { + epSlice1.Name: epSlice1.ResourceVersion, + }, + }, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + esTracker := newEndpointSliceTracker() + esTracker.Update(tc.updateParam) + esTracker.DeleteService(tc.deleteServiceParam.Namespace, tc.deleteServiceParam.Name) + if esTracker.Has(tc.updateParam) != tc.expectHas { + t.Errorf("tc.tracker.Has(%+v) == %t, expected %t", tc.updateParam, esTracker.Has(tc.updateParam), tc.expectHas) + } + if esTracker.Stale(tc.updateParam) != tc.expectStale { + t.Errorf("tc.tracker.Stale(%+v) == %t, expected %t", tc.updateParam, esTracker.Stale(tc.updateParam), tc.expectStale) + } + assert.Equal(t, tc.expectResourceVersionsByService, esTracker.resourceVersionsByService) + }) + } +} diff --git a/pkg/controller/endpointslice/reconciler_test.go b/pkg/controller/endpointslice/reconciler_test.go index 9e52da9934b..c9f89c74376 100644 --- a/pkg/controller/endpointslice/reconciler_test.go +++ b/pkg/controller/endpointslice/reconciler_test.go @@ -983,7 +983,7 @@ func expectActions(t *testing.T, actions []k8stesting.Action, num int, verb, res } func expectTrackedResourceVersion(t *testing.T, tracker *endpointSliceTracker, slice *discovery.EndpointSlice, expectedRV string) { - rrv := tracker.relatedResourceVersions(slice) + rrv, _ := tracker.relatedResourceVersions(slice) rv, tracked := rrv[slice.Name] if !tracked { t.Fatalf("Expected EndpointSlice %s to be tracked", slice.Name)