Avoid race-condition removing object with multiple finalizers#160
Avoid race-condition removing object with multiple finalizers#160crobby merged 4 commits intorancher:mainfrom
Conversation
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
|
|
||
| queue := workqueue.NewRateLimitingQueue(workqueue.DefaultControllerRateLimiter()) |
There was a problem hiding this comment.
These calls are now deprecated in k8s/client-go v0.32 but dang if I can figure out how to use the generic equivalents.
There was a problem hiding this comment.
Correct, it's actually lasso's controller which has some field with a deprecated type. Happy to help refactoring that to adopt new types based on generics in a separate PR.
There was a problem hiding this comment.
After taking a quick look, and have some comments about this:
- The new typed interface allows using
stringinstead ofany, getting stricter type-checking and avoiding checks like this.- (The deprecated
RateLimitingQueueis just an "alias" ofTypedRateLimitingQueue[any]
- (The deprecated
- This leads to a breaking change, since
RateLimiterfromOptionsshould also need to changed toTypedRateLimiter[string]. - Sadly, only the
workqueuewas adapted to support generics. It would be much more interesting if theSharedIndexInformerand itsStorewere also parameterized, so we don't need to cast objects toruntime.Object.
Taking that into account, I believe there is not much motivation to move this forward at the moment. We could still migrate to use TypedRateLimitingQueue[any], which will keep the compatibility with existing code but also completely miss the benefit of using generics. Also, that could also even reduce our chances of ever migrating to use the correct type.
There was a problem hiding this comment.
Here is a draft PR with the changes, for completeness:
#162
please feel free to do whatever you think appropriate with it
ericpromislow
left a comment
ericpromislow
left a comment
There was a problem hiding this comment.
It's fine. Not worth dealing with the deprecated functions for now
3 participants
Refers to rancher/rancher#49328
This PR adds a (failing) unit test for the issue described in the parent PR. Then, it also introduces a fix based on:
processSingleItem(instead ofAddRateLimited, which may retry immediately).deletionTimestampnot nil and and empty finalizers).retryAftermechanism to delay its execution (waiting for theDELETEevent, which will prune the object from the informer store and also enqueue this key immediately).