🐛 fix addon namespace deletion in multi-hub scenarios #1153
Conversation
WalkthroughAdds a helper to detect other active klusterlet agent namespaces and uses it in managed reconcile to conditionally skip deleting the shared addon namespace; also adds tests verifying the helper and cleanup behavior across multiple namespace/label scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.2.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/product/migration-guide for migration instructions ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go (1)
188-197: Wrap the propagated error with %w for proper error chaining.Use %w instead of %v to preserve the original error in the chain.
- return klusterlet, reconcileStop, fmt.Errorf("failed to check for active klusterlet agent namespaces: %v", err) + return klusterlet, reconcileStop, fmt.Errorf("failed to check for active klusterlet agent namespaces: %w", err)pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go (1)
1625-1771: Add a case for “other namespace is terminating” and prefer early-fail assertions.
- Include a test where the “other” klusterlet namespace has DeletionTimestamp set; expected: helper returns false.
- Consider testify/require for shorter, fail-fast assertions.
I can push a test case snippet if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go(1 hunks)pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go (1)
pkg/operator/helpers/helpers.go (2)
KlusterletNamespace(603-610)DefaultAddonNamespace(55-55)
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go (2)
pkg/operator/helpers/helpers.go (1)
DefaultAddonNamespace(55-55)pkg/common/testing/fake_sync_context.go (1)
NewFakeSyncContext(21-27)
⏰ 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). (6)
- GitHub Check: unit
- GitHub Check: build
- GitHub Check: e2e-hosted
- GitHub Check: integration
- GitHub Check: e2e-singleton
- GitHub Check: e2e
🔇 Additional comments (1)
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go (1)
1773-1891: LGTM: cleanup behavior covered for both branches.Good verification that the addon namespace is preserved only when other agents exist and that the agent namespace is always deleted.
...erator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1153 +/- ##
==========================================
+ Coverage 57.94% 58.23% +0.28%
==========================================
Files 211 211
Lines 20765 20809 +44
==========================================
+ Hits 12033 12118 +85
+ Misses 7670 7624 -46
- Partials 1062 1067 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
elgnay
force-pushed
the
fix-addon-ns-deletion
branch
from
f55d55d to
0ae6d1f
Compare
|
/hold |
|
|
||
| // Count namespaces other than the one being deleted | ||
| activeCount := 0 | ||
| for _, ns := range namespaces.Items { |
There was a problem hiding this comment.
seems for loop is not needed? should we check
if (namespaces[0].Name != currentAgentNamespace && len(namespaces.Items) == 1) || len(namespaces.Items) >1
Signed-off-by: Yang Le <[email protected]>
elgnay
force-pushed
the
fix-addon-ns-deletion
branch
from
0ae6d1f to
7383e26
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go (1)
242-261: Ignore terminating namespaces, locally verify labels (fake client), early-return, and wrap errors.Current length-based check can treat terminating namespaces as “active,” and relying solely on server-side selectors can miscount under fake clients. Iterate, skip terminating/self, defensively check the label, and return early; also wrap errors.
func (r *managedReconcile) hasActiveKlusterletAgentNamespaces(ctx context.Context, currentAgentNamespace string) (bool, error) { - // Look for namespaces with klusterlet labels - namespaces, err := r.managedClusterClients.kubeClient.CoreV1().Namespaces().List(ctx, metav1.ListOptions{ - LabelSelector: klusterletNamespaceLabelKey, - }) - if err != nil { - return false, fmt.Errorf("failed to list klusterlet agent namespaces: %v", err) - } - - // check if there exist namespaces other than the one being deleted - if len(namespaces.Items) > 1 || len(namespaces.Items) == 1 && namespaces.Items[0].Name != currentAgentNamespace { - return true, nil - } - - return false, nil + // Prefer server-side label filtering for real clusters; still locally verify for fake clients. + namespaces, err := r.managedClusterClients.kubeClient.CoreV1().Namespaces().List(ctx, metav1.ListOptions{ + LabelSelector: klusterletNamespaceLabelKey, // exists(key) selector + }) + if err != nil { + return false, fmt.Errorf("failed to list klusterlet agent namespaces: %w", err) + } + for i := range namespaces.Items { + ns := &namespaces.Items[i] + if ns.Name == currentAgentNamespace { + continue + } + if ns.DeletionTimestamp != nil { + continue // ignore terminating + } + // Defensive: fake clients may not enforce label selectors; ensure the label exists. + if ns.Labels == nil { + continue + } + if _, ok := ns.Labels[klusterletNamespaceLabelKey]; !ok { + continue + } + return true, nil + } + return false, nil }Verification (optional):
#!/bin/bash # Ensure tests cover terminating namespaces and label presence cases. rg -nP -C3 --type=go 'hasActiveKlusterletAgentNamespaces\(' rg -nP -C2 --type=go 'DeletionTimestamp' rg -nP -C2 --type=go 'Labels\[klusterletNamespaceLabelKey\]'
🧹 Nitpick comments (1)
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go (1)
188-197: Wrap the propagated error with %w.Flow looks good. Use %w to preserve the original error for callers/tools.
- return klusterlet, reconcileStop, fmt.Errorf("failed to check for active klusterlet agent namespaces: %v", err) + return klusterlet, reconcileStop, fmt.Errorf("failed to check for active klusterlet agent namespaces: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go(1 hunks)pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go (1)
pkg/operator/helpers/helpers.go (2)
KlusterletNamespace(603-610)DefaultAddonNamespace(55-55)
⏰ 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). (7)
- GitHub Check: e2e-singleton
- GitHub Check: e2e-hosted
- GitHub Check: e2e
- GitHub Check: integration
- GitHub Check: verify
- GitHub Check: unit
- GitHub Check: build
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elgnay, qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@elgnay are we ok to merge this? |
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
Bug Fixes
Tests