feat(ha): promote a Standby to Active on Active-hub failure - #417
Open
sumanthd032 wants to merge 32 commits into
Open
feat(ha): promote a Standby to Active on Active-hub failure#417sumanthd032 wants to merge 32 commits into
sumanthd032 wants to merge 32 commits into
Conversation
Introduce pkg/ha, the foundation for cross-cluster (Active/Standby) high availability per ADR kubeslice#293 (issue kubeslice#294). - HAMode (active|standby|standalone) with fail-safe parsing: empty or unknown input maps to standalone so a misconfig never disables writes. - Lease helpers over coordination.k8s.io/v1: acquire/renew (bumping leaseTransitions on takeover), get, and a leaseDuration+padding staleness check. - ClusterLeaderElector: IsLeader() reads an atomic flag kept current by background loops, so it is cheap enough to call at the top of every Reconcile and always reflects live leadership. StartLeaseRenewal (Active) renews the local Lease and releases leadership once the renew deadline is exceeded (natural fencing). WatchRemoteLease (Standby) reads the Active's Lease and logs staleness but does not promote; promotion is issue kubeslice#297. Standalone is the default and is always the leader, preserving today's single-hub behaviour (no regression). Unit tests are race-clean and cover leadership by mode, renew-deadline loss, lease staleness, and the standby-never-promotes boundary. vendor: add controller-runtime fake client + interceptor packages (test-only) via go mod vendor. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Wire pkg/ha into the controller so only the Active hub writes (issue kubeslice#294). - Add a LeaderElector field to all nine reconcilers and a per-call guard at the top of every Reconcile: a Standby logs "standby mode, skipping reconcile" and returns without writing. The guard is nil-safe, so a reconciler built without an elector keeps today's behaviour. - main.go: add --ha-mode, --ha-identity, --ha-active-kubeconfig, --ha-lease-duration, --ha-renew-deadline, --ha-retry-period and --ha-padding-seconds; construct the elector (building a remote client from the mounted Active kubeconfig in standby mode); start StartLeaseRenewal (active) or WatchRemoteLease (standby) and pass the shared signal-handler context to the manager. - Add coordination.k8s.io/leases RBAC for the Lease. --ha-mode=standalone is the default and is always the leader, so existing single-hub deployments are unaffected (no regression). The existing --leader-elect (in-cluster pod election) is left untouched. A controller test asserts the Standby skips and logs on every call. vendor: add go.uber.org/zap/zaptest/observer (test-only). Signed-off-by: Sumanth D <sumanthd032@gmail.com>
The HA leader-election Lease lives in the controller's own namespace and is already covered by the existing leader-election Role's leases grant (config/rbac/leader_election_role.yaml). The kubebuilder marker added a redundant cluster-wide grant and left the generated manifests out of sync (make manifests was not run). Replace it with a note pointing at the role that provides the permission, keeping markers and manifests consistent. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…hutdown - acquireOrRenewLease rounds LeaseDurationSeconds up to whole seconds and clamps to a minimum of 1, so a sub-second --ha-lease-duration is not truncated to 0 (invalid, and skews staleness checks). - StartLeaseRenewal and WatchRemoteLease return nil instead of ctx.Err() on context cancellation, so a graceful shutdown is not logged as an error. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
The Lease namespace defaulted to a hard-coded constant, but the controller runs in a namespace injected at runtime via KUBESLICE_CONTROLLER_MANAGER_NAMESPACE (downward API), and the leader-election Role that grants leases is namespaced to that deploy namespace. Deploying into any other namespace would create the Lease where the controller has no leases permission, so the Active could not renew it and would fence itself permanently. Add --ha-lease-namespace defaulting to KUBESLICE_CONTROLLER_MANAGER_NAMESPACE so the Lease lands in the controller's own namespace. Empty (local runs) falls back to the pkg/ha default. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
NewClusterLeaderElector fell back to the hard-coded DefaultLeaseNamespace whenever LeaseNamespace was empty, independent of main.go's flag default. Since the controller already exposes KUBESLICE_CONTROLLER_MANAGER_NAMESPACE to represent its actual runtime namespace, check that env var first so the package resolves correctly on its own, not only by accident of how main.go wires the --ha-lease-namespace flag default. Only falls back to DefaultLeaseNamespace when the env var is unset too (e.g. running outside a pod). Regression tests included. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
StartLeaseRenewal/WatchRemoteLease returning nil (not ctx.Err()) on context cancellation was fixed in 0da6d82 but never actually got a regression test. Also add coverage for: the mode-guard no-op branches, WatchRemoteLease failing fast without a remote client, getLease/ checkRemoteLeaseOnce propagating a missing-Lease error instead of reporting fresh, renewOnce keeping leadership on a transient failure within renewDeadline, and setLeader logging Acquired/Lost exactly once per transition (F4 in 294-evaluation.md). No production code changes. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…uard, ownerRef strip, status mirroring) Part of kubeslice#295 — the write path RemoteSyncer's workqueue will drive (next commit) to mirror KubeSlice CRDs from the Active hub onto the Standby. - MirroredResource + CRDMirrorSet (pkg/ha/mirror_set.go): the hub-side resource table (Project, Cluster, SliceConfig, ServiceExportConfig, SliceQoSConfig, VpnKeyRotation, WorkerSliceConfig, WorkerSliceGateway, WorkerServiceImport, plus core Namespace). Deliberately does not match issue kubeslice#295's own CRD table, which names Slice/SliceGateway/ ServiceExport — none of those types exist in this repo; they are worker-cluster data-plane CRDs owned by the separate worker-operator repo, irrelevant to hub-to-hub mirroring. - mirrorCreateOrUpdate/mirrorDelete (pkg/ha/mirror.go): strip resourceVersion/uid/managedFields/finalizers (+ ownerReferences only for VpnKeyRotation, the one mirrored type that carries one) before writing, label mirrored objects ha.kubeslice.io/synced-from=active, and only ever overwrite or delete a target object that already carries that label — the conflict guard that keeps the syncer off anything the Standby's own reconcilers or an operator created directly, including pre-existing namespaces like kube-system/default now that Namespace is an ordinary mirrored type rather than a special-cased cold-start step. - Every mirrored type has a status subresource. A plain Update() never touches .status once one is registered, so mirrorCreateOrUpdate always follows up with an explicit Status().Update() when the source object has a non-empty status — matching this repo's own UpdateStatus/CleanupUpdateStatus convention. Regression-tested. Fake-client unit tests cover create, update-of-existing, the conflict guard on both update and delete, delete-idempotent-on-NotFound, StripOwnerRefs true/false, and status mirroring. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Part of kubeslice#295. Registered via prometheus.NewHistogramVec/NewCounterVec + prometheus.MustRegister in pkg/ha's own init(), the same self-registration idiom metrics/prometheus.go already uses for KubeSliceEventsCounter — not routed through metrics.StartMetricsCollector, whose default labels are slice-specific and don't apply to a cross-cluster mirror. - ha_sync_lag_seconds{kind,operation}: time.Now() minus CreationTimestamp for creates; minus first-enqueue time for update/delete (RemoteSyncer's workqueue coalesces repeated events for the same object, so there's no single "delivery time" once a retry has backed off a few times — first-enqueue is the more useful number to alert on, since it reflects total time since the triggering change). - ha_sync_errors_total{kind,operation}: counts mirror failures. The syncer keeps running and retries via its workqueue on every increment; this metric never indicates a crash. vendor: add prometheus/client_golang/prometheus/testutil (test-only) via go mod vendor, used to assert metric samples/labels directly. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Part of kubeslice#295. RemoteSyncer runs only in standby mode: builds a controller-runtime cache.Cache against the Active hub's rest.Config, registers one informer per CRDMirrorSet entry, and starts a small worker pool draining a rate-limited workqueue.TypedRateLimitingInterface [syncKey] — the same primitive controller-runtime's own Controller uses internally (internal/controller.Controller). Informer callbacks (handlersFor) only enqueue a syncKey; no mirror logic runs on the informer's own goroutine. A burst of Update events for the same object coalesces into one queued item, and a worker determines the real action at dequeue time by re-reading the Active cache (found -> mirrorCreateOrUpdate, NotFound -> mirrorDelete) — the same way a Reconcile call would. This is the commit that satisfies issue kubeslice#295's own acceptance criterion that the syncer "retries without crashing": on any mirror failure, processOnce calls queue.AddRateLimited instead of dropping the key. The concrete failure mode this fixes: a namespaced object (e.g. a new Project's Cluster) created on Active after the Standby's initial sync has already completed previously had no path to retry — nothing fires again for an object that didn't change on Active once its first mirror attempt failed on "namespace not found". Now it just gets retried a few seconds later once the Namespace mirror (an ordinary CRDMirrorSet row, no special-casing needed) has landed. Start(ctx) delegates its blocking wait directly to remoteCache.Start(ctx), which itself blocks on <-ctx.Done() and returns nil — giving the "return nil, not ctx.Err(), on graceful shutdown" contract StartLeaseRenewal/WatchRemoteLease already use, for free. remoteGetFunc is a small seam (defaulted to a real cache.Cache.Get in the constructor) so the retry engine's tests exercise real workqueue backoff/redelivery behaviour without a *rest.Config or live cluster. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Part of kubeslice#295. Constructs ha.RemoteSyncer alongside the existing ClusterLeaderElector, reusing the same remote *rest.Config and local client the elector already builds rather than loading the Active kubeconfig twice — hoisted the standby block's remoteCfg to an outer remoteHACfg variable so both consumers can see it. Starts remoteSyncer.Start(ctx) in its own goroutine alongside the existing leaderElector.WatchRemoteLease(ctx) in the ha.ModeStandby switch case; a no-op in any other mode, matching RemoteSyncer's own mode check. New flag: --ha-sync-workers (default ha.DefaultSyncWorkers), matching the existing --ha-* flag style. --ha-sync-interval is intentionally not added yet — it belongs to the periodic prune backstop (pkg/ha/ prune.go), which is out of scope for this PR and would otherwise be a flag with no consumer. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…mote read identity Part of kubeslice#295. --ha-active-kubeconfig's RBAC scope on the Active cluster has been undefined since kubeslice#294 introduced the flag — the dev demo uses a full-admin kubeconfig, and config/rbac/leader_election_role.yaml only ever granted configmaps/leases, nothing for the CRD/Namespace reads RemoteSyncer now needs. The ADR (kubeslice#293) itself names this as an unaddressed deployment-level boundary without resolving it. config/ha/active-cluster-clusterrole.yaml: a read-only (get/list/watch) ClusterRole covering Namespace plus every pkg/ha.CRDMirrorSet type, plus a ClusterRoleBinding template (subject left as a placeholder — it's deployment-specific, either a ServiceAccount for in-cluster dialing or a client-cert User for a flattened kubeconfig, and can't be hardcoded). config/ha/README.md explains this must be applied on the Active cluster by whoever provisions it, not through this repo's own deploy/kustomize flow — confirmed neither config/rbac/kustomization.yaml nor config/default/kustomization.yaml reference this directory, so it can never be accidentally auto-applied to the Standby's own cluster, where it would be meaningless. Also documents, ahead of time, that a later credential-mirroring PR appending Secrets to this grant exposes every Secret in the project namespaces on Active, not just the ones actually mirrored — RBAC can't scope Secrets by .type — a real tradeoff to weigh when that lands. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Start gave up permanently on the first GetInformer/AddEventHandler error instead of retrying, unlike StartLeaseRenewal/WatchRemoteLease. Split setup into registerInformers (retries with backoff) wrapping registerInformersOnce (one attempt). A naive retry would double-register handlers on resources that already succeeded, since AddEventHandlerWithResyncPeriod isn't idempotent. Added a handlerRegistered map so retries skip resources already done. CRDMirrorSet's Namespace entry had no filter, so it mirrored every namespace on the Active hub, not just kubeslice ones — and mirrorDelete's label-only guard meant an unrelated Active-side delete could cascade-delete one of those on the Standby. Scoped the Namespace informer to util.LabelsKubeSliceController via cache.Options.ByObject. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
An Active-side object mid-Terminating (deletionTimestamp set, contents still being garbage-collected) is delivered by the informer as an ordinary Update, not a Delete yet. mirrorCreateOrUpdate's payload carried deletionTimestamp straight through, and updating the Standby's non-terminating mirror with it set fails real API server immutable- field validation. Stripping it surfaced a second, related failure: the status-mirror block was still copying a Terminating status onto that now-non-terminating payload, which a real server also rejects (status.Phase may only be Terminating if deletionTimestamp is set). Both are stripped now: deletionTimestamp/deletionGracePeriodSeconds unconditionally alongside the other identity fields already cleared there, and status explicitly via delete(payload.Object, "status") rather than relying on a real API server silently ignoring .status on the main resource endpoint for subresource-registered types — a fake client does not replicate that behavior, so the code's correctness would otherwise depend on which client it's running against. The Standby still converges correctly once Active reports NotFound and mirrorDelete takes over; there's nothing useful to reflect about the in-between Terminating state. New tests: TestMirrorCreateOrUpdate_StripsDeletionTimestampFromTerminatingSource, TestMirrorCreateOrUpdate_SkipsStatusMirrorWhenSourceIsTerminating. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Dockerfile never copied pkg/ into the build context, so no image has been buildable since pkg/ha was introduced — go build works directly, docker build did not. Added COPY pkg/ pkg/ alongside the other source directories. config/rbac/role.yaml never granted namespaces/status: every other CRDMirrorSet type already has an explicit <kind>/status rule, but Namespace never needed one before it became a mirrored type with explicit status mirroring. Under real RBAC this permanently fails the Namespace status write. Added the +kubebuilder:rbac marker in main.go next to the existing namespaces rule and regenerated via make manifests. config/ha/active-cluster-clusterrole.yaml never granted coordination.k8s.io/leases: it was scoped only to what RemoteSyncer itself reads (Namespace + CRDMirrorSet), but the same --ha-active-kubeconfig identity is also used by WatchRemoteLease to read the Active's own Lease directly. Under a least-privilege identity using exactly this sample, the Standby could mirror correctly but never observe Lease staleness at all. Added the leases rule and updated the README to describe both consumers of this grant. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Informers self-heal missed updates via periodic resync and the workqueue
owns retry-on-failure, but neither can remove a mirror whose Active-side
original was deleted while the Standby wasn't watching (e.g. between two
Standby runs): cold-start informers only deliver what currently exists,
so such an orphan would survive forever.
Add a prune loop to RemoteSyncer that periodically lists Standby objects
carrying the ha.kubeslice.io/synced-from label per mirrored type, diffs
them against the remote cache, and enqueues anything no longer present
on the Active hub onto the ordinary mirror workqueue. The worker re-reads
Active at dequeue time, so the existing conflict guard, NotFound->delete
semantics, and rate-limited retry apply unchanged, and no second write
path races the workers.
Fail-safe choices:
- The first pass waits for the remote cache to sync; an unsynced cache
lists empty, which would otherwise read as "everything was deleted"
and prune every mirror on the Standby.
- A failed list (remote or local) skips that kind for the round and
increments ha_sync_errors_total{kind,"prune"} instead of pruning on
partial information.
Configurable via --ha-sync-interval (default 60s).
Part of kubeslice#295
Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…lure Surface mirror failures as Kubernetes events on the Standby, attached to the object that failed to sync, using the EventRecorder main.go already builds for the reconcilers. The entry lives in config/events/controller.yaml with the generated map and config-map output from make generate-events committed alongside — RecordEvent hard-fails for any EventName missing from the generated EventsMap, so skipping that step would silently no-op the whole feature. A regression test pins the entry's presence in the generated map (and that a missing entry errors loudly), so an accidental revert of the generated code fails in go test rather than at runtime. One event per failure episode, not per retry attempt: NumRequeues is 0 only on the first failure since the last success, and early workqueue backoff retries arrive milliseconds apart — although the recorder aggregates repeats into one Event's Count, every call is still an API-server write. ha_sync_errors_total continues to count every attempt. The recorder is called directly rather than through util.RecordEvent: that helper logs via util.CtxLogger, which panics on any context that didn't pass through a reconciler's request-context setup — true for the syncer's own context (main.go's signal-handler context). Caught by the new event-emission test before it could crash a live Standby. Part of kubeslice#295 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…Account, Role, RoleBinding)
Mirror the credential objects a promoted Standby needs to serve its
worker clusters without manual re-provisioning: worker-identity RBAC
(Role/RoleBinding/ServiceAccount — the only RBAC kinds
access_control_service.go ever creates; no ClusterRole or
ClusterRoleBinding exists to mirror, despite ADR Decision 6's broader
wording) and Secrets such as the gateway certificates the ovpn job
generates.
kubernetes.io/service-account-token Secrets are excluded: SA tokens are
signed by the issuing cluster's service-account key, so an Active-minted
token is cryptographically invalid on the Standby. Mirroring the
ServiceAccount is what matters — the Standby's own token controller
mints a locally-valid token for it.
Unlike the CRD set, these are core types that exist cluster-wide, so
every row is scoped in two layers:
- Server-side, the remote informers are scoped in cache.Options:
ServiceAccount/Role/RoleBinding by the same
util.LabelsKubeSliceController selector the Namespace informer already
uses (util.GetOwnerLabel embeds that exact key/value pair on every
credential object the controller creates), and Secret — which cannot
be label-scoped, the cert-generator job creates its Secrets unlabeled
— by a field selector excluding the SA-token type.
- Client-side, every row sets RequireMirroredNamespace: the object only
mirrors if its namespace is in the remote cache's label-scoped
Namespace view, i.e. a namespace the syncer itself mirrors. The
boundary is deliberately NOT name-based: under the Helm chart's
real-world --project-namespace-prefix ("kubeslice-"), the
controller's own kubeslice-controller namespace matches the
project-namespace naming pattern, and a prefix rule would have
mirrored its webhook TLS key, image-pull credentials, and Helm
release Secrets onto the Standby — found by running the Standby
against a Helm-installed Active hub. The label boundary is the one
ReconcileProjectNamespace actually maintains. A transient failure
reading the namespace surfaces as an error (workqueue retry), never
as a silent skip.
StripOwnerRefs is set on every row: ownerReferences resolve by UID,
which never survives a cross-cluster copy, and credential objects are
also written by actors outside this repo (token controller, cert job),
so the CRD set's audited only-VpnKeyRotation-needs-it reasoning cannot
hold here.
Verified live against the real two-hub Kind topology: worker SAs,
Roles, RoleBindings, and dashboard Secrets in both project namespaces
mirror with the sync label; SA-token Secrets and everything in
kubeslice-controller and kube-system stay off the Standby.
Part of kubeslice#295
Signed-off-by: Sumanth D <sumanthd032@gmail.com>
The prune loop so far only walked one direction — Standby mirrors whose Active-side original disappeared. Walk the other direction too: an Active-side object with no Standby mirror gets re-enqueued onto the ordinary workqueue, where the worker re-reads Active and runs the full Skip/namespace/conflict-guard chain. Three real cases produce that state, none of which the forward pass or the informers handle promptly: - a mirror someone deleted directly on the Standby (previously healed only by the informer's resync period, default 10 minutes); - an object whose RequireMirroredNamespace verdict was decided before its namespace's informer had delivered on cold start — a skip is terminal for that queue item, so without this it also waited for resync; - a key stuck deep in rate-limiter backoff after repeated failures (observed live: a fixed failure cause still took minutes to heal because the next retry was scheduled ~164s out; enqueue bypasses the rate limiter's delay, so recovery lands within one prune tick). Re-enqueueing is always safe: objects that should not mirror simply no-op through the guards again. Verified live: a mirrored Secret deleted directly on the Standby was re-created 15s later, within one --ha-sync-interval, with zero errors logged. Part of kubeslice#295 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…C sample The Standby now mirrors ha.FullMirrorSet — CRDMirrorSet plus CredentialMirrorSet. No new configuration: the mirrored-namespace gate derives entirely from the label boundary the controller already maintains, so the syncer needs no knowledge of the deployment's --project-namespace-prefix. config/ha/active-cluster-clusterrole.yaml gains read access to secrets/serviceaccounts (core) and roles/rolebindings (rbac), and the README's forward-looking credential-mirroring note becomes present-tense documentation. The Secret grant's security tradeoff is stated where it will be read rather than buried: RBAC cannot scope Secrets by .type or namespace label, and a ClusterRole binding is cluster-wide, so the Standby's identity can read every Secret on the Active hub — the syncer's field selector and mirrored-namespace gate narrow what gets copied, not what the identity could read. The narrower per-namespace RoleBinding alternative is documented alongside, with its maintenance cost. Grant coverage verified live via an impersonation can-i matrix: get/list/watch allowed for every mirrored type plus the HA Lease, writes and unrelated resources denied. Fixes kubeslice#295 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
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
There was a problem hiding this comment.
Pull request overview
This PR implements cross-cluster Active/Standby HA behavior for the KubeSlice controller, including detection of an unreachable Active hub and promotion of a Standby hub to Active, while adding supporting mirroring, metrics, events, and status publication needed for failover.
Changes:
- Add Standby→Active promotion flow with pre-promotion guards, bounded waits/timeouts, and a promotion event/metric.
- Add/extend Active→Standby mirroring (including prune anti-entropy, credential mirroring boundaries, and mirror failure events/metrics).
- Wire HA mode, leader gating (write fence), remote syncer, and activeController status publication into
main.goand reconcilers.
Reviewed changes
Copilot reviewed 45 out of 64 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/ha/promotion.go |
Implements Standby→Active promotion sequencing with hooks and bounded waits. |
pkg/ha/promotion_guards.go |
Adds self-health + final bounded re-read guard logic to reduce false promotions. |
pkg/ha/leader_elector.go |
Updates elector to retain last good remote Lease and trigger promotion on staleness. |
pkg/ha/remote_syncer.go |
Adds/extends remote mirroring engine behavior, informer scoping, retries, events, and prune integration. |
pkg/ha/prune.go |
Adds periodic diff-based pruning + reverse-diff re-enqueue to heal mirror drift. |
pkg/ha/mirror*.go / pkg/ha/mirror_set.go |
Defines mirror sets, conflict guard, status mirroring, and credential mirroring rules. |
pkg/ha/active_publisher.go |
Publishes status.activeController while leading; supports synchronous publish for promotion. |
pkg/ha/metrics.go |
Adds HA sync/failover/abort metrics and registers them. |
pkg/ha/*_test.go |
Adds extensive unit tests for promotion ordering, guards, mirroring, pruning, publisher, and metrics. |
main.go |
Adds HA flags/config, wires elector/syncer/publisher, and gates reconcilers by leadership. |
controllers/** |
Adds leader gating (write fence) at the start of reconcilers. |
events/events_generated.go + config/events/* |
Registers HA-related events in generated map and config. |
config/ha/* |
Adds Active-cluster RBAC manifest and guidance for Standby’s remote kubeconfig identity. |
config/rbac/role.yaml |
Grants namespaces/status permissions needed for mirroring status updates. |
Dockerfile |
Ensures pkg/ is included in the build context. |
vendor/** + vendor/modules.txt |
Updates vendored dependencies to include new test/util packages used by HA tests. |
Files not reviewed (1)
- apis/controller/v1alpha1/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+572
to
+576
| leaderElector.SetPromotionHooks(ha.PromotionHooks{ | ||
| // Cancel the mirror and WAIT for it to confirm it has stopped. The | ||
| // wait is the point: RemoteSyncer.Start already drains its workqueue | ||
| // and prune goroutine before returning, so returning from it is a | ||
| // sufficient and already-correct barrier. Without the wait, a hub can |
Comment on lines
+233
to
+243
| if e.hooks.KickReconcilers != nil { | ||
| kickCtx, cancel := context.WithTimeout(ctx, e.promotionGracePeriod) | ||
| err := e.hooks.KickReconcilers(kickCtx) | ||
| cancel() | ||
| if err != nil { | ||
| e.log.Errorw("could not re-enqueue objects after promotion; pre-existing state may "+ | ||
| "stay unreconciled until the informer resync period", "error", err) | ||
| } else { | ||
| e.log.Infow("re-enqueued all reconciled types after promotion") | ||
| } | ||
| } |
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
Implements failover (ADR #293): detecting the Active hub is gone and promoting the Standby.
status.activeController, open the write fence, kick a reconcile of existing objects. Idempotent, bounded by--ha-promotion-dial-timeout/--ha-promotion-grace-period.PromotedToActiveevent.This branch merges
feat/297-active-controller(#414) on top of #416. Its diff includes #414's commits until that merges. New commits here start at825f7531throughd1d33fc(7 commits plus the merge).Part of #297
How Has This Been Tested?
go test -race -count=1 ./pkg/ha/..., including a concurrency test for the promote-once-only latch. Live-tested against a Kind hub with its API server shut down: staleness detected in ~21s against a 20s budget.Checklist:
Does this PR introduce a breaking change for other components like worker-operator?
No. Promotion only runs under
--ha-mode=standby.