fix: show namespaced resources owned by cluster scoped resources #366
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #366 +/- ##
==========================================
+ Coverage 54.71% 55.23% +0.51%
==========================================
Files 41 41
Lines 4834 4908 +74
==========================================
+ Hits 2645 2711 +66
- Misses 1977 1985 +8
Partials 212 212 ☔ View full report in Codecov by Sentry. |
|
Is this issue dead? |
|
@empath-nirvana Sorry! We moved it to v2.4 and I got busy with other stuff. I will try to address the comments soon and get them merged |
chetan-rns
force-pushed
the
fix-cluster-resources
branch
2 times, most recently
from
5b257f8
to
a5891b2
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
Are we still waiting on this? |
|
when can this be added? we have a cluster scoped resource to represent a "tenant", which spins off many resources into a new namespace with the corresponding namespace name. without this merge we can only see the namespace, which is a huge bummer |
|
@alexmt can you take another look? |
|
is this issue dead? would love to see this feature get added |
|
I would also really like to have this feature. It's a big deficiency from the perspective of our management team. What is blocking this feature being added? Are more changes needed? Is there an external dependency? |
|
@alexmt @crenshaw-dev The PR is ready for review. Please let me know if there are any changes needed |
|
@alexmt You asked me at kubecon to remind you about this PR -- said it's possible to enable this as a feature flag because of performance implications? |
|
Thank you for bringing this up @empath-nirvana ! Yeah, main concern was how this change might affect performance. Adding a feature flag is a good compromise. |
|
@chetan-rns can you please allow conditionally disable analyzing cluster level resource children to mitigate performance degradation? |
|
@chetan-rns Is there any progress on this issue? |
chetan-rns
force-pushed
the
fix-cluster-resources
branch
from
a5891b2
to
cb6b120
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
@chetan-rns are there any remaining blockers for this feature to be added? Would love to see this make it into a release! |
|
@the-scott-hand I've resolved the review comments @crenshaw-dev @leoluz Please do let me know if there are any other changes required to get this in. |
pkg/cache/cluster.go
Outdated
| @@ -1032,6 +1050,12 @@ func (c *clusterCache) onNodeRemoved(key kube.ResourceKey) { | |||
| if len(ns) == 0 { | |||
| delete(c.nsIndex, key.Namespace) | |||
| } | |||
|
|
|||
| nsAll, ok := c.nsIndex[""] | |||
There was a problem hiding this comment.
I don't understand this line. Why c.nsIndex[""] and then delete by key instead of "".
There was a problem hiding this comment.
Sorry for the confusion. The nsIndex is a map of map and I was using a reference of the internal map in IterateHierarchy() . Since it was updating the original nsIndex I was removing the updated element here.
I have now changed it to use a copy of the internal map instead of a reference so that we get rid of this update here.
|
@leoluz @crenshaw-dev @chetan-rns any blockers or remaining changes that need to be made to get this merged and included in a release? |
|
We really would love to get this fix here, as we really want argo to manage and track resources created by operators, such as the gpu operator by nvidia, it hits this bug when you deploy it |
|
@chetan-rns are you still able to work on this? |
chetan-rns
force-pushed
the
fix-cluster-resources
branch
2 times, most recently
from
597e8ee
to
78e42e9
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
@crenshaw-dev Thank you for your patience. I've addressed your comments. PTAL. |
|
Is there anything blocking this PR? |
|
@chetan-rns could you open an Argo CD PR using this branch so we can run tests there? Besides that, I think this is good to go. |
chetan-rns
force-pushed
the
fix-cluster-resources
branch
from
99788eb
to
b1e7466
Compare
|
|
@crenshaw-dev I've rebased the existing Argo CD PR pointing to this branch. I think the SonarCloud check is failing due to issues already in the master. |
|
@chetan-rns @crenshaw-dev Anything else blocking this PR? Would love to see this feature finally added! |
chetan-rns
force-pushed
the
fix-cluster-resources
branch
from
40d7e7a
to
2b8fa3e
Compare
Signed-off-by: Chetan Banavikalmutt <chetanrns1997@gmail.com>
Signed-off-by: Chetan Banavikalmutt <chetanrns1997@gmail.com>
Signed-off-by: Chetan Banavikalmutt <chetanrns1997@gmail.com>
Signed-off-by: Chetan Banavikalmutt <chetanrns1997@gmail.com>
Signed-off-by: Chetan Banavikalmutt <chetanrns1997@gmail.com>
Signed-off-by: Chetan Banavikalmutt <chetanrns1997@gmail.com>
Signed-off-by: Chetan Banavikalmutt <chetanrns1997@gmail.com>
Signed-off-by: Lukas Wöhrl <lukas.woehrl@plentymarkets.com>
chetan-rns
force-pushed
the
fix-cluster-resources
branch
from
2b8fa3e
to
1c693c0
Compare
|
Currently, the namespaced resources owned by cluster scoped resources are not shown on the UI. This PR updates
IterateHierarchy()to consider all resources(both cluster and namespace scoped) while managing a cluster-level object.Part of: argoproj/argo-cd#7733
Signed-off-by: Chetan Banavikalmutt chetanrns1997@gmail.com