Recover Full Site Before Reconfiguring Rotuer#2519
Conversation
Adds a new recovery mode to supress partial router ConfigMap writes with missing bridge configuration. Adds additional Controler init processes for recovering all resources that can affect site bindings prior to finalization, including selector connector pods and network status for exposeByPods listeners. Signed-off-by: Christian Kruse <christian@c-kruse.com>
📝 WalkthroughWalkthroughAdds pod watcher sync support, recovery-mode tracking, conflict-aware status retries, and controller startup recovery changes so recovered sites finish with stable router config. ChangesRecovery flow and router config stability
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/kube/site/extended_bindings.go (1)
397-413: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEarly-return skips syncing remaining independent watchers.
Both methods stop at the first failing selector/connector, so all subsequent (independent) pod watchers are never given a chance to sync. Since the caller only logs and continues on failure (best-effort during recovery), continuing the loop and aggregating the overall result would maximize how much state is fresh before
updateRecoveredRouterConfigbuilds the final config.♻️ Proposed fix: continue syncing all watchers, aggregate result
func (b *ExtendedBindings) SyncConnectorPods(stopCh <-chan struct{}) bool { + ok := true for _, selector := range b.selectors { - if selector != nil && !selector.Sync(stopCh) { - return false - } + if selector != nil && !selector.Sync(stopCh) { + ok = false + } } - return true + return ok } func (b *ExtendedBindings) SyncAttachedConnectorPods(stopCh <-chan struct{}) bool { + ok := true for _, connector := range b.connectors { - if connector.watcher != nil && !connector.watcher.Sync(stopCh) { - return false - } + if connector.watcher != nil && !connector.watcher.Sync(stopCh) { + ok = false + } } - return true + return ok }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 493f76bd-0b66-45e3-a727-a0fdcd1e8bbc
📒 Files selected for processing (6)
internal/kube/controller/controller.gointernal/kube/controller/controller_test.gointernal/kube/site/bindings.gointernal/kube/site/bindings_test.gointernal/kube/site/extended_bindings.gointernal/kube/site/site.go
| for _, connector := range c.attachedConnectorWatcher.List() { | ||
| if !c.namespaces.isControlled(connector.Namespace) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Filter attached connector recovery by target site namespace.
checkAttachedConnector applies the object to connector.Spec.SiteNamespace, so this recovery filter can skip externally-namespaced attached connectors that target a controlled site. Use the target site namespace for the control check.
🐛 Proposed fix
for _, connector := range c.attachedConnectorWatcher.List() {
- if !c.namespaces.isControlled(connector.Namespace) {
+ if !c.namespaces.isControlled(connector.Spec.SiteNamespace) {
continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, connector := range c.attachedConnectorWatcher.List() { | |
| if !c.namespaces.isControlled(connector.Namespace) { | |
| continue | |
| } | |
| for _, connector := range c.attachedConnectorWatcher.List() { | |
| if !c.namespaces.isControlled(connector.Spec.SiteNamespace) { | |
| continue | |
| } |
| func (s *Site) FinishRecovery(siteDef *skupperv2alpha1.Site) error { | ||
| if !s.inRecovery { | ||
| return s.Reconcile(siteDef) | ||
| } | ||
|
|
||
| err := s.Reconcile(siteDef) | ||
| s.inRecovery = false | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return s.updateRecoveredRouterConfig() | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clearing inRecovery before confirming the deferred update succeeded.
s.inRecovery = false runs immediately after s.Reconcile(siteDef), before its error is checked. If Reconcile fails, updateRecoveredRouterConfig() is never invoked for this attempt, yet inRecovery is already cleared — so a subsequent FinishRecovery retry (if any) will take the !s.inRecovery fast path and call plain Reconcile instead of re-running the aggregated recovery update. This happens to still converge correctly today because a fully-initialised site's normal update path pushes full state (not incremental listener-by-listener state), but it's fragile: any future change to that normal path (or an interleaved event calling updateRouterConfigForGroup on a single component in between) could reintroduce the very partial-config churn this PR is fixing.
Consider keeping inRecovery set until updateRecoveredRouterConfig() actually succeeds:
🐛 Proposed fix: keep inRecovery set until the recovered update succeeds
err := s.Reconcile(siteDef)
- s.inRecovery = false
if err != nil {
+ s.inRecovery = false
return err
}
- return s.updateRecoveredRouterConfig()
+ if err := s.updateRecoveredRouterConfig(); err != nil {
+ return err
+ }
+ s.inRecovery = false
+ return nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *Site) FinishRecovery(siteDef *skupperv2alpha1.Site) error { | |
| if !s.inRecovery { | |
| return s.Reconcile(siteDef) | |
| } | |
| err := s.Reconcile(siteDef) | |
| s.inRecovery = false | |
| if err != nil { | |
| return err | |
| } | |
| return s.updateRecoveredRouterConfig() | |
| } | |
| func (s *Site) FinishRecovery(siteDef *skupperv2alpha1.Site) error { | |
| if !s.inRecovery { | |
| return s.Reconcile(siteDef) | |
| } | |
| err := s.Reconcile(siteDef) | |
| if err != nil { | |
| s.inRecovery = false | |
| return err | |
| } | |
| if err := s.updateRecoveredRouterConfig(); err != nil { | |
| return err | |
| } | |
| s.inRecovery = false | |
| return nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/kube/site/site.go (1)
1535-1542: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid mutating
s.sitebefore persisting network status.updateSiteNetworkStatusupdates the in-memory object first; ifUpdateSiteStatusor the conflict retry fails, the nextNetworkStatusUpdatedcall can hit thereflect.DeepEqualfast path and skip retrying the same network update.Suggested change
func (s *Site) updateSiteNetworkStatus(network []skupperv2alpha1.SiteRecord) (*skupperv2alpha1.Site, error) { - s.site.Status.Network = network - s.site.Status.SitesInNetwork = len(network) - updated, err := s.UpdateSiteStatus(s.site) + desired := s.site.DeepCopy() + desired.Status.Network = network + desired.Status.SitesInNetwork = len(network) + updated, err := s.UpdateSiteStatus(desired) if !errors.IsConflict(err) { return updated, err }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 03368e85-0b2a-4a32-a189-275c72090bb5
📒 Files selected for processing (2)
internal/kube/site/binding_status.gointernal/kube/site/site.go
c-kruse
force-pushed
the
fix-site-recovery-stability
branch
from
86a4949 to
c9223bb
Compare
1 participant
Adds a new recovery mode to supress partial router ConfigMap writes with missing bridge configuration. Adds additional Controler init processes for recovering all resources that can affect site bindings prior to finalization, including selector connector pods and network status for exposeByPods listeners.
Closes #2504
Summary by CodeRabbit
New Features
Bug Fixes