fix(ha): make a promoted hub reconcile again (#297 PR3) - #5
Open
sumanthd032 wants to merge 4 commits into
Open
Conversation
…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>
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 usable. Two independent problems, one commit each.
A promoted hub error-looped on every registered cluster. The token Secret for a worker's ServiceAccount was only created in the branch that creates the account, which assumes the two are always absent together. The state mirror copies ServiceAccounts but not their tokens, since a token signed by one cluster is invalid on another, so a promoted hub found the account present, skipped the branch, never minted a token, and then failed every reconcile on the missing Secret while reporting a successful promotion. Checking the Secret's own existence is a no-op on a hub that created its own accounts, and it also repairs any cluster whose token Secret was deleted by hand.
Flipping the write fence caused no reconcile at all. The fence returns without requeuing, so requests dropped while fenced are discarded rather than parked, and nothing fires again until an object changes or the informer resyncs. Adds one event channel per reconciled type, filled once on promotion. Note that kubeslice#297's acceptance criterion about a new Slice reconciling passes without this, because a new object generates its own event; it is the pre-existing mirrored state that stays frozen.
Depends on kubeslice#297 PR2 (#4), so its commits appear in this diff.
Part of kubeslice#297
How Has This Been Tested?
go test -race -count=2 -shuffle=onclean, plus two new tests in controllers/controller.undefined: util.Clientin three files), which is unrelated to this change. Verified live instead, A/B against the same cluster state: without the fix, 28could not find secreterrors in 45s and no Secret; with it, 0 errors and the Secret created and populated by the local control plane. Re-running with both present created nothing and changed no UID, confirming it is a no-op on a healthy hub.Checklist:
Does this PR introduce a breaking change for other components like kubeslice-controller, worker-operator?
No. The kick field is optional and registers no watch when unset, so reconcilers behave identically outside HA. The service change is a no-op wherever the Secret already exists, which is every hub that created its own service accounts.