Adds selective owner reference removal - #2601
Conversation
Signed-off-by: raaizik <132667934+raaizik@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
📝 WalkthroughWalkthroughThe ChangesSelective Owner Reference Removal
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
37f71be to
81a2b59
Compare
Remove only the specified owner's reference by matching Name and UID. Follows Kubernetes standard approach while working within metav1.Object interface constraints. Preserves other ownerReferences for proper garbage collection. Resolves RamenDR#2600 Signed-off-by: raaizik <132667934+raaizik@users.noreply.github.com>
81a2b59 to
4de9c63
Compare
| } | ||
|
|
||
| newOwnerRefs = append(newOwnerRefs, ref) | ||
| } |
There was a problem hiding this comment.
We can replace the loop and variables with:
newOwnerRefs := slices.DeleteFunc(currentOwnerRefs, func(ref metav1.OwnerReference) bool {
return ref.UID == ownerUID
})There was a problem hiding this comment.
slices.DeleteFunc reuses the underlying array and only adjusts the slice length. This can be problematic when modifying ownerReferences that may be referenced elsewhere in the code potentially leading to unexpected behavior. The current approach creates a new slice with its own backing array. Since we typically have O(1) ownerReferences per resource the performance difference is negligible.
There was a problem hiding this comment.
No code should reference the slice returned by obj.GetOwnerReferences() - if we have such code it should be fixed. If code need to keep this is must copy the slice to other code cannot modify the slice. So modifying the slice here is safe - it will not lead to unexpected behavior.
The main motivation for using slices.DeleteFunc() is to avoid the manual code creating a new slices. We should use go standard library for common operations instead of reinvent them everywhere.
This will minimize the code we need to maintain and minimize the executable size and memory usage.
In case when we must clone the slice because it is references elsewhere we can use slices.Clone().
|
|
||
| return true, nil | ||
| newOwnerRefs := []metav1.OwnerReference{} | ||
| removed := false |
There was a problem hiding this comment.
The flag is not needed - we can return:
return len(currentOwnerRefs) != len(newOwnerRef), nil
|
A trivial unit tests can be useful, checking that we handle all cases:
|
Yes, we have 9 options: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue The reason to use Fixes is that this is the convention in the current code, and it makes it easier to inspect git log. My preferences is: This way we can use tools that extract trailers from git messages to get metadata. |
|
@raaizik Fixing removal of owner references is nice - but all 3 builds failed with this change int the same way: https://github.com/RamenDR/ramen/actions/runs/27356176128/job/80830943357 https://github.com/RamenDR/ramen/actions/runs/27356176128/attempts/2?pr=2601 https://github.com/RamenDR/ramen/actions/runs/27356176128/attempts/3?pr=2601 This is not a random failure (happens from time to time). The old behavior of removing all owner references was correct, and the new behavior is not. You can download the logs from the builds and inspect them. You should be able to reproduce the same in your local env. |
nirs
left a comment
nirs
left a comment
There was a problem hiding this comment.
All e2e builds failed - the change is wrong.
|
Actually @BenamarMk that call site where the TODO is was meant to remove all OwnerRefs, not selectively. The VolSync cleanup functions (ReleasePVCOwnership and UnprotectVolSyncPVC) need to completely disown PVCs by removing all owner references. What can be done for clarity is the code could be refactored with two distinct utility functions:
The selective removal logic already exists in vshandler's |
Is it possible that there are other refs that we don't control and we need to remove only our refs? This can be solved by supporting multiple refs and remove all refs we manage: If no other ref is expected - nobody should own our resources - using RemoveAll and RemoveOne sounds good. If the current code need to remove all and we don't need yet selective remove we can just rename the current function which is wrong - it claims to remove one ref but it removes all. |
@nirs I just wrote that we do need selective removal, to quote myself:
|
|
|
||
| // Always update ownerReferences to maintain consistency | ||
| // Return true only if we actually removed an owner | ||
| obj.SetOwnerReferences(newOwnerRefs) |
There was a problem hiding this comment.
Why do we need to update ownerReferences if no owner reference was removed? If no matching owner reference was found, then newOwnerRefs should be identical to currentOwnerRefs
There was a problem hiding this comment.
We need to set only if we changed the value.
There was a problem hiding this comment.
Yes, but the comment says: "Always update ownerReferences to maintain consistency" and we do it always indeed.
4 participants
Resolves #2600
Summary by CodeRabbit