feat(ha): detect a dead Active hub and promote the Standby (#297 PR2) - #4
Open
sumanthd032 wants to merge 12 commits into
Open
feat(ha): detect a dead Active hub and promote the Standby (#297 PR2)#4sumanthd032 wants to merge 12 commits into
sumanthd032 wants to merge 12 commits into
Conversation
The signal a worker uses to find the Active hub after a failover, per ADR kubeslice#293 Decision 7. Each hub writes this field about itself, on its own API server, and only while it holds leadership. A Standby's copy is populated by the state mirror from the Active, so it names the Active rather than itself. That is what lets a worker watching both hub endpoints resolve which one is Active by the rule "trust whichever endpoint is reachable and reports an ActiveIdentity matching that endpoint's own identity", without needing to know which role either hub currently holds, and without inferring a death from a timeout. LastUpdated is not in the ADR's YAML sketch. It is added deliberately: Decision 7's open tie-break question needs a freshness signal if a partition causes both hubs to self-declare at once, and comparing a timestamp already on the object is cheaper than making the worker read coordination.k8s.io Leases across clusters. StorageCapabilities.LastUpdated in this same struct is existing precedent for the pattern. The field is additive and omitempty throughout, so a non-HA deployment never populates it and an existing worker sees no behaviour change. The same types are being added to github.com/kubeslice/apis, which is what worker-operator imports; this repo carries its own copy of them. Note on the CRD manifest: only the activeController schema is included. make manifests also rewrites the controller-gen version annotation in all ten CRD files, because the committed manifests were generated with v0.19.0 while the Makefile pins v0.17.3. That pre-existing drift is left alone rather than folded into this change. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…ship ADR kubeslice#293 Decision 7 requires each hub to declare itself on its own API server while it holds leadership, so a worker watching both hub endpoints can tell which one is Active without knowing either hub's role. Publishing only at promotion would leave a worker unable to identify the Active before the first-ever failover, so this is a continuous loop rather than a step in the promotion sequence. It is also standalone rather than part of ClusterService.ReconcileCluster, because it has to converge independently of reconciler traffic — and reconciler traffic is exactly what is absent right after a promotion, when the write fence has just opened but nothing has re-enqueued the pre-existing objects yet. PublishOnce is exported so promotion can run one synchronous pass and not tie failover latency to the tick. Details worth calling out: - The convergence check deliberately excludes LastUpdated. Including it would make every pass differ from itself and turn a convergence check into a write to every Cluster CR on every tick. - The publisher refuses to write an empty endpoint or the shipped placeholder (https://controller.cisco.com:6443/), because advertising an unreachable address as the failover target is worse than advertising nothing. Refusing is not an error: a hub that cannot describe itself should keep reconciling. - The placeholder literal is duplicated in pkg/ha rather than imported, because main.go overwrites service.ControllerEndpoint with the flag value at startup and the default is unrecoverable afterwards. TestPlaceholderMatchesServiceDefault fails if the two ever drift. - An unreadable CA bundle is logged and publication continues without it. The endpoint and identity are what select a hub, and a worker that already pins the hub's CA does not need it republished. - Nothing ever clears the field. A hub stops publishing only by losing leadership, which means it stopped renewing its Lease and is unreachable, so a worker cannot read the stale declaration anyway. Auto-demotion of a recovered hub is an explicit ADR non-goal (Decision 8), and LastUpdated is what lets a consumer prefer the fresher of two claims if it ever does see both. The elector is taken as a narrow two-method interface so the publisher is testable without a live Lease. 12 tests, covering the not-leader no-op, the converged-pass-writes-nothing property, both endpoint refusals, CA bundle encoding and absence, partial failure across clusters, and graceful shutdown. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Adds --ha-self-ca-bundle-path (default the in-pod service account CA path) and starts the publisher alongside the existing HA loops. Two wiring decisions worth stating: It is deliberately not started in standalone mode. Standalone is always the leader, so the publisher would run and start writing status.activeController on every existing non-HA deployment. Leaving the field absent there is what keeps an existing worker's behaviour unchanged, which is the no-regression guarantee HA is built on. A Standby does start it. The publisher no-ops while the hub is not the leader, so it costs one list per interval and needs no extra wiring when promotion flips leadership in a later change. It writes through localHAClient — the same direct, uncached client the elector uses — rather than the manager's cached client, so it does not depend on the manager cache having started. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Found in live testing against a Kind hub: a freshly started Active took 31 seconds to advertise itself, not the ~2 seconds intended. Start ran its first pass immediately, but an Active does not hold its Lease yet at that instant — acquisition lands a second or two later. So the first pass saw IsLeader() false, skipped, and the next attempt was a full publish interval away. Any worker booting inside that window could not identify the hub. Unit tests could not catch this: the test double is the leader from the first call, so the race does not exist there. The fix is driven by the loop now waiting on the short leadership interval whenever a pass found this hub was not the leader, and on the publish interval only once it is. A non-leader returns before touching the API server, so polling at 2s costs nothing while idle — and it means a Standby also picks up leadership promptly at promotion, independently of promotion remembering to call PublishOnce. The regression test then caught a second, narrower version of the same bug in the first fix: choosing the wait from its own IsLeader() call meant leadership arriving between the publish check and the wait check still cost a full interval. publishOnce now reports whether it held leadership, and the wait is chosen from what the pass actually did rather than from a second read. Verified live after the fix: published in 2s. resourceVersion held steady across a full publish interval, so the convergence check still writes nothing once converged. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Signed-off-by: Sumanth D <sumanthd032@gmail.com> # Conflicts: # main.go
…ed pod checkRemoteLeaseOnce returned (false, err) when the read failed — reporting "not stale". So only one failure mode was ever detectable: the controller pod dying while its API server stayed up. An Active that lost its API server, its node, or the whole cluster was invisible, forever. That is the disaster this feature exists for, and it is the one case every demo so far could not have caught, because they all killed the process. The fix follows from noticing that the two cases are the same event. When the pod dies, reads succeed and renewTime is frozen at T. When the API server dies, reads fail, so the newest renewTime this hub has ever seen is frozen at T. In both, the newest proof of life stops advancing; the difference is at the transport layer, not in the meaning. So the elector now retains the last successfully-read Lease. A successful read replaces it; a failed read leaves it alone and logs. The verdict is then a single isLeaseStale call against that retained view, which ages on its own against a moving clock — covering both modes with one threshold, no second timer, and the already-tested staleness helper doing the work. Detection lands at roughly leaseDuration + padding + one poll in both. checkRemoteLeaseOnce still never changes leadership. It reports candidacy; the guards and the promotion sequence are separate commits, so "we think the Active is gone" and "we took over" stay independently testable, and kubeslice#294's tests asserting a Standby does not promote continue to hold.⚠️ The nil check for the retained Lease is deliberately a separate statement and must never be folded into the isLeaseStale call. isLeaseStale(nil, ...) returns TRUE — correct for its original caller, where a Lease absent from your own cluster should be created — but here it would mean a Standby that has never once read the Active's Lease promotes itself on its first tick. A broken kubeconfig, a missing RBAC grant or a mistyped namespace would each become a guaranteed split brain. TestNeverArmed_NeverBecomesCandidate fails if anyone merges the two conditions. That nil case doubles as the arming rule: never promote without having proved, at least once, that the Active's Lease is reachable. It separates "it worked, then it stopped" from "it never worked". The cost is real and accepted: a Standby restarting during an outage can never arm and so will not promote. That is the safer failure — a missed promotion is visible downtime an operator can resolve, a false one is silent dual writes — and it is now documented in the ADR beside the split-brain non-goal. lastGoodRead is recorded but not used by the verdict, which anchors on the Lease's own renewTime. It is carried so an optional local-only staleness floor stays available without a redesign if clock skew between hubs ever becomes a practical problem. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Staleness says the Active's newest proof of life has aged out. It does not say that is why. These guards ask whether the observation is actually evidence about the Active, and record every refusal. They exist because the costs are not symmetric. A missed promotion is downtime — visible, and an operator can force a takeover. A false promotion is two hubs writing to their own copies of the world simultaneously, with the mirror still running in one direction: objects overwriting each other, prune's reverse diff resurrecting deletes, workers taking contradictory instructions, and recovery by hand. Two bounded reads to avoid the second is a good trade inside a budget that already tolerates leaseDuration + padding. Self-health. A dead Active API server, a partition between the hubs, and this hub losing its own networking produce byte-identical observations: reads of the remote Lease simply stop succeeding. Asking whether the local API server still answers is the only cheap way to separate the last case, and it is the most likely cause of a false promotion after outright misconfiguration. It reads this hub's own Lease through the existing local client, so it needs no new client and no new RBAC — the leader-election Role already grants leases in the controller's own namespace.⚠️ NotFound counts as HEALTHY here, and getting it backwards would block every real first failover while looking entirely reasonable in review. On a first-ever promotion no Lease exists on this hub yet, and NotFound means the API server answered — precisely what is being tested. Only transport errors, timeouts and server errors indicate an unhealthy self. TestSelfHealthy_NotFoundCountsAsHealthy pins it. Final dial. One fresh, bounded read at decision time. Worth stating plainly what it does and does not buy: against an Active that is reachable but has stopped renewing, it closes a real polling race, because the Active may have renewed moments after the last poll. Against an unreachable Active it buys almost nothing — it is the next failed read after a sustained run of them. And it is NOT a split-brain guard: in a genuine partition it travels the same broken path as every other read, fails identically, and the Standby promotes anyway. Safety on that path comes from duration and from the arming rule, not from this read. The ADR previously implied otherwise and has been corrected. Both reads are bounded by --ha-promotion-dial-timeout (5s). This is not optional: main.go builds the remote client with a plain uncached client.New and no timeout, so a dial to a black-holed API server blocks until the OS TCP timeout — minutes, far outside the failover budget. Aborting changes nothing but the attempt. The cached Lease is not cleared and the elector is not disarmed, so the next tick re-evaluates from scratch and a genuinely recovered Active heals the state on its next successful read. Clearing it would be worse than useless: an already-gone Active can never re-arm the elector, so one transient guard failure would leave a hub that never fails over again. TestGuardsAbort_DoesNotDisarm covers it. Metrics: ha_failover_total, and ha_promotions_aborted_total{reason} with reasons self_unhealthy, lease_live and already_promoting. Without the second, every guard is invisible in production — a hub that correctly declines to take over looks identical to one that never noticed anything — and these are the branches most worth demonstrating. --ha-promotion-grace-period is defined here too, used by the next commit. It is a sequencing budget for publishing status.activeController before the write fence opens, and is deliberately distinct from --ha-padding-seconds, which is a detection threshold. Issue kubeslice#297's --ha-promotion-grace is an alias of the latter and is not implemented. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
The sequence a Standby runs once detection says the Active's newest proof of life has aged out. Two orderings in it are load-bearing, and both issue kubeslice#297 and an earlier draft of ADR Decision 5 had them wrong. The mirror is stopped, and confirmed stopped, before the write fence opens. The most common trigger for a failover is the Active's controller pod dying while its API server stays perfectly healthy — in which case the mirror's informers are still live and still mirroring at the moment of promotion. Every mirrored object carries the syncer's own label and the mirror's conflict guard only skips objects WITHOUT it, so a fence opened first means the mirror overwrites exactly the objects the new Active's reconcilers are writing, while prune's reverse diff resurrects anything they delete. You would promote into a hub fighting itself. A mirror that fails to stop aborts the promotion rather than pressing on, because half-stopped is the very state this step exists to prevent. The reconcile kick runs after the fence opens, not before. The fence drops requests rather than requeuing them, so a kick delivered while it was still shut would be dropped without requeue — the exact failure the kick exists to fix. Steps 0 and 8 bracket the whole sequence with a promoting latch, and IsLeader() reports false while it is held regardless of the leadership flag. So the fence stays shut from the first step to the last, which gives "the reconcilers are not live until promotion completes" real teeth without inventing any external status surface. Effects outside the elector are injected as PromotionHooks rather than imported, so pkg/ha stays independent of the mirror, the publisher and the manager, and so the entire sequence is testable without any of them. Nil hooks are skipped: an elector with none still takes leadership correctly, it just does it without the parts that make promotion safe and complete. Failure handling follows what each step actually costs. A guard refusal is a correct outcome, not an error, and leaves the hub a fenced, still-armed Standby free to retry next tick. Publication is bounded by --ha-promotion-grace-period and failing it still promotes, because a hub that cannot describe itself is a better Active than no Active at all and the publisher's own loop keeps retrying. Failing the kick likewise still promotes; it costs pre-existing state staying unreconciled, not the takeover. mode becomes an atomic.Value. It is written by the promotion goroutine and read by Mode(), StartLeaseRenewal and WatchRemoteLease from theirs, so a plain field is a data race. -race found a real bug here during development, and it is worth recording because the shape recurs. Promotion was documented as once-only, but the latch is released on success, so nothing actually stopped a second run — and a second run is not merely redundant, it races the renewal loop the first one started, which owns lastRenew and is actively writing the Lease. An already-Active hub is already in the state promotion produces, so it now short-circuits, with a re-check under the latch for two callers that both passed the first check. TestPromote_IsOnceOnly asserts no hook runs twice. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Records a failover as a Kubernetes Event, so an operator finds out from `kubectl get events` and not only from a log line. The Lease is the involved object. It is the leadership record, there is exactly one, and it lives in the controller's own namespace — and the recorder derives an Event's namespace from the object it is attached to, so passing the Lease is what puts the Event beside the controller that emitted it.⚠️ Deliberately not kubeslice-system. Issue kubeslice#297 asks for the Event there, but that namespace does not exist on a hub: per ADR kubeslice#293 Decision 1 it is a worker-cluster namespace, and the hub's is kubeslice-controller / $KUBESLICE_CONTROLLER_MANAGER_NAMESPACE. What makes this worth a comment rather than a silent correction is that a constant with exactly the wrong meaning sits in the vendor tree — kubeslice-monitoring's logger.ControlPlaneNamespace — waiting to be reached for by anyone implementing the issue literally.⚠️ recorder.RecordEvent is called directly, never util.RecordEvent. That helper begins with util.CtxLogger(ctx), which nil-panics on any context that has not been through PrepareKubeSliceControllersRequestContext — and promotion runs on main.go's signal-handler context, which has not. The same mistake crashed a live Standby during kubeslice#295, so it is called out here rather than left to be rediscovered. The event name is registered in config/events/controller.yaml and generated into both events_generated.go and config/events/events_config_map.yaml. RecordEvent hard-fails on an unregistered name, so a hand-written entry without a `make generate-events` run would fail during a real failover; a test asserts the generated entry exists and that the failure mode is loud rather than silent. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Adds --ha-promotion-dial-timeout and --ha-promotion-grace-period, and installs the promotion hooks on a Standby. The mirror gets its own cancellable context, derived from the manager's, so promotion can stop it without tearing down everything else that hangs off the signal-handler context. StopMirror cancels it and then waits for Start to return, which is the part that matters: RemoteSyncer.Start already drains its workqueue and prune goroutine before returning, so returning from it is a sufficient and already-correct barrier and there is nothing to reimplement. Waiting is what stops a hub opening its write fence while the mirror is still writing — and in the most common failover trigger, the Active's pod dying while its API server stays up, the mirror is very much still alive at that moment. The publisher moves above the mode switch so it exists before the hooks that reference it. Its PublishOnce becomes promotion step 7, which is what keeps failover latency off the publisher's own tick. The hooks are installed only in standby mode. An Active has nothing to promote from, and standalone must stay untouched: it is always the leader, so the whole HA path has to remain inert there. The event recorder is passed by value, not address — EventRecorder is an interface, and taking its address yields a pointer-to-interface that does not satisfy it. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Found by live-testing an Active whose API server was shut down — the case this feature exists for and the one no test had ever exercised. The guards' reads were bounded; the periodic poll was not. The watch loop calls checkRemoteLeaseOnce synchronously, so a read that blocks blocks the loop, and while it is blocked no staleness is evaluated at all. main.go builds the remote client with a plain uncached client.New and no timeout, so an API server that accepts the connection and then stops answering leaves the read hanging until the transport gives up. Measured: a single read blocked for about twelve seconds, with no poll and no staleness evaluation in the whole window, before the connection finally broke. That was a graceful container shutdown, which at least tears the connection down eventually. A powered-off node or a partition that drops packets has nothing to break it, and the wait becomes the OS TCP timeout — minutes. The failover budget would be blown by waiting rather than by deciding, which is the failure mode the bounded guards were added to prevent in the first place; the poll simply got missed. After the fix the same shutdown produces a regular two-second cadence with one four-second gap where the read times out, and promotion lands at 21.1s of staleness against a 20s budget. A timed-out read is treated as exactly what it is — a failed read — so the retained view is kept and continues to age. The verdict logic is unchanged. Also adds tests for three promotion branches that were uncovered: the concurrency latch under genuinely concurrent callers, failure to acquire the Lease (the one step whose failure means the hub cannot lead, so it must abort rather than open the fence), and failure to emit the event (a report, not a step — a hub that took over but could not say so is still the Active). promote() goes from 84% to 94% statement coverage. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
An audit of the failure paths, rather than the happy one, turned up a hang in the step most carefully reasoned about — and the reasoning is what caused it. Waiting indefinitely for the mirror to stop looked like the safe choice, because proceeding without it is precisely the dual-writer state that step exists to prevent. It is not safe. The watch loop calls promote synchronously, so an unbounded wait on a mirror that never exits blocks the loop: no further polls, no further staleness evaluation, no failover ever, and nothing logged after "promotion sequence starting". Choosing "never promote into a dual writer" that way silently buys "never promote at all", which is strictly worse and completely invisible. Reproduced with a hook that never returns; promote blocked forever. Bounded by --ha-promotion-grace-period, expiry aborts the attempt loudly and the next tick retries — so a merely slow mirror costs one tick, and a genuinely stuck one is visible rather than mute. The hub is left exactly as it was: fenced, still armed, free to try again. The same bound now applies to the two steps that run after the fence opens. A hang there cannot cost the failover, since leadership is already taken, but it can cost the promotion ever finishing or reporting itself. The kick is the one that will matter: it pushes into a channel per reconciled type, and those are only drained once the manager is running, while main.go starts this watch loop before mgr.Start. A kick arriving in that window has nothing reading the other end. Also adds an explicit precondition for a missing remote client. Without a client to the Active there is no way to have observed it alive, so nothing could justify concluding it is gone. It is unreachable from the watch loop, which refuses to start without one and could not arm without one either, but promote is a method and a caller reaching it another way crashed inside the final dial instead of being told no. Both flag descriptions were left inaccurate by these changes and by the previous commit — the dial timeout now bounds every networked Lease read including the periodic poll, and the grace period now bounds four sequencing steps rather than one. Corrected. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Detection and promotion for cross-cluster HA. A Standby now notices the Active is gone and takes over.
Detection previously reported "not stale" whenever the read failed, so only a dead controller pod was detectable — a dead API server, node or cluster was not. The Standby now retains the newest Lease it read successfully and ages that, which covers both cases with one threshold.
Before promoting it must have read the Active's Lease at least once, reach its own API server, and fail one final bounded read of the Active's. The mirror is stopped and confirmed stopped before the write fence opens; opening it first lets the mirror overwrite what the new Active writes.
Adds
--ha-promotion-dial-timeoutand--ha-promotion-grace-period, thePromotedToActiveevent,ha_failover_totalandha_promotions_aborted_total.Diverges from the issue in two places, both per ADR kubeslice#293 Decision 5: the event goes in the controller's own namespace rather than
kubeslice-system, which does not exist on a hub, and the mirror is stopped beforemode = Activerather than after.Depends on kubeslice#297 PR1 (#3), merged into this branch, so its commits appear in this diff.
Part of kubeslice#297
How Has This Been Tested?
go test -race -count=2 -shuffle=on ./pkg/ha/...clean. Package coverage 85.8%.status.activeControllerpublished, event recorded inkubeslice-controller.go vet ./service/...fails on a pre-existingundefined: util.Clientin a test file on the base branch, unrelated to this change.Checklist:
Does this PR introduce a breaking change for other components like kubeslice-controller, worker-operator?
No. Everything is gated on
--ha-mode, which defaults tostandalone.