test(hub): cover #468's failover robustness scenarios - #501
Merged
veenamj-avesha merged 11 commits intoAug 29, 2026
Merged
Conversation
A worker cannot learn about a controller failover from the hub that just failed, because the promotion is recorded on the other hub. So each hub publishes status.activeController on this worker's own Cluster CR while it holds leadership, and a Standby's mirrored copy repeats the Active's declaration. This is the worker's half: read that field from both pre-provisioned endpoints and decide who to talk to. Wired to nothing. No caller, no behaviour change, and a non-HA worker is byte-identical. The decision logic is the part with real substance, it reviews on its own, and landing it separately keeps the change that touches the hub connection small when it comes. The rule, in order: an unreachable hub has no say; a hub that published nothing has no say, which is what every non-HA deployment looks like from here; a claim naming an endpoint outside the configured candidate set is rejected, because the field selects among endpoints an operator provisioned rather than pointing the worker at arbitrary addresses; agreement between the hubs wins, and agreement is the normal case since the Standby mirrors the Active's declaration; disagreement prefers the fresher declaration, which keeps behaviour single-valued during the split brain the design does not claim to solve. No usable claim means change nothing — a worker that disconnected whenever it was unsure would turn every hub blip into a worker outage. Two properties worth their own tests. A switch needs consecutive confirming polls, so one divergent poll cannot move a worker, and the comparison excludes LastUpdated: the Active republishes on a timer, so including it would reset the counter every poll and no switch could ever confirm. Every read is deadline-bounded, because an API server that accepts a connection and then stops answering hangs until the OS TCP timeout otherwise; the controller side of this feature shipped that bug and measured a single read blocking ~12s against a stopped API server. The field is read unstructured rather than through the shared github.com/kubeslice/apis types. It is four scalars out of one status field, and reading them untyped keeps a third repository's release cadence off the critical path of a package that is otherwise self-contained. Nothing in go.mod, go.sum or vendor/ changes as a result. Part of kubeslice#467 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…ment The endpoint and the credentials for a hub are only valid as a pair, but they were reaching the client builders by two different routes. HUB_HOST_ENDPOINT was read inside NewHubClientConfig and manager.Start, while HubTokenFile and HubCAFile are package-level vars evaluated before main runs. That difference does not matter with one hub and cannot be worked around with two. Overriding the environment from main would move the address without moving the token, producing a client aimed at one hub authenticating as the other — which fails as a TLS or authorization error and reads like a network fault. Both now take a hub.Connection carrying all three together. Callers with a single hub pass PrimaryConnection(), built from the same environment lookups as before, so the resulting rest.Config is identical field for field. Also exports resolver.Prober, which was unexported and therefore unnameable by the first package to build one. Left alone deliberately: the hub manager's webhook server still takes its Host from the HubEndpoint package var. It registers no webhooks, so the server never starts and the value is dead config; changing it here would mean altering behaviour in a commit whose whole point is not to. Part of kubeslice#467 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
A worker pinned to one hub endpoint cannot survive that hub being promoted away from. With the controller running Active/Standby, leadership can move, and until now the only way to point a worker at the new Active was to edit its deployment. The worker now resolves which hub holds leadership before it opens any connection, and keeps watching. When the answer changes and holds across consecutive polls, it logs, counts it, and shuts down cleanly; the kubelet restarts it and startup resolution picks the hub that is now Active. Restarting rather than rebuilding in place is the deliberate choice. Both hub connections are assembled once from a rest.Config, and manager.Start already exits the process on any hub error, so a clean restart is both the smaller change and the one this process is built for. The data plane is untouched either way: gateways and tunnels run in their own pods. Everything is gated on HUB_SECONDARY_HOST_ENDPOINT. Unset, which is every deployment today, no resolver is built, no extra client is opened, and the connection is the same one the environment has always described. The resolution rule refuses claims naming any endpoint outside the two configured hubs, so a Cluster CR cannot redirect a worker somewhere nobody provisioned. Two metrics: kubeslice_worker_hub_switches_total, and kubeslice_worker_hub_probe_errors_total by hub slot. The second is the one worth alerting on — a hub that has been quietly unreachable for days is a problem to hear about before a failover rather than during one. Not implemented: issue kubeslice#467 asks for a ControllerConnected condition. ClusterStatus has no Conditions field, so that needs a further change to the shared apis module, and a hub-side condition can only be written while the hub is reachable — it can never report the state anyone wants to see. Local metrics and logs carry it instead. A durable local health surface belongs with kubeslice#469. Part of kubeslice#467 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Describes the resolution rule, the configuration, the metrics, and how to verify the whole thing in Kind. config/manager/manager.yaml gains the env, volume mount and volume for the second hub, commented out, so the shape is visible where an operator would look for it. The part worth being explicit about is the second credential. A worker authenticates with a token the hub it talks to minted, so following a failover needs a Standby-valid credential mounted before the Active fails — afterwards nothing is left to hand it one. The controller side already produces it: the Standby mirrors the worker's ServiceAccount and an empty token Secret shell, and its own token controller fills that shell in. Getting it from there onto the worker belongs to cluster registration and the charts, which live in neither repository, so it currently has no owner. The manual procedure is written out rather than left implied, including a check that the token actually authenticates before it is installed — a credential that is present but invalid is worse than none, because it fails only at failover. Part of kubeslice#467 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Startup resolution never worked at any confirmation setting above one. The caller resolves once before opening a connection, but the very first winner had to clear the same consecutive-poll threshold as a later switch — and a single call can never reach it. The worker fell back to its configured primary every time, including when the primary is the hub that just lost leadership, which is the one case startup resolution exists for. The threshold guards a *change* of hub: it stops one divergent poll re-pointing a worker that is already connected somewhere. Before anything is established there is no connection to protect and nothing to flap between, so the first winner is now taken as it stands. Later changes are unaffected. Found by running the failover demo against live clusters, where startup logged "candidate active hub not yet confirmed, seen 1 need 2" and then ignored a hub that had plainly declared itself. No unit test caught it because every existing test drove the resolver in a loop, which is exactly what startup does not do. TestResolve_RequiresConsecutiveConfirmations asserted the old behaviour in passing and has been corrected. Part of kubeslice#467 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
sumanthd032
requested review from
Rahul-D78,
bharath-avesha,
gourishkb and
richiesebastian
as code owners
August 25, 2026 16:42
7 tasks
sumanthd032
force-pushed
the
test/468-failover-robustness-tests
branch
from
August 26, 2026 05:15
238302d to
aad9009
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds worker-operator support code and tests for hub controller failover robustness, including active-hub resolution logic, connection health reporting (conditions/events/metrics), and documentation/config updates to enable a secondary hub credential.
Changes:
- Introduces a new hub resolver (probe + selection + anti-flap confirmation) and a failover follower that restarts the worker on a confirmed hub leadership move.
- Threads startup failover context into the hub-side cluster reconciler to set
ControllerConnected/ControllerEndpointSyncedconditions and emit new controller-connection events. - Adds unit tests covering the four robustness scenarios, plus docs and example deployment configuration for secondary hub credentials.
Reviewed changes
Copilot reviewed 28 out of 32 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| vendor/modules.txt | Vendors updated module replacement metadata for github.com/kubeslice/apis. |
| vendor/github.com/kubeslice/apis/pkg/controller/v1alpha1/zz_generated.deepcopy.go | Vendored deepcopy updates to support new status fields (conditions, active controller, storage capabilities). |
| vendor/github.com/kubeslice/apis/pkg/controller/v1alpha1/cluster_types.go | Vendored API type updates adding ActiveController, Conditions, and storage capability structs. |
| tests/hub/hub_suite_test.go | Updates hub test suite wiring to pass ConnectionInfo into reconciler. |
| pkg/hub/resolver/resolver.go | New resolver implementing active-hub selection and anti-flap confirmation. |
| pkg/hub/resolver/resolver_test.go | Unit tests for resolver correctness, determinism, and trust boundary behavior. |
| pkg/hub/resolver/probe.go | New unstructured probe for reading status.activeController with per-read timeouts. |
| pkg/hub/resolver/probe_test.go | Unit tests for probe decoding and timeout/error behavior. |
| pkg/hub/manager/manager.go | Hub manager now takes an explicit hub connection and passes connection info to the cluster reconciler. |
| pkg/hub/hubclient/hubclient.go | Hub client builder now takes a Connection instead of reading env directly. |
| pkg/hub/hubclient/connection.go | New connection wrapper that keeps endpoint/token/CA as an atomic triple. |
| pkg/hub/hubclient/connection_test.go | Tests ensuring Connection and PrimaryConnection() are consistent and complete. |
| pkg/hub/hubclient/classify.go | Adds connection-error classification (DialFailed vs CertVerificationFailed). |
| pkg/hub/hubclient/classify_test.go | Tests for certificate vs non-certificate error classification. |
| pkg/hub/failover/robustness_test.go | Adds robustness tests mapping to #468 scenarios (signal correctness, cert failure safety, non-HA precedence). |
| pkg/hub/failover/nogateway_test.go | Static import-safety test ensuring failover-following code doesn’t import gateway/tunnel packages. |
| pkg/hub/failover/failover.go | New follower that uses resolver/probe and exports hub failover metrics. |
| pkg/hub/failover/failover_test.go | Unit tests for config parsing, startup resolution, and watch-trigger behavior. |
| pkg/hub/controllers/cluster/reconciler.go | Plumbs ConnectionInfo into reconciler and sets hub-connection conditions during reconcile. |
| pkg/hub/controllers/cluster/reconciler_unit_test.go | Updates unit tests for reconciler constructor signature change. |
| pkg/hub/controllers/cluster/deregister_unit_test.go | Updates deregister unit tests for reconciler constructor signature change. |
| pkg/hub/controllers/cluster/conditions.go | New connection-condition logic (ControllerConnected, ControllerEndpointSynced) + event emission. |
| pkg/hub/controllers/cluster/conditions_test.go | Tests for non-HA, reconnect-once semantics, and idempotency. |
| pkg/hub/controllers/cluster/cluster_suite_test.go | Updates envtest suite wiring to pass ConnectionInfo. |
| pkg/hub/controllers/cluster/backward_compat_test.go | Explicit test that non-HA state takes precedence over a stray Reconnected flag. |
| main.go | Adds startup hub selection + watch-based restart on failover, plus best-effort “connection lost” reporting. |
| go.sum | Updates sums for the replaced apis module source. |
| go.mod | Adds a replace directive redirecting github.com/kubeslice/apis to a fork/pseudo-version. |
| events/events_generated.go | Adds new controller connection-related events to generated events map. |
| docs/hub-failover.md | New documentation describing the failover-following design, configuration, and limitations. |
| config/manager/manager.yaml | Documents commented-out env/volume wiring for the secondary hub credential. |
| config/events/worker-events.yaml | Adds event definitions for new controller connection events. |
Suppressed comments (1)
pkg/hub/resolver/resolver.go:204
Probecan returnReachable=truewith a non-nilErr(e.g., NotFound). In that casegather()currently falls intoverdict.Claim == niland logs “published no activeController”, which is misleading and drops the actual error signal. Handleverdict.Err != nilexplicitly so NotFound/misconfig errors don’t get silently masked as the non-HA case.
switch {
case !verdict.Reachable:
r.log.V(1).Info("hub candidate unreachable", "hub", candidate.Name,
"endpoint", candidate.Endpoint, "error", verdict.Err)
case verdict.Claim == nil:
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+176
to
+182
| // Resolve probes every candidate once and returns the hub the worker should be | ||
| // talking to, or nil to mean "change nothing". | ||
| // | ||
| // Returning nil is a real answer, not an error: with no usable claim the | ||
| // correct behaviour is to leave the existing connection alone. A worker that | ||
| // disconnected whenever it was unsure would turn every hub blip into a worker | ||
| // outage, which is strictly worse than talking to a hub that might be stale. |
Comment on lines
+198
to
+204
| counted := func(ctx context.Context, c resolver.HubCandidate) resolver.Verdict { | ||
| v := probe(ctx, c) | ||
| if !v.Reachable { | ||
| hubProbeErrorsTotal.WithLabelValues(c.Name).Inc() | ||
| } | ||
| return v | ||
| } |
Comment on lines
+372
to
+393
| func reportConnectionLost(hubClient client.Client, er *monitoringEvents.EventRecorder) { | ||
| cr := &hubv1alpha1.Cluster{} | ||
| err := hubClient.Get(context.Background(), client.ObjectKey{ | ||
| Name: controllers.ClusterName, | ||
| Namespace: hub.ProjectNamespace, | ||
| }, cr) | ||
| if err != nil { | ||
| setupLog.With("error", err).Info("could not report connection loss before reconnecting; the hub may already be unreachable") | ||
| return | ||
| } | ||
| meta.SetStatusCondition(&cr.Status.Conditions, metav1.Condition{ | ||
| Type: hubCluster.ConditionControllerConnected, | ||
| Status: metav1.ConditionUnknown, | ||
| Reason: hubCluster.ReasonReconnecting, | ||
| Message: "following a resolved hub failover; reconnecting", | ||
| }) | ||
| if err := hubClient.Status().Update(context.Background(), cr); err != nil { | ||
| setupLog.With("error", err).Info("could not persist the Reconnecting condition before restart") | ||
| return | ||
| } | ||
| utils.RecordEvent(context.Background(), er, cr, nil, ossEvents.EventControllerConnectionLost, "hub-failover") | ||
| } |
Comment on lines
+175
to
+179
| // Decide which hub to talk to before any client is built. Inert unless | ||
| // HUB_SECONDARY_HOST_ENDPOINT is set, in which case hubConn is exactly the | ||
| // primary connection this worker has always used. | ||
| hubConn := hub.PrimaryConnection() | ||
| failoverCfg := failover.ConfigFromEnv() |
| sigs.k8s.io/yaml v1.4.0 // indirect | ||
| ) | ||
|
|
||
| replace github.com/kubeslice/apis => github.com/sumanthd032/apis v0.5.1-0.20260825105516-d7d920d4b404 |
Pulls in ActiveController/StorageCapabilities (already merged locally, never vendored) and the new Conditions field kubeslice#469 needs, taken from the upstream commit that merged kubeslice/apis#47. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…icationFailed Distinguishes a TLS/cert-handshake failure from everything else, for issue kubeslice#469's ControllerConnected reason table. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
… worker's Cluster CR Adds the two conditions, four events and two metrics from issue kubeslice#469, wired through main.go/manager.Start/the cluster reconciler. Only ever writes Connected, ReconnectedAfterFailover or EndpointNotConfigured live; DialFailed/CertVerificationFailed are unit-tested but architecturally can't persist on a hub this worker can't reach. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…metrics Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…t handoff Fills the reconciler-level test kubeslice#469 asked for but the prior commit skipped: non-HA, the one-shot ReconnectedAfterFailover-then-Connected handoff, LastTransitionTime idempotency, and that a live failure reason never gets set. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Reconnect-signal correctness in both directions, cert-verification-failure classification and safe fallback, a static check that the failover package never touches gateway/tunnel code, and non-HA precedence over a stray Reconnected flag. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
sumanthd032
force-pushed
the
test/468-failover-robustness-tests
branch
from
August 29, 2026 05:59
aad9009 to
30bd650
Compare
veenamj-avesha
merged commit Aug 29, 2026
30bd650
into
kubeslice:ha-integration-branch
3 checks passed
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
Covers issue #468's four scenarios at whatever level each is actually testable: reconnect-signal correctness in both directions, cert-verification-failure classification with a safe fallback, a static check that the failover-following package never imports gateway/tunnel code, and non-HA precedence over a stray Reconnected flag.
Stacked on #500. Only 1 commit is new here.
Fixes #468
How Has This Been Tested?
go test -race -count=1 ./pkg/hub/...clean, including the envtest/ginkgo suites. Live cross-cloud tunnel continuity was not re-verified here: it is separately blocked by an unrelated worker-CRD-skew defect, unrelated to this change, documented in docs/hub-failover.md.Checklist:
Does this PR introduce a breaking change for other components like kubeslice-controller?
No. Test-only change.