From 1056cef384517e6ef1ef1f7b0724587bd3da2a1e Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 18 Dec 2020 09:36:28 -0800 Subject: [PATCH] Sync garbage collector controller less often by adjusting its filters - Only sync on add/update of secrets in the same namespace which have the "storage.pinniped.dev/garbage-collect-after" annotation, and also during a full resync of the informer whenever secrets in the same namespace with that annotation exist. - Ignore deleted secrets to avoid having this controller trigger itself unnecessarily when it deletes a secret. This controller is never interested in deleted secrets, since its only job is to delete existing secrets. - No change to the self-imposed rate limit logic. That still applies because secrets with this annotation will be created and updated regularly while the system is running (not just during rare system configuration steps). --- .../supervisorstorage/garbage_collector.go | 17 ++++++- .../garbage_collector_test.go | 45 ++++++++++++++----- ...ervisor_storage_garbage_collection_test.go | 19 ++++---- 3 files changed, 60 insertions(+), 21 deletions(-) diff --git a/internal/controller/supervisorstorage/garbage_collector.go b/internal/controller/supervisorstorage/garbage_collector.go index d2f6aef3..e2d35d30 100644 --- a/internal/controller/supervisorstorage/garbage_collector.go +++ b/internal/controller/supervisorstorage/garbage_collector.go @@ -34,6 +34,14 @@ func GarbageCollectorController( secretInformer corev1informers.SecretInformer, withInformer pinnipedcontroller.WithInformerOptionFunc, ) controllerlib.Controller { + isSecretWithGCAnnotation := func(obj metav1.Object) bool { + secret, ok := obj.(*v1.Secret) + if !ok { + return false + } + _, ok = secret.Annotations[crud.SecretLifetimeAnnotationKey] + return ok + } return controllerlib.New( controllerlib.Config{ Name: "garbage-collector-controller", @@ -45,7 +53,14 @@ func GarbageCollectorController( }, withInformer( secretInformer, - pinnipedcontroller.MatchAnythingFilter(nil), + controllerlib.FilterFuncs{ + AddFunc: isSecretWithGCAnnotation, + UpdateFunc: func(oldObj, newObj metav1.Object) bool { + return isSecretWithGCAnnotation(oldObj) || isSecretWithGCAnnotation(newObj) + }, + DeleteFunc: func(obj metav1.Object) bool { return false }, // ignore all deletes + ParentFunc: nil, + }, controllerlib.InformerOption{}, ), ) diff --git a/internal/controller/supervisorstorage/garbage_collector_test.go b/internal/controller/supervisorstorage/garbage_collector_test.go index 81f104df..a21067ee 100644 --- a/internal/controller/supervisorstorage/garbage_collector_test.go +++ b/internal/controller/supervisorstorage/garbage_collector_test.go @@ -48,22 +48,47 @@ func TestGarbageCollectorControllerInformerFilters(t *testing.T) { when("watching Secret objects", func() { var ( - subject controllerlib.Filter - secret, otherSecret *corev1.Secret + subject controllerlib.Filter + secretWithAnnotation, otherSecret *corev1.Secret ) it.Before(func() { subject = secretsInformerFilter - secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}} - otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}} + secretWithAnnotation = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace", Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": "some timestamp", + }}} + otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-namespace"}} }) - when("any Secret changes", func() { - it("returns true to trigger the sync function for all secrets", func() { - r.True(subject.Add(secret)) - r.True(subject.Update(secret, otherSecret)) - r.True(subject.Update(otherSecret, secret)) - r.True(subject.Delete(secret)) + when("any Secret with the required annotation is added or updated", func() { + it("returns true to trigger the sync function", func() { + r.True(subject.Add(secretWithAnnotation)) + r.True(subject.Update(secretWithAnnotation, otherSecret)) + r.True(subject.Update(otherSecret, secretWithAnnotation)) + }) + }) + + when("any Secret with the required annotation is deleted", func() { + it("returns false to skip the sync function because it does not need to worry about secrets that are already gone", func() { + r.False(subject.Delete(secretWithAnnotation)) + }) + }) + + when("any Secret without the required annotation changes", func() { + it("returns false to skip the sync function", func() { + r.False(subject.Add(otherSecret)) + r.False(subject.Update(otherSecret, otherSecret)) + r.False(subject.Delete(otherSecret)) + }) + }) + + when("any other type is passed", func() { + it("returns false to skip the sync function", func() { + wrongType := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "some-ns", Namespace: "some-ns"}} + + r.False(subject.Add(wrongType)) + r.False(subject.Update(wrongType, wrongType)) + r.False(subject.Delete(wrongType)) }) }) }) diff --git a/test/integration/supervisor_storage_garbage_collection_test.go b/test/integration/supervisor_storage_garbage_collection_test.go index a80a6a79..e7efa159 100644 --- a/test/integration/supervisor_storage_garbage_collection_test.go +++ b/test/integration/supervisor_storage_garbage_collection_test.go @@ -44,11 +44,12 @@ func TestStorageGarbageCollection(t *testing.T) { } // Start a background goroutine which will end as soon as the test ends. - // Keep updating a secret in the same namespace just to get the controller to respond faster. - // This is just a performance optimization because otherwise this test has to wait - // ~3 minutes for the controller's next full-resync. + // Keep updating a secret which has the "storage.pinniped.dev/garbage-collect-after" annotation + // in the same namespace just to get the controller to respond faster. + // This is just a performance optimization to make this test pass faster because otherwise + // this test has to wait ~3 minutes for the controller's next full-resync. stopCh := make(chan bool, 1) // It is important that this channel be buffered. - go createAndUpdateSecretEveryTwoSeconds(t, stopCh, secrets) + go updateSecretEveryTwoSeconds(t, stopCh, secrets, secretNotYetExpired) t.Cleanup(func() { stopCh <- true }) @@ -68,12 +69,10 @@ func TestStorageGarbageCollection(t *testing.T) { require.NoError(t, err) } -func createAndUpdateSecretEveryTwoSeconds(t *testing.T, stopCh chan bool, secrets corev1client.SecretInterface) { +func updateSecretEveryTwoSeconds(t *testing.T, stopCh chan bool, secrets corev1client.SecretInterface, secret *v1.Secret) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() - unrelatedSecret := createSecret(ctx, t, secrets, "unrelated-to-gc", time.Time{}) - i := 0 for { select { @@ -87,9 +86,9 @@ func createAndUpdateSecretEveryTwoSeconds(t *testing.T, stopCh chan bool, secret time.Sleep(2 * time.Second) i++ - unrelatedSecret.Data["foo"] = []byte(fmt.Sprintf("bar-%d", i)) + secret.Data["foo"] = []byte(fmt.Sprintf("bar-%d", i)) var updateErr error - unrelatedSecret, updateErr = secrets.Update(ctx, unrelatedSecret, metav1.UpdateOptions{}) + secret, updateErr = secrets.Update(ctx, secret, metav1.UpdateOptions{}) require.NoError(t, updateErr) } } @@ -125,6 +124,6 @@ func newSecret(namePrefix string, expiresAt time.Time) *v1.Secret { Annotations: annotations, }, Data: map[string][]byte{"some-key": []byte("fake-data")}, - Type: "storage.pinniped.dev/gc-test-integration-test", + Type: "storage.pinniped.dev/gc-test-integration-test", // the garbage collector controller doesn't care about the type } }