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 6 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
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.
- Clarify the Lease/Secret namespace is read from the downward API (KUBESLICE_CONTROLLER_MANAGER_NAMESPACE), not a hardcoded literal; confirm kubeslice-controller (not kubeslice-system) is correct, and add the missing --ha-lease-namespace flag. - Resolve the finalizer/stuck-Terminating open issue: StateMirror strips finalizers on write, with periodic prune-on-resync as a backstop for missed deletes. - Enumerate every process main.go starts and mark each as always-runs/Active-only/Standby-only; add a security note that ENABLE_WEBHOOKS=false is a deployment convenience, not a security boundary. - Fix --ha-mode's documented default and align the Decision 3/4 code snippets with the merged kubeslice#294 implementation (LeaderElector field name, nil-check, and the actual tested "skipping reconcile" log line). - Add an open issue on the worker's failover-time detection of status.activeController across both hubs, for #467 to settle. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
|
@gourishkb All items addressed in d521a70 - namespace, finalizers, goroutine gating, and webhook security note fixed;
This one is added as Open Issue 4, still needs your input. |
gourishkb
left a comment
gourishkb
left a comment
There was a problem hiding this comment.
All blocking items from the previous round are addressed in d521a70:
- Finalizers / stuck-Terminating → resolved in Decision 6:
StateMirrorstrips all finalizers on write, with periodic prune-on-resync as a backstop for missed deletes. No longer an open issue. - Namespace →
--ha-lease-namespacenow defaults toKUBESLICE_CONTROLLER_MANAGER_NAMESPACE(downward API), withkubeslice-controlleronly as a last-resort fallback. Correct. - Goroutine gating → the flags table now enumerates what runs on Active-only vs always, and
ENABLE_WEBHOOKS=falseon Standby is documented with the security note. - Webhook security note → added.
Open Issue 4 (status.activeController dual-watch mechanism) is the right place for this — the analysis is thorough and the direction (two permanent watches, each hub self-declares, worker trusts whichever is reachable and self-declares Active) is sound. The two remaining sub-items (tie-break rule and credential prerequisite) must be resolved with the worker-operator maintainers before worker-operator #467 is opened — please do not start #467 until that is settled.
Approving. This ADR is ready to merge as a design gate for the downstream implementation issues.
…slice#297) Designing the promotion path against the implemented leader election and state mirror surfaced four places where this ADR was wrong or silent, and two where its wording invited an incorrect implementation. Amending it here so the design document and the code being built from it agree. Decision 5 — what triggers promotion. Rewritten. * Detection only covered "Lease readable but renewTime frozen". It said nothing about a Lease that cannot be read at all, so the loss of an entire hub - API server, node, or cluster - was undetectable. Both failures are the same event from the Standby's seat: the newest proof of life it holds stops advancing. Restated as one rule over the newest renewTime ever successfully read, which covers both with one threshold. * Added the two conditions that were missing entirely. A Standby must have read the Active's Lease at least once before it may promote, so a bad kubeconfig or a missing RBAC grant cannot masquerade as a dead Active; and it must confirm its own API server answers, so its own network failing is not mistaken for the Active's. Both are recorded with their costs, including that the first means a Standby restarting mid-outage will not promote. * The final dial was framed as the safety mechanism. It is not - in a partition it travels the same broken path and fails identically, so the Standby promotes anyway. Described as what it is: a guard against the Active having renewed between polls. * Step order corrected. Stopping the mirror came after leadership was granted. In the most common trigger the Active's API server is still healthy, so the mirror is still running: it would overwrite what the promoted reconcilers write, and the resync's reverse diff would re-create what they delete. The mirror must stop, and be confirmed stopped, before the write fence opens. * Added the re-enqueue step. The Decision 4 fence drops requests rather than requeuing them, so flipping it causes no reconcile at all and pre-existing mirrored state - which carries no finalizers until a reconciler re-adds them - would sit untouched until the informer resync period. Noted that a test which only creates new objects passes without this, since a new object generates its own event. * Specified the Event: the controller's own namespace, with the acquired Lease as the involved object. The namespace it belongs in was stated correctly in Decision 1 but never restated here, and kubeslice-system exists only on workers. Decision 6 — "each hub mints its own token" was written as a property the system has. It is a requirement on the registration path, which today creates the token Secret only when the ServiceAccount is absent. A mirrored ServiceAccount arrives without its Secret, so that branch never runs and a promoted hub fails every cluster reconcile while reporting success. Stated as the requirement it is. Decision 7 — the activeController field was described as something a promoted hub sets, which leaves a worker unable to identify the Active before any failover has happened. Open Issue 4 already requires each hub to self-declare while it holds leadership; made that a continuous local publisher rather than a promotion step, stated that the write never crosses into a worker cluster, and required a hub still carrying the placeholder endpoint to refuse to publish rather than advertise an unreachable failover target. Decision 8 — recorded why a recovered Active cannot be demoted automatically. The two Leases are separate objects on separate API servers, so there is no shared record of who won; a recovered hub reads its own Lease, sees itself as holder, and resumes. Flags — the two promotion durations were described in near-synonymous terms and are not the same setting: one decides when the Active is considered dead, the other how long promotion waits on its own publication step. Rewritten to distinguish them, and noted that issue kubeslice#297's --ha-promotion-grace is an alias of the shipped --ha-padding-seconds and is deliberately not implemented. Added the dial timeout and self CA bundle path. The worker's pre-failover credential for the Standby remains open and is deliberately not settled here; it spans this repo and worker-operator and needs maintainer agreement on where it lands. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Implementing the promotion sequence surfaced a hazard this decision did not name, in the step it reasoned about most carefully. Stopping the mirror before the write fence opens is the step where an unbounded wait looks cautious: proceeding without a stopped mirror is exactly the dual-writer state the ordering exists to prevent. But the Standby evaluates staleness in a single loop that runs the promotion sequence inline, so a step that blocks forever blocks that loop — no further polls, no further staleness evaluation, no failover ever, and nothing logged after the sequence began. "Never promote into a dual writer" silently becomes "never promote at all", which is worse and invisible. So every step that waits on another component is bounded. Expiry before the fence opens aborts the attempt and the next poll retries; expiry after it is logged against a hub that is already leading. Both flag descriptions are corrected to match what they actually bound. The dial timeout covers every networked Lease read, including each periodic poll — an unbounded poll stalls detection for as long as the read hangs, which live testing measured at twelve seconds against an API server that was shutting down and which has no upper limit against one that simply stops answering. The grace period covers four sequencing steps, not only the publication one. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Decision 7 requires a worker to already hold a credential for both hubs. Open Issue 4 said the mechanism for that was unsolved. The two have contradicted each other since the design was written, and the contradiction was left in deliberately while the mechanism was undecided. Decision 6 now carries it: the mirror copies the worker token Secret's shell, without the token value or the account UID annotation, and the Standby's own token controller populates it. A Standby holds a credential valid on itself continuously, before any failover, with no unfencing. Records the two properties that are easy to get wrong — the shell must be create-only or resyncs rotate the token underneath the worker, and the Standby now caches every Secret cluster-wide, which widens caching but not access. Open Issue 4 keeps only what is genuinely still open: delivering that token to the worker at registration, which lives in the charts and the documented registration procedure rather than in either repo, and consuming it, which is worker-operator #467. Also corrects the registration-path paragraph, which assumed a mirrored ServiceAccount arrives without its token Secret. It now normally arrives with the shell; the independent existence check is still required, for the cases the mirror cannot cover. Part of kubeslice#293 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
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.