fix(ha): reconcile pre-existing state after promotion, not just new events - #418
Open
sumanthd032 wants to merge 36 commits into
Open
fix(ha): reconcile pre-existing state after promotion, not just new events#418sumanthd032 wants to merge 36 commits into
sumanthd032 wants to merge 36 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>
…f the account The token Secret was only ever created inside the branch that creates the ServiceAccount. That assumes the two are always absent together, which holds when this routine created both and fails as soon as a ServiceAccount arrives by any other means. Cross-cluster HA is where it bites. The state mirror copies ServiceAccounts but deliberately not their tokens, because a token signed by one cluster's key is invalid on another. So a promoted hub finds the account already present, skips the branch, never mints a token, and then fails every reconcile of every registered cluster on the missing Secret — while its logs and metrics report a successful promotion. The requeue guard in ClusterService.ReconcileCluster does not catch it either, and the reason is easy to miss: the ServiceAccount is built with its Secrets reference already populated, so the mirrored copy claims a Secret that does not exist and the nil check passes. Checking the Secret's own existence is behaviour-neutral on a hub that has only ever created its own accounts, since the Secret is present whenever the account is. It also repairs any cluster whose token Secret was deleted by hand, which today has no recovery path at all. This is shared, non-HA code on the cluster registration path, so it is kept to its own commit. Note that the service test package does not compile on master (undefined: util.Client in three test files), so no unit test could be added alongside it; the fix is verified against a live promoted hub instead. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Flipping the write fence causes no reconcile at all. The fence returns without requeuing, so every request a Standby dropped is discarded rather than parked, and nothing fires again until an object changes or the informer resyncs — ten hours by default. A promoted hub sits on state it believes it owns and never touches it. The most visible consequence is deletes. The mirror strips finalizers by design, and only a running reconciler re-adds them, so until this runs a delete on the promoted hub skips its cleanup entirely and the object simply vanishes. Worth stating for anyone reading the acceptance criteria: "a new Slice created on the Standby reconciles successfully" passes without any of this, because a new object generates its own event. It is the pre-existing mirrored state that stays frozen, so a green run there would imply a correctness that is not present. One channel per type, not one shared channel. Every source.Channel starts its own goroutine reading the channel it was handed, so nine sources over one Go channel would have nine goroutines competing for each value: every event reaches exactly one arbitrary controller and each type sees a random subset of its own objects. With a couple of objects in a test that looks like it works, which is what makes it worth a test of its own. Sends are non-blocking. The consumers only start draining once the manager is running and main.go starts the promotion path before mgr.Start, so a blocking send in that window would hang promotion on a channel nobody is reading — on a hub that has already taken leadership. A full channel is counted and logged instead. Losing a kick costs a reconcile that would have happened anyway on the next change or resync, which is exactly where the hub would be without this component. A type whose list fails is reported and the rest still run: partial coverage beats a promoted hub with nothing reconciled because one API call failed. The kicker is built unconditionally and each reconciler takes its channel as an optional field, so the controllers are identical in HA and standalone mode; outside HA the kick simply never fires. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
source.Channel rejects a nil channel when the manager starts the source — "must specify Channel.Source" — so registering the watch unconditionally breaks every caller that constructs a reconciler without wiring a kick. This package's own envtest suite is one such caller, and any out-of-tree consumer is another. The field was already documented as optional. It now actually is. Missed locally because the envtest suite cannot run on this machine at all (the controlplane binaries are absent), which hides a manager-start failure behind an earlier environment failure. Found by reading what source.Channel does with nil rather than by a red test. Part of kubeslice#297 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
The cancellation check was a select case beside the send. Both are ready whenever the channel has room, and select chooses among ready cases uniformly at random, so a cancelled context aborted the kick only about half the time. Checked explicitly before the send instead, which also keeps the send itself non-blocking. Found by -shuffle: the test passed on its own and failed under repetition, which is the only way this shows up. 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 strengthens HA failover behavior by ensuring a Standby→Active promotion reconciles pre-existing state (not just new events) and fixes a promotion-exposed gap where ServiceAccount token Secrets may be missing after mirroring. It also introduces supporting HA components (promotion guards, reconcile kick, mirror pruning, ActiveController publishing) plus the required RBAC/CRD/events updates.
Changes:
- Fix ServiceAccount token Secret creation by checking/creating the Secret independently of ServiceAccount creation.
- Add HA promotion “kick” + write-fence gating so a promoted hub actively reconciles existing objects and Standby hubs do not mutate state.
- Expand HA machinery (mirror pruning, promotion guards/events, metrics, ActiveController publisher) and update CRD/RBAC/events + vendor.
Reviewed changes
Copilot reviewed 49 out of 68 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| vendor/sigs.k8s.io/controller-runtime/pkg/internal/objectutil/objectutil.go | Vendored controller-runtime helper used by new code/tests. |
| vendor/sigs.k8s.io/controller-runtime/pkg/client/interceptor/intercept.go | Vendored controller-runtime interceptor client used by tests. |
| vendor/sigs.k8s.io/controller-runtime/pkg/client/fake/doc.go | Vendored controller-runtime fake client docs (vendor sync). |
| vendor/modules.txt | Vendor manifest updated for new vendored packages. |
| vendor/k8s.io/apimachinery/pkg/util/rand/rand.go | Vendored apimachinery rand util (vendor sync). |
| vendor/go.uber.org/zap/zaptest/observer/observer.go | Vendored zap observer for log assertions in tests. |
| vendor/go.uber.org/zap/zaptest/observer/logged_entry.go | Vendored zap observer support type. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/testutil.go | Vendored Prometheus testutil for metrics tests. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/validations/units.go | Vendored Prometheus promlint validations. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/validations/histogram_validations.go | Vendored Prometheus promlint validations. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/validations/help_validations.go | Vendored Prometheus promlint validations. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/validations/generic_name_validations.go | Vendored Prometheus promlint validations. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/validations/counter_validations.go | Vendored Prometheus promlint validations. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/validation.go | Vendored Prometheus promlint wiring. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/promlint.go | Vendored Prometheus promlint implementation. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/problem.go | Vendored Prometheus promlint problem type. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/lint.go | Vendored Prometheus testutil lint entrypoint. |
| service/access_control_service.go | Fix token Secret minting by checking Secret existence independently. |
| pkg/ha/remote_syncer_test.go | Tests for RemoteSyncer retry/handler behavior and informer setup. |
| pkg/ha/prune.go | Add prune loop to repair drift when deletes weren’t observed. |
| pkg/ha/prune_test.go | Tests for prune forward/reverse diff behavior and cache-sync guard. |
| pkg/ha/promotion.go | Promotion sequence now coordinates mirror stop, publish, kick, event. |
| pkg/ha/promotion_guards.go | Add self-health + final-dial guards to reduce false promotions. |
| pkg/ha/promotion_guards_test.go | Tests for guard correctness and dial timeout enforcement. |
| pkg/ha/promotion_event.go | Emit “promoted” event via Lease-attached involvedObject. |
| pkg/ha/promotion_event_test.go | Tests for event namespace behavior and EventsMap registration. |
| pkg/ha/mode.go | Define and parse HA modes (active/standby/standalone). |
| pkg/ha/mode_test.go | Tests for HA mode parsing/validation. |
| pkg/ha/mirror.go | Mirror write path with conflict guard + explicit status mirroring. |
| pkg/ha/mirror_test.go | Tests for mirror create/update/delete, status handling, conflict guard. |
| pkg/ha/mirror_set.go | Define mirrored resource sets, including credential mirroring rules. |
| pkg/ha/metrics.go | Add HA metrics (sync lag/errors, failover count, aborted promotions). |
| pkg/ha/metrics_test.go | Tests validating HA metric labeling/collection. |
| pkg/ha/lease.go | Lease acquire/renew helpers and staleness logic. |
| pkg/ha/lease_test.go | Tests for lease semantics, transitions, duration clamp. |
| pkg/ha/kicker.go | Reconcile kicker: per-type channels + non-blocking fanout after promotion. |
| pkg/ha/kicker_test.go | Tests for kicker correctness, isolation per type, cancellation, fullness. |
| pkg/ha/events_test.go | Tests for HAMirrorSyncFailed event emission semantics. |
| pkg/ha/credential_set_test.go | Tests for credential mirror boundary rules and secret-type skip. |
| pkg/ha/active_publisher.go | ActivePublisher maintains status.activeController on Cluster CRs. |
| events/events_generated.go | Add generated event schemas and constants for HA events. |
| Dockerfile | Ensure pkg/ is included in build context for new HA package. |
| controllers/worker/workerslicegateway_controller.go | Add promotion kick watch + leader write fence gating. |
| controllers/worker/workersliceconfig_controller.go | Add promotion kick watch + leader write fence gating. |
| controllers/worker/workerserviceimport_controller.go | Add promotion kick watch + leader write fence gating. |
| controllers/controller/vpnkey_rotation_controller.go | Add promotion kick watch + leader write fence gating. |
| controllers/controller/sliceqosconfig_controller.go | Add promotion kick watch + leader write fence gating. |
| controllers/controller/sliceconfig_controller.go | Add promotion kick watch + leader write fence gating. |
| controllers/controller/serviceexportconfig_controller.go | Add promotion kick watch + leader write fence gating. |
| controllers/controller/promotion_kick_test.go | Guard against nil PromotionKick causing source.Channel startup failure. |
| controllers/controller/project_controller.go | Add promotion kick watch + leader write fence gating. |
| controllers/controller/leader_gate_test.go | Test Standby reconcile gate behavior and logging. |
| controllers/controller/cluster_controller.go | Add promotion kick watch + leader write fence gating. |
| config/rbac/role.yaml | Add namespaces/status permissions (needed for mirroring status). |
| config/ha/README.md | Document cross-cluster RBAC application for standby’s remote kubeconfig. |
| config/ha/active-cluster-clusterrole.yaml | Provide least-privilege read RBAC for Standby on Active hub cluster. |
| config/events/events_config_map.yaml | Include new HA events in events ConfigMap. |
| config/events/controller.yaml | Define HA event schemas for generation. |
| config/crd/bases/controller.kubeslice.io_clusters.yaml | CRD schema update: add status.activeController fields. |
| apis/controller/v1alpha1/zz_generated.deepcopy.go | Generated deepcopy updates for ActiveControllerInfo. |
| apis/controller/v1alpha1/cluster_types.go | API type update: add ClusterStatus.ActiveController + type definition. |
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
+72
to
+74
| if t.LeaderElector != nil && !t.LeaderElector.IsLeader() { | ||
| t.Log.Info("standby mode, skipping reconcile") | ||
| return ctrl.Result{}, nil |
Comment on lines
+72
to
+74
| if c.LeaderElector != nil && !c.LeaderElector.IsLeader() { | ||
| c.Log.Info("standby mode, skipping reconcile") | ||
| return ctrl.Result{}, nil |
Comment on lines
+60
to
+62
| if r.LeaderElector != nil && !r.LeaderElector.IsLeader() { | ||
| r.Log.Info("standby mode, skipping reconcile") | ||
| return ctrl.Result{}, nil |
Comment on lines
+59
to
+61
| if r.LeaderElector != nil && !r.LeaderElector.IsLeader() { | ||
| r.Log.Info("standby mode, skipping reconcile") | ||
| return ctrl.Result{}, nil |
Comment on lines
+73
to
+75
| if r.LeaderElector != nil && !r.LeaderElector.IsLeader() { | ||
| r.Log.Info("standby mode, skipping reconcile") | ||
| return ctrl.Result{}, nil |
Comment on lines
+73
to
+75
| if r.LeaderElector != nil && !r.LeaderElector.IsLeader() { | ||
| r.Log.Info("standby mode, skipping reconcile") | ||
| return ctrl.Result{}, nil |
Comment on lines
+60
to
+62
| if r.LeaderElector != nil && !r.LeaderElector.IsLeader() { | ||
| r.Log.Info("standby mode, skipping reconcile") | ||
| return ctrl.Result{}, nil |
Comment on lines
+73
to
+75
| if c.LeaderElector != nil && !c.LeaderElector.IsLeader() { | ||
| c.Log.Info("standby mode, skipping reconcile") | ||
| return ctrl.Result{}, nil |
Comment on lines
+60
to
+62
| if r.LeaderElector != nil && !r.LeaderElector.IsLeader() { | ||
| r.Log.Info("standby mode, skipping reconcile") | ||
| return ctrl.Result{}, nil |
| } | ||
| foundSecret, err := util.GetResourceIfExist(ctx, secretNamespacedName, &corev1.Secret{}) | ||
| if err != nil { | ||
| logger.With(zap.Error(err)).Errorf("Couldnt fetch service account secret") |
pnavali
requested review from
Rahul-D78,
gourishkb and
rajendra-avesha
and removed request for
that-backend-guy
August 18, 2026 15:06
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 68 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- apis/controller/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (4)
service/access_control_service.go:1
- The new log line has a typo (“Couldnt”) and is missing key context (namespace/name) that would help debug production failures. Consider changing it to a grammatically correct message and include
secretNamespacedName.Namespace/secretNamespacedName.Name(and ideally clarify that this is the ServiceAccount token Secret) so operators can identify the failing object quickly.
pkg/ha/remote_syncer.go:1 - The last word “Informer.” looks like an accidental fragment and makes the comment misleading/unfinished. Please remove it or rephrase the sentence so the doc comment reads cleanly.
pkg/ha/remote_syncer.go:1 - The license header contains
# # SPDX-License-Identifier: Apache-2.0, which is not a standard SPDX tag format and looks like an artifact (the# #). For compliance tooling and consistency, change this to a conventional SPDX line (e.g.,SPDX-License-Identifier: Apache-2.0) without the extra characters, and consider applying the same fix across the newly added files that share this header.
controllers/controller/cluster_controller.go:73 - Logging
Infoon every skipped reconcile can create high log volume in Standby (especially if informers keep firing due to mirrored updates/resyncs). Consider lowering this toDebugand/or only logging on transition (e.g., when leadership status changes) to avoid noisy logs in steady state.
func (c *ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
// HA write fence: only the Active hub (or a standalone controller) writes.
// A Standby evaluates this on every call and no-ops.
if c.LeaderElector != nil && !c.LeaderElector.IsLeader() {
c.Log.Info("standby mode, skipping reconcile")
return ctrl.Result{}, nil
}
Comment on lines
+379
to
+380
| // both modes. | ||
| reconcileKicker := ha.NewReconcileKicker(mgr.GetClient(), ha.ReconciledGVKs(), |
Contributor
|
@gourishkb @Rahul-D78 please review. |
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
Makes a promoted hub actually reconcile the state it inherits:
-shuffle).Stacked on #417. Only 4 commits are new here.
Part of #297
How Has This Been Tested?
go test -race -shuffle=on ./pkg/ha/... ./controllers/.... No unit test for the token-Secret fix: theservicetest package doesn't currently compile onmaster(filed separately). Verified against a live promoted hub instead.Checklist:
Does this PR introduce a breaking change for other components like worker-operator?
No. The reconcile kick only exists under
--ha-mode.