[HYPERSHELL-173] Implement GatewayRelease reconciliation - #235
Conversation
Replace the no-op GatewayReleaseReconciler stub with a real reconciler that validates the release image reference, writes back a deterministic status (Available/Invalid), and fans out to referencing gateways when a release image changes. Fan-out reuses a shared gateway reconcile queue via EnqueueForced so the gateway phase gate (which skips Running/Provisioning/Degraded gateways) is bypassed on release-driven re-reconciles. The GatewayRelease watch now runs through a reconcile queue for per-release serialization and retry. An invalid image is not propagated and retains the last valid image baseline, so a later correction to a different image is still detected as a genuine change and fans out. Adds unit tests for the release reconciler and watcher-level tests proving the phase-gate bypass wiring. Adds the behavior spec. Refs HYPERSHELL-173 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. This is a clean, well-documented, and well-tested implementation of GatewayRelease reconciliation: image validation, idempotent status write-back, and change-driven fan-out are all correct and match the accompanying behavior spec. I am not blocking on this PR's own merit, but there are material cross-PR design decisions (below) that maintainers should settle before/around merge, plus a few minor observations.
What's good
- Deterministic, idempotent status write-back (no redundant write when persisted status already matches), with
errors-style wrapping (fmt.Errorf("...: %w", err)) on every failure path. - Fan-out is correctly scoped to gateways referencing the release by
release_id, and only fires on a genuine effective-image change. - The invalid-image-retains-baseline behavior is subtle and correctly reasoned; the regression test (
TestGatewayRelease_CorrectionAfterInvalidFansOut) locks it in. - Failures return errors so the new reconcile queue retries them rather than logging-and-dropping; the fan-out-failure-not-advancing-baseline path is tested.
- No
panic(), no secrets logged, image reference validated before it is ever logged (so no log-injection via the%simage log), delete treated as an idempotent no-op. Spec added and indexed.
Findings
All findings are Minor; see inline comments for specifics.
- [Minor] The
lastImagechange-detection baseline is in-memory only and theGatewayReleasewatch intentionally has no startup seed, so a release image edited while the controller is down is not re-observed on restart and never fans out. This is harmless today (release_id -> image resolution is out of scope) but becomes a latent gap once that resolution lands. - Reconciliation / restart durability - [Minor] The
activecreate-or-skip map is now redundant with the per-release reconcile queue's serialization, and it returnsnil(success) on a skip, which would mask a dropped reconcile if the invariant ever broke. - Reconciliation pattern - [Minor]
listGatewaysForReleasepages the entire gateway inventory and filters byrelease_idclient-side on every image change; fine at current scale but worth a note as fleets grow. - Efficiency
Cross-PR coordination
-
#151 redefines the gateway reconciler's provisioning gate from a phase gate (skip
Running/Provisioning/Degraded) to a generation/observed_generationconvergence gate, deleting the exactgw.Phasecheck this PR's fan-out is built to defeat. This PR propagates release image changes by force-clearing a gateway's phase (EnqueueForced->clearGatewayPhaseForRetry) so the phase gate does not skip aRunninggateway. If #151 lands, clearing the phase no longer bypasses the gate, and release-driven fan-out to an already-converged gateway would be silently skipped. Maintainers need to decide the merge order and how release fan-out signals "re-provision" under a convergence gate (e.g. bump desired generation) rather than by clearing phase. -
#185 specifies periodic control-plane world synchronization that explicitly includes
GatewayReleaseinventory polling, requires the reconcile queue to replace any "create-or-skip or active-map pattern," and constrains the shared Gateway queue so periodic sync SHALL NOT force-clear a gateway phase. This PR's design makes the opposite assumptions: release change detection lives in an ephemeral in-memorylastImagebaseline with no release resync, and it introduces an out-of-bandEnqueueForced(phase-clearing) enqueuer onto the shared Gateway queue. Maintainers should decide whether release change detection becomes durable/world-sync-driven and how force-clear ownership on the shared queue is bounded. -
#200 defines a control-plane reconciliation contract requiring status/finalizer writes to be conditional on resource generation ("a stale pass SHALL NOT publish success for a newer generation") and change detection anchored to generation. This PR writes release status and detects change without any generation conditioning (in-memory
lastImage). A decision is needed on whether this reconciler must conform to that contract (and therefore whether it should land before or be reworked after the contract is adopted).
| // The first observation records a baseline without fanning out: a brand-new | ||
| // release has no referencing gateways yet, and on controller restart every | ||
| // release would otherwise force-reconcile every running gateway. | ||
| prev, seen := r.lastImageFor(event.ResourceID) |
There was a problem hiding this comment.
The lastImage baseline is in-memory and WatchGatewayReleases deliberately has no startup seed, so change detection resets on every controller restart. A release whose image is edited while the controller is down is not re-observed on reconnect (the stream only carries future events), so its referencing gateways are never fanned out; and the first post-restart event for any release is classified as a baseline (seen == false) and skips fan-out.
This is harmless while release_id -> image resolution is out of scope, but it becomes a real convergence gap once gateways consume the release image. Consider deriving the change signal from persisted state (e.g. the release generation / previously-reconciled image on the resource) rather than process-local memory, or from the world-sync inventory pass, so a change across a restart is still detected. See the Cross-PR coordination note.
|
|
||
| _, endSpan := cpotel.StartReconcileSpan(ctx, "GatewayRelease", event.Type.String()) | ||
| defer func() { endSpan(nil) }() | ||
| var reconcileErr error |
There was a problem hiding this comment.
(Refers to the active create-or-skip guard at the top of Handle.) That guard is now redundant: WatchGatewayReleases drives this handler through a per-release reconcileQueue, which already serializes work per resource, so a concurrent second Handle for the same ID should never occur. If the invariant ever did break, the guard returns nil (success), which would cause the queue to treat a genuinely dropped reconcile as done and clear any retry/backoff - masking the drop rather than requeuing it. Consider removing the map now that serialization is owned by the queue (also called out as a pattern to retire in the world-sync design).
|
|
||
| // listGatewaysForRelease returns every gateway whose release_id references the | ||
| // given release, paginating through the API server. | ||
| func (r *GatewayReleaseReconciler) listGatewaysForRelease(ctx context.Context, releaseID string) ([]*pb.Gateway, error) { |
There was a problem hiding this comment.
listGatewaysForRelease pages the full gateway inventory and filters by release_id in the client on every image change. That is fine at current scale (page size matches the other reconcilers), but it is O(all gateways) per release edit with no server-side filter. Worth a follow-up to add a server-side release_id filter to ListGateways if fleet sizes grow, to avoid scanning unrelated gateways.
Cross-PR coordination — merge order with #151Following up on the Amber review's cross-PR findings. Short version: this PR (HYPERSHELL-173) is still required and correct as-is — no other open PR implements The one thing maintainers must not missThis PR's release fan-out re-provisions a referencing gateway by routing it through the shared queue with #151 replaces that phase gate with a convergence gate: - if gw.Phase != nil && (*gw.Phase == "Running" || "Provisioning" || "Degraded") { skip }
+ if gw.ObservedGeneration != nil && *gw.ObservedGeneration == gw.Generation { skip }Once #151 lands, clearing Requested decisionWhichever merge order maintainers pick, the release fan-out must migrate from phase-clear to bumping the gateway's desired
Follow-up ticket to track the migration is being filed (linked to #151). The reconciler's core logic — image validation, idempotent status write-back, change detection, and which gateways to fan out to — is unaffected either way; only the signal used to request re-provision changes. cc @jsell-rh |
2 participants
Summary
Replaces the no-op
GatewayReleaseReconcilerstub with a real reconciler for the cluster-local control plane (HYPERSHELL-173).A
GatewayReleasehas no direct Kubernetes footprint, so "reconciliation" here means:gateway.ValidateImageReference.Availablefor a valid image,Invalid: <reason>otherwise — idempotently (no redundant write when the persisted status already matches).Design notes
EnqueueForced, which clears the gateway phase so the gateway reconciler's phase gate (which skipsRunning/Provisioning/Degraded) does not silently drop a release-driven re-reconcile. Using directHandlecalls would both block the release watch goroutine on multi-minute provisioning and hit that phase gate.GatewayReleasewatch now runs through areconcileQueue(previously it only logged handler errors inline).Scope
In scope: image validation, deterministic status write-back, fan-out on image change, delete = no cluster footprint. Out of scope (sibling tasks):
release_id→image resolution (174), rollout/canary (175/176).Testing
gateway_release_test.go): valid/invalid status, no redundant write, fan-out targeting only referencing gateways, rename no-op, delete no-op, correction-after-invalid regression, status-write and fan-out failure retry.gateway_reconcile_queue_test.go) proving the phase-gate bypass wiring through the real shared queue.go build ./...,go vet ./...,gofmt,go test ./...andgo test -raceall green./amber-reviewloop (APPROVE after the invalid-baseline fix).Refs HYPERSHELL-173
🤖 Generated with Claude Code