-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix: resourceTree node duplication caused by having multiple OwnerReferences (#19910) #19911
base: master
Are you sure you want to change the base?
Conversation
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
✅ Preview Environment created on Bunnyshell but will not be auto-deployedSee: Environment Details Available commands (reply to this comment):
|
…erences for some resources. Signed-off-by: DomineCore <fengyafei0405@163.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19911 +/- ##
==========================================
+ Coverage 55.81% 55.84% +0.02%
==========================================
Files 320 320
Lines 44378 44382 +4
==========================================
+ Hits 24769 24783 +14
+ Misses 17046 17029 -17
- Partials 2563 2570 +7 ☔ View full report in Codecov by Sentry. |
issue: #19910 |
Who can help me review my code? |
nodesMap := make(map[kube.ResourceKey]appv1.ResourceNode) | ||
err = ctrl.stateCache.IterateHierarchyV2(a.Spec.Destination.Server, managedResourcesKeys, func(child appv1.ResourceNode, appName string) bool { |
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 problem is caused by the IterateHierarchyV2 implementation. It is unexpected to have the callback happen twice for the same node. The fix would need to be implemented in https://github.com/argoproj/gitops-engine. I think @andrii-korotkov-verkada can help you figure it out, but you should be able to write a unit tests in that repo and test it when a resource has 2 owner references.
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.
Thank you for your analysis. I will take a look at the IterateHierarchyV2 implementation and see if I can address the issue in the https://github.com/argoproj/gitops-engine repository. I also appreciate your suggestion to involve @andrii-korotkov-verkada. I will reach out to him/her for assistance. Additionally, I will work on writing unit tests as you suggested and test when a resource has two owner references. Thank you again for your valuable input.
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.
What's the use case for having multiple owner references? My understanding is that this breaks a tree-like structure of the resources and not sure what's the best way to support this. We can pick one of parents, though I thought that's what should be happening based on visited flagging. Do you see a whole sub-tree being added or only the top-most entry?
…erences for some resources.
Checklist: