-
-
Notifications
You must be signed in to change notification settings - Fork 364
fix(predicate): track resource UID to handle recreated resources #1836
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixes kube-rs#1830 Resources recreated with the same name/namespace but different UIDs were incorrectly filtered out by the predicate filter. The cache now tracks both ObjectRef and UID to distinguish between different resource instances. The bug occurred because ObjectRef deliberately excludes UID from its Hash implementation (for general-purpose use), but PredicateFilter needs to distinguish between different resource instances. Changes: - Modified PredicateFilter cache to HashMap<ObjectRef<K>, (Option<String>, u64)> - Updated poll_next to check both UID and predicate value changes - Added regression test for recreated resources with same generation The fix ensures that when a resource is deleted and recreated with the same name/namespace but a different UID (and same generation), the new resource is properly reconciled by controllers using predicate filters. Signed-off-by: doxxx93 <[email protected]>
|
i will fix lint soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot for this!
i have one comment initially because it solves it in a way i did not expect, but it's all private implementation details. i just want to make it as simple as possible 😄
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1836 +/- ##
=======================================
+ Coverage 74.5% 74.6% +0.1%
=======================================
Files 84 84
Lines 7856 7877 +21
=======================================
+ Hits 5849 5871 +22
+ Misses 2007 2006 -1
🚀 New features to boost your workflow:
|
…dicated cache key Signed-off-by: doxxx93 <[email protected]>
2b84017 to
b683a2e
Compare
|
hum.. sorry for lint |
Reorder imports alphabetically to match nightly rustfmt requirements. Signed-off-by: doxxx93 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks great. I would like one extra thing in the tests ideally, and have a comment suggestion, but tiny things. Thanks a lot. Looks very clean.
…creation scenarios Signed-off-by: doxxx93 <[email protected]>
7e79179 to
eaa580e
Compare
…ion scenarios Signed-off-by: doxxx93 <[email protected]>
…across generations Signed-off-by: doxxx93 <[email protected]>
Description
Fixes #1830
Resources recreated with the same name/namespace but different UIDs were incorrectly filtered out by the predicate filter. This PR fixes the issue by tracking UIDs in the cache to distinguish between different resource instances.
Problem
When using predicate filters (e.g.,
predicates::generation), if a resource is:The recreated resource would be incorrectly filtered out because the cache only used
ObjectRefas the key, which deliberately excludes UID from its Hash implementation.This caused controllers to miss reconciliation of recreated resources, breaking scenarios where resources need to be managed via owner references and garbage collection.
Solution
Created a new
PredicateCacheKeystruct that includes UID in the cache key:Modified
PredicateFiltercache from:to:
This ensures different resource instances (with different UIDs) are treated as separate cache entries.
Changes
PredicateCacheKeystruct that includes UID in equality/hashingPredicateFiltercache to usePredicateCacheKeyinstead ofObjectRefpoll_nextto create cache keys that include UIDpredicate_filtering_should_handle_recreated_resources_with_same_generationTesting
All existing tests pass (67 passed), including new regression test that verifies:
cargo test --package kube-runtime --libChecklist