docs(adr): add ADR for Active/Standby HA controller (#293)#405
docs(adr): add ADR for Active/Standby HA controller (#293)#405sumanthd032 wants to merge 2 commits into
Conversation
Add the design record for multi-cluster Active/Standby HA of the kubeslice-controller: one Active hub holds a Lease and does all writes while a Standby mirrors its state and watches the Lease, promoting itself on Active failure. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds an Architecture Decision Record (ADR) documenting the proposed multi-cluster Active/Standby HA design for the KubeSlice controller, capturing key decisions and expected operational behavior as a prerequisite for the downstream implementation work.
Changes:
- Introduces an ADR describing the Active/Standby hub architecture, goals/non-goals, and core components.
- Documents explicit design decisions for lease placement, credential bootstrap, fencing, promotion triggers, state mirroring, worker reconnection, and split-brain handling.
- Adds Mermaid-based architecture + sequence diagrams and a table enumerating mirrored resources.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@pnavali @gourishkb - review comments addressed and the ADR is updated. Please do a final review before merging. Thanks! |
Signed-off-by: Sumanth D <sumanthd032@gmail.com>
d2b4318 to
43dcc20
Compare
gourishkb
left a comment
There was a problem hiding this comment.
Good structure overall — the scope note correctly dismissing --leader-elect as orthogonal is exactly right, and the decision to mirror Secrets/RBAC/Namespaces (not just CRDs) is the kind of detail that prevents a promoted hub from being stuck waiting for credential propagation. A few things need resolution before this gates implementation.
Must clarify before #294/#295/#297 start
-
status.activeControllerambiguity. Decision 7 and the diagram describe this field as living on the Active hub'sClusterCR (ac_clusterin the flowchart), yet the worker is supposed to watch it to learn the new Active. But the worker can't reach the hub's API when the hub is down — that's the exact failure scenario. Please clarify: does the new Active write this signal to a CR on each worker cluster after promotion (so the worker watches a local CR), or does it live on the hub (and if so, how does the worker read it when the hub is unreachable)? This needs to be precise because worker-operator #467 depends on it. -
Namespace:
kubeslice-controllervskubeslice-system. Decision 2 places theactive-hub-kubeconfigSecret in namespacekubeslice-controller, but the controller deploys intokubeslice-system. Please verify and use the correct namespace consistently throughout the ADR. -
Open Issue #2 (finalizers / stuck Terminating objects) must be resolved before any code merges. This isn't something that can be addressed post-MVP. If
StateMirrorwrites objects with finalizers (e.g.,Cluster,SliceConfig) and those objects are then deleted on the Active, the mirror propagates the deletion to the Standby. But on the Standby, the finalizer controllers are gated (IsLeader()=false), so the object sits inTerminatingforever. This leaks state and corrupts the Standby's view of the world. The ADR should propose a resolution — stripping finalizers from mirrored copies, or skipping the deletion mirror and relying on a periodic prune-and-resync — before implementation begins.
Should fix
-
Background goroutine gating is underspecified. Decision 4 mentions gating reconcilers and says "background writers (e.g. the
VpnKeyRotationtimer) start only whenIsLeader()". Butmain.goalso starts other background processes: the metrics server, health-check server, leader-election renewer. Which processes run on both Active and Standby, and which are gated? The ADR should enumerate all goroutines started bymain.goand explicitly mark each as "always runs", "Active only", or "Standby only". This directly determines how--ha-modechangesmain.go's startup path. -
ENABLE_WEBHOOKS=falseon Standby — security note. Disabling webhooks on the Standby soStateMirrorcan write objects without triggering validation is pragmatic, but it means that if an operator mistakenly applies aSliceConfigdirectly to the Standby while it is in Standby role, that invalid config will be accepted. Please add a note in the ADR that the Standby's API server should ideally be unreachable from external clients (network policy or no exposed route), and thatENABLE_WEBHOOKS=falseis a deployment-level guard, not a security boundary.
Minor
- The recovery path ("a recovered failed hub re-joins as Standby by manual redeploy") is mentioned in one sentence. This deserves a paragraph in the Operational Model or Non-Goals section — specifically: what state does the recovered hub have, how does an operator wipe and redeploy it as Standby, and what happens if the operator skips the wipe step and redeploys without clearing the old Lease.
| end | ||
|
|
||
| sb_watcher -. "watch renewTime (remote client)" .-> ac_lease | ||
| sb_mirror -. "mirror CRDs + certs + RBAC + SAs + Secrets" .-> active |
There was a problem hiding this comment.
This kubeconfig Secret is placed in Retracted — kubeslice-controller namespace, but the controller runs in kubeslice-system.kubeslice-controller is the correct namespace for the hub controller; kubeslice-system is the worker-side namespace. The namespace in Decision 2 is correct as written.
| localClient client.Client // own cluster — renew the Lease | ||
| remoteClient client.Client // Standby only — watch the Active's Lease | ||
| mode HAMode // Active | Standby | Standalone | ||
| } |
There was a problem hiding this comment.
Decision 4 says every reconciler has an IsLeader() gate, but doesn't say the check is per-call (not cached at startup). Issue #294 explicitly requires this. Please add: "IsLeader() is evaluated on every invocation of Reconcile(), not once at startup, because leadership can change while the controller is running."
| |---|---| | ||
| | `Project`, `Cluster`, `SliceConfig`, `ServiceExportConfig`, `SliceQoSConfig`, `VpnKeyRotation` | `controller.kubeslice.io` | | ||
| | `WorkerSliceConfig`, `WorkerSliceGateway`, `WorkerServiceImport` | `worker.kubeslice.io` | | ||
| | Gateway VPN cert `Secret`s (OpenVPN/WireGuard), CA/issuer `Secret`s, worker kubeconfig `Secret`s | `core/v1` | |
There was a problem hiding this comment.
Open Issue #2 (finalizers on mirrored objects causing stuck Terminating) needs a proposed resolution before any code review for #295 will be approved. The simplest fix: StateMirror strips all finalizers from mirrored copies before writing them to the Standby. Since reconcilers are gated on the Standby, finalizer controllers will never run there anyway — so carrying finalizers on the mirror is only harmful. Please add this as a decision (or an explicit resolution of this open issue) before implementation starts.
| **Exception:** hub-issued SA **tokens** are not mirrored. A token signed by the Active is invalid on the Standby, so the mirror carries the `ServiceAccount` + RBAC (the identity and authorization) and each hub mints its own token (Decision 7). | ||
|
|
||
| ### 7. How do workers find the new Active after failover? | ||
|
|
There was a problem hiding this comment.
The status.activeController signal lives on the Active hub's Cluster CR in the diagram, but the worker needs to read this when the hub is down. Please clarify the actual delivery path: does the promoted Standby write a signal to a CR on each worker cluster (so the worker watches locally), or is there a different mechanism? This is the most critical implementation detail in #467 — worker-operator #467 cannot be designed until this is resolved.
Add the design record for multi-cluster Active/Standby HA of the kubeslice-controller: one Active hub holds a Lease and does all writes while a Standby mirrors its state and watches the Lease, promoting itself on Active failure.
Fixes #293
How Has This Been Tested?
N/A - documentation-only change (ADR). No code paths are modified, so there is nothing to run. Reviewed by rendering the Markdown, verifying the two sequence diagrams, and checking each decision against the required-answers table in issue #293.
Checklist:
Does this PR introduce a breaking change for other components like worker-operator?
No. This is a design document only and changes no runtime behavior.