From dd4f1fb995a9511911d0ebf4f8770894ceac1217 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?= Date: Tue, 29 Nov 2022 11:24:17 +0100 Subject: [PATCH] fix: report deletion fighting with garbage collection (#5486) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Charles-Edouard Brétéché Signed-off-by: Charles-Edouard Brétéché --- .../report/admission/controller.go | 14 +++++++-- .../report/background/controller.go | 10 ++++-- pkg/controllers/report/resource/controller.go | 31 ++++++++++++------- 3 files changed, 38 insertions(+), 17 deletions(-) diff --git a/pkg/controllers/report/admission/controller.go b/pkg/controllers/report/admission/controller.go index 6e3d0a2834..0c53ec1d62 100644 --- a/pkg/controllers/report/admission/controller.go +++ b/pkg/controllers/report/admission/controller.go @@ -29,6 +29,7 @@ const ( Workers = 2 ControllerName = "admission-report-controller" maxRetries = 10 + deletionGrace = time.Minute * 2 ) type controller struct { @@ -61,7 +62,14 @@ func NewController( queue: queue, metadataCache: metadataCache, } - c.metadataCache.AddEventHandler(func(uid types.UID, _ schema.GroupVersionKind, _ resource.Resource) { queue.Add(cache.ExplicitKey(uid)) }) + c.metadataCache.AddEventHandler(func(eventType resource.EventType, uid types.UID, _ schema.GroupVersionKind, _ resource.Resource) { + // if it's a deletion, give some time to native garbage collection + if eventType == resource.Deleted { + queue.AddAfter(cache.ExplicitKey(uid), time.Minute) + } else { + queue.Add(cache.ExplicitKey(uid)) + } + }) controllerutils.AddEventHandlersT( admrInformer.Informer(), func(obj metav1.Object) { queue.Add(cache.ExplicitKey(reportutils.GetResourceUid(obj))) }, @@ -205,10 +213,10 @@ func (c *controller) cleanupReports(ctx context.Context, uid types.UID, hash str var toDelete []metav1.Object for _, report := range reports { if report.GetName() != string(uid) { - if reportutils.GetResourceHash(report) == hash || report.GetCreationTimestamp().Add(time.Minute*2).Before(time.Now()) { + if reportutils.GetResourceHash(report) == hash || report.GetCreationTimestamp().Add(deletionGrace).Before(time.Now()) { toDelete = append(toDelete, report) } else { - c.queue.AddAfter(cache.ExplicitKey(uid), time.Minute*2) + c.queue.AddAfter(cache.ExplicitKey(uid), deletionGrace) } } } diff --git a/pkg/controllers/report/background/controller.go b/pkg/controllers/report/background/controller.go index 9d7fc1ef8a..fc1a56636b 100644 --- a/pkg/controllers/report/background/controller.go +++ b/pkg/controllers/report/background/controller.go @@ -85,7 +85,11 @@ func NewController( } controllerutils.AddEventHandlersT(polInformer.Informer(), c.addPolicy, c.updatePolicy, c.deletePolicy) controllerutils.AddEventHandlersT(cpolInformer.Informer(), c.addPolicy, c.updatePolicy, c.deletePolicy) - c.metadataCache.AddEventHandler(func(uid types.UID, _ schema.GroupVersionKind, resource resource.Resource) { + c.metadataCache.AddEventHandler(func(eventType resource.EventType, uid types.UID, _ schema.GroupVersionKind, res resource.Resource) { + // if it's a deletion, nothing to do + if eventType == resource.Deleted { + return + } selector, err := reportutils.SelectorResourceUidEquals(uid) if err != nil { logger.Error(err, "failed to create label selector") @@ -93,10 +97,10 @@ func NewController( if err := c.enqueue(selector); err != nil { logger.Error(err, "failed to enqueue") } - if resource.Namespace == "" { + if res.Namespace == "" { c.queue.Add(string(uid)) } else { - c.queue.Add(resource.Namespace + "/" + string(uid)) + c.queue.Add(res.Namespace + "/" + string(uid)) } }) return &c diff --git a/pkg/controllers/report/resource/controller.go b/pkg/controllers/report/resource/controller.go index c35480a0a9..f6cff23998 100644 --- a/pkg/controllers/report/resource/controller.go +++ b/pkg/controllers/report/resource/controller.go @@ -40,7 +40,16 @@ type Resource struct { Hash string } -type EventHandler func(types.UID, schema.GroupVersionKind, Resource) +type EventType string + +const ( + Added EventType = "ADDED" + Modified EventType = "MODIFIED" + Deleted EventType = "DELETED" + Stopped EventType = "STOPPED" +) + +type EventHandler func(EventType, types.UID, schema.GroupVersionKind, Resource) type MetadataCache interface { GetResourceHash(uid types.UID) (Resource, schema.GroupVersionKind, bool) @@ -118,7 +127,7 @@ func (c *controller) AddEventHandler(eventHandler EventHandler) { c.eventHandlers = append(c.eventHandlers, eventHandler) for _, watcher := range c.dynamicWatchers { for uid, resource := range watcher.hashes { - eventHandler(uid, watcher.gvk, resource) + eventHandler(Added, uid, watcher.gvk, resource) } } } @@ -176,7 +185,7 @@ func (c *controller) updateDynamicWatchers(ctx context.Context) error { Namespace: obj.GetNamespace(), Name: obj.GetName(), } - c.notify(uid, gvk, hashes[uid]) + c.notify(Added, uid, gvk, hashes[uid]) } logger := logger.WithValues("resourceVersion", resourceVersion) logger.Info("start watcher ...") @@ -202,9 +211,9 @@ func (c *controller) updateDynamicWatchers(ctx context.Context) error { for event := range watchInterface.ResultChan() { switch event.Type { case watch.Added: - c.updateHash(event.Object.(*unstructured.Unstructured), gvr) + c.updateHash(Added, event.Object.(*unstructured.Unstructured), gvr) case watch.Modified: - c.updateHash(event.Object.(*unstructured.Unstructured), gvr) + c.updateHash(Modified, event.Object.(*unstructured.Unstructured), gvr) case watch.Deleted: c.deleteHash(event.Object.(*unstructured.Unstructured), gvr) } @@ -222,7 +231,7 @@ func (c *controller) updateDynamicWatchers(ctx context.Context) error { watcher.watcher.Stop() delete(oldDynamicWatcher, gvr) for uid, resource := range watcher.hashes { - c.notify(uid, watcher.gvk, resource) + c.notify(Stopped, uid, watcher.gvk, resource) } } return nil @@ -237,13 +246,13 @@ func (c *controller) stopDynamicWatchers() { c.dynamicWatchers = map[schema.GroupVersionResource]*watcher{} } -func (c *controller) notify(uid types.UID, gvk schema.GroupVersionKind, obj Resource) { +func (c *controller) notify(eventType EventType, uid types.UID, gvk schema.GroupVersionKind, obj Resource) { for _, handler := range c.eventHandlers { - handler(uid, gvk, obj) + handler(eventType, uid, gvk, obj) } } -func (c *controller) updateHash(obj *unstructured.Unstructured, gvr schema.GroupVersionResource) { +func (c *controller) updateHash(eventType EventType, obj *unstructured.Unstructured, gvr schema.GroupVersionResource) { c.lock.Lock() defer c.lock.Unlock() watcher, exists := c.dynamicWatchers[gvr] @@ -256,7 +265,7 @@ func (c *controller) updateHash(obj *unstructured.Unstructured, gvr schema.Group Namespace: obj.GetNamespace(), Name: obj.GetName(), } - c.notify(uid, watcher.gvk, watcher.hashes[uid]) + c.notify(eventType, uid, watcher.gvk, watcher.hashes[uid]) } } } @@ -269,7 +278,7 @@ func (c *controller) deleteHash(obj *unstructured.Unstructured, gvr schema.Group uid := obj.GetUID() hash := watcher.hashes[uid] delete(watcher.hashes, uid) - c.notify(uid, watcher.gvk, hash) + c.notify(Deleted, uid, watcher.gvk, hash) } }