feat(ha): publish status.activeController from the leading hub - #414
Open
sumanthd032 wants to merge 11 commits into
Open
feat(ha): publish status.activeController from the leading hub#414sumanthd032 wants to merge 11 commits into
sumanthd032 wants to merge 11 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>
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>
7 tasks
There was a problem hiding this comment.
Pull request overview
Adds cross-cluster (Active/Standby) HA signaling for workers by introducing status.activeController on the Cluster CRD and a background publisher that continuously self-declares the active hub while it holds leadership, alongside wiring HA leader-election/write-fencing into the manager and reconcilers.
Changes:
- Add
ClusterStatus.activeController(with endpoint, CA bundle, identity, lastUpdated) to the API types + CRD schema. - Introduce
pkg/hacomponents for cross-cluster leader election and anActivePublisherloop that publishesstatus.activeControlleronly while leading (with prompt publication after late leadership acquisition). - Wire HA mode/flags into
main.goand gate reconciler writes on leadership; add unit tests for HA pieces.
Reviewed changes
Copilot reviewed 21 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vendor/sigs.k8s.io/controller-runtime/pkg/internal/objectutil/objectutil.go | Vendored helper for label filtering used by controller-runtime fake client. |
| vendor/sigs.k8s.io/controller-runtime/pkg/client/interceptor/intercept.go | Vendored interceptor client to inject behaviors in tests. |
| vendor/sigs.k8s.io/controller-runtime/pkg/client/fake/doc.go | Vendored docs for controller-runtime fake client. |
| vendor/sigs.k8s.io/controller-runtime/pkg/client/fake/client.go | Vendored controller-runtime fake client (enables richer unit tests, selectors, intercepts). |
| vendor/modules.txt | Vendor manifest updates for newly vendored packages. |
| vendor/k8s.io/apimachinery/pkg/util/rand/rand.go | Vendored rand util dependency pulled in by fake client. |
| vendor/go.uber.org/zap/zaptest/observer/observer.go | Vendored zap observer core used for log assertions in tests. |
| vendor/go.uber.org/zap/zaptest/observer/logged_entry.go | Vendored zap observer entry type used for log assertions in tests. |
| pkg/ha/mode.go | Defines HA modes and parsing behavior for --ha-mode. |
| pkg/ha/mode_test.go | Unit tests for HA mode parsing/validation. |
| pkg/ha/lease.go | Lease acquire/renew helpers + staleness detection for HA leadership. |
| pkg/ha/lease_test.go | Unit tests for lease helpers and edge cases (duration clamp, notfound, etc.). |
| pkg/ha/leader_elector.go | Cross-cluster leader election implementation for active renew / standby watch. |
| pkg/ha/leader_elector_test.go | Unit tests for leader election behavior and logging transitions. |
| pkg/ha/active_publisher.go | Publisher loop to write status.activeController while leading; includes promptness fix. |
| pkg/ha/active_publisher_test.go | Unit tests for publisher correctness, refusal rules, CA handling, promptness regression. |
| main.go | Adds HA flags, constructs HA clients/elector, starts HA loops + publisher, wires elector into reconcilers. |
| controllers/worker/workerslicegateway_controller.go | Gates reconcile writes on HA leadership (standby no-ops). |
| controllers/worker/workersliceconfig_controller.go | Gates reconcile writes on HA leadership (standby no-ops). |
| controllers/worker/workerserviceimport_controller.go | Gates reconcile writes on HA leadership (standby no-ops). |
| controllers/controller/vpnkey_rotation_controller.go | Gates reconcile writes on HA leadership (standby no-ops). |
| controllers/controller/sliceqosconfig_controller.go | Gates reconcile writes on HA leadership (standby no-ops). |
| controllers/controller/sliceconfig_controller.go | Gates reconcile writes on HA leadership (standby no-ops). |
| controllers/controller/serviceexportconfig_controller.go | Gates reconcile writes on HA leadership (standby no-ops). |
| controllers/controller/project_controller.go | Gates reconcile writes on HA leadership (standby no-ops). |
| controllers/controller/cluster_controller.go | Gates reconcile writes on HA leadership (standby no-ops). |
| controllers/controller/leader_gate_test.go | Asserts standby reconcile gating behavior + skip logging. |
| config/crd/bases/controller.kubeslice.io_clusters.yaml | CRD schema updated to include status.activeController. |
| apis/controller/v1alpha1/zz_generated.deepcopy.go | Adds deepcopy support for new ActiveControllerInfo + status field. |
| apis/controller/v1alpha1/cluster_types.go | Adds ActiveController to ClusterStatus and defines ActiveControllerInfo. |
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
+308
to
+312
| // Set up cross-cluster HA leader election (ADR #293 / issue #294). In | ||
| // standalone mode (the default) the elector is always the leader, so the | ||
| // reconciler write-fence is a no-op and behaviour is unchanged. | ||
| haRunMode := ha.ParseHAMode(haMode) | ||
| localHAClient, err := client.New(mgr.GetConfig(), client.Options{Scheme: scheme}) |
Comment on lines
+125
to
+134
| if opts.Identity == "" { | ||
| if hostname, err := os.Hostname(); err == nil { | ||
| opts.Identity = hostname | ||
| } else { | ||
| opts.Identity = "kubeslice-controller" | ||
| } | ||
| } | ||
| if opts.Log == nil { | ||
| opts.Log = util.NewLogger().With("name", "ha-leader-elector") | ||
| } |
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
Adds
status.activeControllerto the Cluster CRD (ADR #293 Decision 7). Each hub publishes its endpoint and identity on its own API server while it holds leadership, so a worker can find the Active hub after a failover. Field is additive andomitempty; non-HA deployments are unaffected.Stacked on #409. Only 4 commits are new here.
Part of #297
How Has This Been Tested?
Unit tests for the publish loop (leader/non-leader, endpoint validation, convergence). Verified live on a Kind hub: publishes within 2s of acquiring leadership.
Checklist:
Does this PR introduce a breaking change for other components like worker-operator?
No. Additive field, gated behind
--ha-mode.