Skip to content

Commit 7d598d7

Browse files
aman4433k8s-publishing-bot
authored andcommitted
Refactor: Contextualize CRDFinalizer to fix goroutine leak
Signed-off-by: Aman Shrivastava <amanshrivastava118@gmail.com> Kubernetes-commit: f60f5b24605d98fe5976dd3a7dc554333bb85e07
1 parent 8db5ab6 commit 7d598d7

1 file changed

Lines changed: 19 additions & 15 deletions

File tree

pkg/controller/finalizer/crd_finalizer.go

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ type CRDFinalizer struct {
6464
crdSynced cache.InformerSynced
6565

6666
// To allow injection for testing.
67-
syncFn func(key string) error
67+
syncFn func(ctx context.Context, key string) error
6868

6969
queue workqueue.TypedRateLimitingInterface[string]
7070
}
@@ -109,7 +109,7 @@ func NewCRDFinalizer(
109109
return c
110110
}
111111

112-
func (c *CRDFinalizer) sync(key string) error {
112+
func (c *CRDFinalizer) sync(ctx context.Context, key string) error {
113113
cachedCRD, err := c.crdLister.Get(key)
114114
if apierrors.IsNotFound(err) {
115115
return nil
@@ -132,7 +132,7 @@ func (c *CRDFinalizer) sync(key string) error {
132132
Reason: "InstanceDeletionInProgress",
133133
Message: "CustomResource deletion is in progress",
134134
})
135-
crd, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(context.TODO(), crd, metav1.UpdateOptions{})
135+
crd, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(ctx, crd, metav1.UpdateOptions{})
136136
if apierrors.IsNotFound(err) || apierrors.IsConflict(err) {
137137
// deleted or changed in the meantime, we'll get called again
138138
return nil
@@ -152,10 +152,10 @@ func (c *CRDFinalizer) sync(key string) error {
152152
Message: "instances overlap with built-in resources in storage",
153153
})
154154
} else if apiextensionshelpers.IsCRDConditionTrue(crd, apiextensionsv1.Established) {
155-
cond, deleteErr := c.deleteInstances(crd)
155+
cond, deleteErr := c.deleteInstances(ctx, crd)
156156
apiextensionshelpers.SetCRDCondition(crd, cond)
157157
if deleteErr != nil {
158-
if _, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(context.TODO(), crd, metav1.UpdateOptions{}); err != nil {
158+
if _, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(ctx, crd, metav1.UpdateOptions{}); err != nil {
159159
utilruntime.HandleError(err)
160160
}
161161
return deleteErr
@@ -170,15 +170,15 @@ func (c *CRDFinalizer) sync(key string) error {
170170
}
171171

172172
apiextensionshelpers.CRDRemoveFinalizer(crd, apiextensionsv1.CustomResourceCleanupFinalizer)
173-
_, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(context.TODO(), crd, metav1.UpdateOptions{})
173+
_, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(ctx, crd, metav1.UpdateOptions{})
174174
if apierrors.IsNotFound(err) || apierrors.IsConflict(err) {
175175
// deleted or changed in the meantime, we'll get called again
176176
return nil
177177
}
178178
return err
179179
}
180180

181-
func (c *CRDFinalizer) deleteInstances(crd *apiextensionsv1.CustomResourceDefinition) (apiextensionsv1.CustomResourceDefinitionCondition, error) {
181+
func (c *CRDFinalizer) deleteInstances(ctx context.Context, crd *apiextensionsv1.CustomResourceDefinition) (apiextensionsv1.CustomResourceDefinitionCondition, error) {
182182
// Now we can start deleting items. While it would be ideal to use a REST API client, doing so
183183
// could incorrectly delete a ThirdPartyResource with the same URL as the CustomResource, so we go
184184
// directly to the storage instead. Since we control the storage, we know that delete collection works.
@@ -193,7 +193,6 @@ func (c *CRDFinalizer) deleteInstances(crd *apiextensionsv1.CustomResourceDefini
193193
}, err
194194
}
195195

196-
ctx := genericapirequest.NewContext()
197196
allResources, err := crClient.List(ctx, nil)
198197
if err != nil {
199198
return apiextensionsv1.CustomResourceDefinitionCondition{
@@ -263,37 +262,42 @@ func (c *CRDFinalizer) deleteInstances(crd *apiextensionsv1.CustomResourceDefini
263262
}
264263

265264
func (c *CRDFinalizer) Run(workers int, stopCh <-chan struct{}) {
265+
c.RunWithContext(workers, wait.ContextForChannel(stopCh))
266+
}
267+
268+
//logcheck:context // RunWithContext should be used instead of Run in code which supports contextual logging.
269+
func (c *CRDFinalizer) RunWithContext(workers int, ctx context.Context) {
266270
defer utilruntime.HandleCrash()
267271
defer c.queue.ShutDown()
268272

269273
klog.Info("Starting CRDFinalizer")
270274
defer klog.Info("Shutting down CRDFinalizer")
271275

272-
if !cache.WaitForCacheSync(stopCh, c.crdSynced) {
276+
if !cache.WaitForCacheSync(ctx.Done(), c.crdSynced) {
273277
return
274278
}
275279

276280
for i := 0; i < workers; i++ {
277-
go wait.Until(c.runWorker, time.Second, stopCh)
281+
go wait.UntilWithContext(ctx, c.runWorker, time.Second)
278282
}
279283

280-
<-stopCh
284+
<-ctx.Done()
281285
}
282286

283-
func (c *CRDFinalizer) runWorker() {
284-
for c.processNextWorkItem() {
287+
func (c *CRDFinalizer) runWorker(ctx context.Context) {
288+
for c.processNextWorkItem(ctx) {
285289
}
286290
}
287291

288292
// processNextWorkItem deals with one key off the queue. It returns false when it's time to quit.
289-
func (c *CRDFinalizer) processNextWorkItem() bool {
293+
func (c *CRDFinalizer) processNextWorkItem(ctx context.Context) bool {
290294
key, quit := c.queue.Get()
291295
if quit {
292296
return false
293297
}
294298
defer c.queue.Done(key)
295299

296-
err := c.syncFn(key)
300+
err := c.syncFn(ctx, key)
297301
if err == nil {
298302
c.queue.Forget(key)
299303
return true

0 commit comments

Comments
 (0)