Minor fixes/improvements to the spoke submariner agent controllers#2649
Minor fixes/improvements to the spoke submariner agent controllers#2649tpantelis wants to merge 5 commits intostolostron:mainfrom
Conversation
Update the error message when a deployment has no available replicas to include the namespace and actual deployment name instead of a transformed version. Also fixes grammar: "replica" -> "replicas" Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Replace the map[string]*SubmarinerConfig with a simple pointer since this controller only manages a single SubmarinerConfig instance (the spoke cluster's own config on the hub). The map was unnecessary overhead as it would only ever contain the single instance. This simplifies the code and makes the singleton nature explicit. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRefactored submariner agent internals: consolidated config cache to a single pointer, adjusted deployment degraded messaging and error handling, preallocated several slices, and threaded context.Context through numerous Ginkgo test helpers and test setups. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/spoke/submarineragent/config_controller.go (1)
130-139: Event filter is now less restrictive.The filter changed from matching a specific object name to matching any object in the cluster namespace. Since the informers are already namespace-scoped via
WithNamespace(o.ClusterName), this filter is effectively a no-op—all events in the namespace will pass through.The sync function still fetches objects by their specific names (
constants.SubmarinerAddOnName,constants.SubmarinerConfigName), so correctness is maintained. However, this may cause extra reconcile loops for unrelated objects in the namespace. If the namespace is expected to contain only these specific resources, this is a minor concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/spoke/submarineragent/config_controller.go` around lines 130 - 139, The event filters passed to WithFilteredEventsInformers are too broad (they only check namespace equality) and cause reconciles for any object in the namespace; narrow them to only allow events for the specific resources the sync expects by checking object name as well (e.g., in the AddOn informer filter verify metaObj.GetNamespace()==c.clusterName && metaObj.GetName()==constants.SubmarinerAddOnName, and in the Config informer filter verify metaObj.GetNamespace()==c.clusterName && metaObj.GetName()==constants.SubmarinerConfigName) so that the filters on input.AddOnInformer.Informer() and input.ConfigInformer.Informer() only pass events for the targeted resources.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/spoke/submarineragent/config_controller.go`:
- Around line 130-139: The event filters passed to WithFilteredEventsInformers
are too broad (they only check namespace equality) and cause reconciles for any
object in the namespace; narrow them to only allow events for the specific
resources the sync expects by checking object name as well (e.g., in the AddOn
informer filter verify metaObj.GetNamespace()==c.clusterName &&
metaObj.GetName()==constants.SubmarinerAddOnName, and in the Config informer
filter verify metaObj.GetNamespace()==c.clusterName &&
metaObj.GetName()==constants.SubmarinerConfigName) so that the filters on
input.AddOnInformer.Informer() and input.ConfigInformer.Informer() only pass
events for the targeted resources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f0d8372-ddd4-404b-8573-dc4cae8aaa03
📒 Files selected for processing (7)
pkg/spoke/submarineragent/config_controller.gopkg/spoke/submarineragent/config_controller_test.gopkg/spoke/submarineragent/connections_controller_test.gopkg/spoke/submarineragent/deployment_controller.gopkg/spoke/submarineragent/deployment_controller_test.gopkg/spoke/submarineragent/gateways_controller_test.gopkg/spoke/submarineragent/submarineragent_suite_test.go
In error handling switch statements with the pattern: case apierrors.IsNotFound(err): case err == nil: case err != nil: The third case is redundant since if we reach it, err must be non-nil (the previous cases already handled NotFound and nil). Using 'default' is more idiomatic and clearer in intent. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Pre-allocate slice capacity where the final size is known or can be reasonably estimated. This reduces memory allocations and copies during append operations. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Update all test files to properly propagate the Ginkgo-injected context through helper function chains, eliminating inappropriate uses of context.TODO(). Note that context.TODO() is still used for long-running controller contexts rather than the request-scoped Ginkgo-injected context. Added nolint:contextcheck directives to ignore these. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
|
/override ci/prow/sonarcloud |
|
@tpantelis: Overrode contexts on behalf of tpantelis: ci/prow/sonarcloud DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aswinsuryan The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
2 participants
See commits for details.
Summary by CodeRabbit
Bug Fixes
Performance
Tests