[HYPERSHELL-174] Database-backed gateway version selection - #238
Conversation
|
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. The implementation is clean, spec-backed, and well-tested: release_id now correctly resolves to its GatewayRelease image, takes precedence over a direct image, and fails-closed (retries) instead of silently deploying a default when the release is unresolvable. The one substantive concern is a backward-compatibility question — release_id is a required field that was previously ignored, so making it authoritative changes the effective image (and can permanently fail reconcile) for gateways that already exist — which needs a maintainer/author confirmation rather than blocking the change.
Amber Analysis
Database-backed version selection is implemented exactly as the new gateway-version-selection.spec.md describes: release precedence, direct-image fallback, empty→platform-default, and deterministic retried failure on an unresolvable release. Error wrapping is correct (fmt.Errorf(... %w ...)), the stream context is propagated, there is no panic(), no secret exposure, and the tests are additive (no pre-existing assertion was flipped). The gRPC field usage (gw.ReleaseId, GetGatewayRelease, rel.Image) matches the generated stubs.
The findings below are about the transition from the previous "release_id stored but never resolved" behavior to "release_id is authoritative", not about the in-spec logic itself.
Major — behavior change for pre-existing gateways (precedence flip + hard-fail on dangling reference)
release_id is a required field on gateway creation (components/api-server/openapi/openapi.gateways.yaml), so every already-provisioned gateway carries a release_id that this code path previously ignored. After this PR:
- A gateway that has both a
release_idand a directimage(image is optional and some gateways were deployed from it) now silently switches to the release's image — the direct image no longer wins. This is the intended data-model behavior, but it retroactively changes the effective image of existing gateways. - A gateway whose
release_idpoints at a deleted / danglingGatewayReleasenow fails the reconcile permanently (retried forever) where it previously deployed from the direct image or the platform default.
Mitigating factors keep this at Major rather than Blocker: GatewayRelease.image is required on create, so a well-formed, still-existing release always resolves; and the phase gate (reconciler.go:1279) skips re-reconciling Running/Provisioning/Degraded gateways today, so the precedence flip is deferred until a gateway is recreated or a re-trigger mechanism lands.
Please confirm one of: (a) release deletion is blocked while a gateway references it (referential integrity), or (b) a dangling release_id cannot otherwise occur — and note the precedence change's effect on already-provisioned gateways in the PR description. Confidence: Medium.
Minor — unresolved-release failure is not surfaced on the gateway
When selectGatewayImage fails, Handle returns the error and the reconcile retries, but the gateway phase/status is not set (e.g. Failed) and there is no dedicated log for the retry loop. This is consistent with the sibling resolveDatabaseConfig/OIDC/route error paths (all before the Provisioning transition), so it is not a regression, but a gateway stuck retrying release resolution is invisible to an operator. Consider a phase/status write or a WARN log. Status write-back may belong to HYPERSHELL-173 scope. Confidence: Medium.
Cross-PR coordination
Two open pull requests require maintainer coordination with this change:
-
#235 (GatewayRelease reconciliation) introduces a
GatewayReleasestatus(Available/Invalid) and image validation. This PR's resolver deliberately does not gate on that status, so once #235 lands the control plane will still deploy the image of a release that #235 has flaggedInvalid. Maintainers should confirm this interim behavior is acceptable and decide whether/when release-image resolution should honor release status. Both PRs also drive "a release change reaches a gateway" from different angles (this PR resolvesrelease_id→image at reconcile time; #235 fans out to referencing gateways on release-image change by clearing the phase gate); they need to agree on a single re-trigger mechanism so the two do not implement divergent, competing paths. -
#151 (gate re-provisioning on desired-state convergence) replaces the phase gate this PR sits behind with a
generation/observed_generationconvergence gate. This PR's "version change through release reference is applied on reconcile" requirement is inert forRunninggateways under today's phase gate; it depends on #151 — specifically the API server must advancegenerationwhen a gateway'srelease_id(or its resolved image) changes — for repointingrelease_idto actually redeploy a running gateway. Maintainers should sequence this dependency; otherwise operators repointingrelease_idon a running gateway see no effect.
Findings Summary (ordered by severity, highest first)
- [Major]
release_idbecomes authoritative for existing gateways: precedence flip over directimage, and permanent reconcile failure on a dangling release reference, with no migration/fallback note — Backward Compatibility / Data Model (reconciler.go:1912, 1929) - [Minor] Unresolved-release failure not surfaced on the gateway (no phase/status, no retry log) — Observability (reconciler.go:1421)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| Proper context propagation (stream ctx) | Pass |
| No secrets in logs or error messages | Pass |
| Reconcile fails-closed (no silent default) | Pass |
| Status updated on error paths | Partial |
| Test diff additive (no flipped assertions) | Pass |
| Conventional commit message | Pass |
| // manifest layer to apply the platform default. See | ||
| // specs/platform/gateway-version-selection.spec.md. | ||
| func (r *GatewayReconciler) selectGatewayImage(ctx context.Context, gw *pb.Gateway) (string, error) { | ||
| if gw.ReleaseId != "" { |
There was a problem hiding this comment.
[Major] Precedence flip for pre-existing gateways. release_id is a required field on gateway creation, and until this PR it was stored but never resolved. Making it authoritative here means any already-provisioned gateway that carries both a release_id and a direct image (deployed from the direct image) will now switch to the release's image. That is the intended data-model behavior, but it retroactively changes the effective image of existing gateways. Please note this in the PR description; the impact is deferred by the phase gate (line 1279) until a gateway is recreated or a re-trigger lands.
| // See specs/platform/gateway-version-selection.spec.md. | ||
| func (r *GatewayReconciler) resolveReleaseImage(ctx context.Context, gw *pb.Gateway) (string, error) { | ||
| client := pb.NewGatewayReleaseServiceClient(r.grpcConn) | ||
| resp, err := client.GetGatewayRelease(ctx, &pb.GetGatewayReleaseRequest{Id: gw.ReleaseId}) |
There was a problem hiding this comment.
[Major] Dangling release_id now fails reconcile permanently. Because release_id is required and was previously ignored, a gateway whose release_id points at a deleted/missing GatewayRelease now returns an error here and retries forever, where it previously deployed from the direct image or the platform default. GatewayRelease.image being required on create mitigates the empty-image case, but not a dangling reference. Please confirm release deletion is blocked while a gateway references it (referential integrity), or otherwise that a dangling release_id cannot occur. The fail-closed choice itself is correct per the spec.
| // over a direct image, and an empty result lets the manifest layer apply the | ||
| // platform default. See specs/platform/gateway-version-selection.spec.md. | ||
| image, err := r.selectGatewayImage(ctx, gw) | ||
| if err != nil { |
There was a problem hiding this comment.
[Minor] Surface the unresolved-release failure. On error the reconcile retries but the gateway phase/status is not set (e.g. Failed) and there is no dedicated log, so a gateway stuck retrying release resolution is invisible to operators. This matches the sibling resolveDatabaseConfig/OIDC/route error paths (all pre-Provisioning), so it is not a regression, but a phase/status write or a WARN log would help. Status write-back may belong to HYPERSHELL-173 scope.
cdc7128 to
9196614
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR finally wires release_id -> GatewayRelease.image resolution into the reconciler with correct precedence over a direct image, and it does so cleanly and with good test coverage. The implementation is sound and matches the accompanying spec; my only in-PR note is a minor observability gap on the unresolvable-release path, plus a cross-PR coordination point around how release-driven version changes re-trigger a Running gateway.
What is good
selectGatewayImageimplements the documented precedence (release_id authoritative -> direct image -> empty for platform default) and matches every scenario ingateway-version-selection.spec.md.- Errors are wrapped with context (
fmt.Errorf("...: %w", err)), there is nopanic(), and the gRPC client is built fromr.grpcConnconsistent with the existingresolveDatabaseConfigpattern. - An unresolvable release (missing, empty image, nil payload, transient RPC error) returns an error and is retried rather than silently deploying a default/empty image, which is the correct and explicitly-specified behavior.
- The new
gateway_version_selection_test.goexercises all eight branches over a real in-process gRPCGatewayReleaseService; no pre-existing test assertions were modified, so there is no removed-guarantee concern.
Findings
[Minor] Unresolvable release_id is only visible via logs/span and an unbounded retry (reconciler.go:2110-2126, call site 1448-1451). When a release cannot be resolved, reconcileErr is returned and only flows into endSpan(reconcileErr); no gateway status/condition is written, so an operator whose release_id points at a genuinely missing release sees a gateway that never deploys with no user-facing reason. This is consistent with the sibling error paths in this function (database/oidc), so it is not a regression introduced here - but a NotFound on GetGatewayRelease is arguably terminal (retrying forever will not help until the release exists or release_id is corrected), and surfacing it as a gateway status condition would make it actionable. Related release status write-back is in flight elsewhere; a short follow-up to reflect "unresolvable release" on the gateway would close the gap. Confidence: Medium.
Cross-PR coordination
- PR #235 (GatewayRelease reconciliation). This PR resolves
release_id-> image at reconcile time, but by its own design it does not re-reconcile an already-Runninggateway when the referenced release's image changes or whenrelease_idis repointed; the spec added here delegates that re-trigger to the release fan-out delivered in #235 (EnqueueForced, which clears the phase so the provisioning gate does not drop the event). As a result the version-selection feature only produces an end-to-end version change when both land: merged alone, this PR still leaves a Running gateway on its old image after its release's image is updated. Maintainers should coordinate the merge/order of these two PRs and confirm the combined behavior, and reconcile the two adjacent additions to the sub-spec index table inspecs/platform/control-plane.spec.mdso both rows survive. - PR #151 vs PR #235 (competing re-trigger / provisioning-gate designs). This PR's "version change is applied on reconcile" requirement rests on "the provisioning gate and re-trigger mechanisms defined elsewhere," and two different such mechanisms are currently proposed: #235 keeps the phase gate and bypasses it by clearing phase, while #151 replaces the phase gate with a
generation/observed_generationconvergence gate. These are competing designs for the same gate that decides whether a Running gateway is re-reconciled after a version change, and it is not defined whether repointingrelease_idbumpsgenerationunder #151. Maintainers need to decide which mechanism owns re-triggering release-driven version changes so this PR's requirement is satisfied by exactly one path rather than two overlapping ones.
Findings Summary (ordered by severity, highest first):
- [Minor] Unresolvable
release_idhas no operator-visible status and retries unboundedly (NotFound arguably terminal) - Observability / Status-on-error (reconciler.go:2110-2126, 1448-1451)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
Proper context propagation (stream ctx, no context.TODO()) |
Pass |
| Reconcile pattern (error -> retry, no silent fallback) | Pass |
| Status updated on error paths | Partial |
| Test Diff Scrutiny (no modified pre-existing assertions) | Pass |
| Conventional commit message | Pass |
| client := pb.NewGatewayReleaseServiceClient(r.grpcConn) | ||
| resp, err := client.GetGatewayRelease(ctx, &pb.GetGatewayReleaseRequest{Id: gw.ReleaseId}) | ||
| if err != nil { | ||
| return "", fmt.Errorf("resolve GatewayRelease %s: %w", gw.ReleaseId, err) |
There was a problem hiding this comment.
[Minor] Unresolvable release surfaces only via logs/span + unbounded retry.
This error (and the nil-payload / empty-image branches below) is returned as reconcileErr, which only feeds endSpan(reconcileErr) at the call site - no gateway status/condition is written. An operator whose release_id points at a genuinely missing release sees a gateway that never deploys with no user-facing reason.
Two related points:
- A
codes.NotFoundfromGetGatewayReleaseis arguably terminal rather than transient: retrying forever will not help until the release is created orrelease_idis corrected. Consider distinguishing NotFound (terminal, write aFailed/Invalidgateway condition) from transient RPC errors (retry). - Surfacing an "unresolvable release" condition on the gateway would make this actionable.
This matches the existing sibling error paths (database/oidc), so it is not a regression - flagging as a follow-up, not a blocker. Confidence: Medium.
Resolve a Gateway's release_id to the image published by its referenced GatewayRelease at reconcile time, instead of only using a directly-specified image. release_id takes precedence over a direct image; a direct image is the fallback; and when neither is set the manifest layer applies the platform default. A release_id that cannot be resolved to a release with a non-empty image (missing release, empty image, empty payload, or a transient lookup error) fails the reconcile so it is retried, rather than silently deploying a default or empty image. Adds selectGatewayImage/resolveReleaseImage on the gateway reconciler, unit tests over an in-process gRPC GatewayReleaseService, and the behavior spec. Refs HYPERSHELL-174 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
9196614 to
f7da4f6
Compare
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 focused, well-specified, and well-tested change: it finally wires release_id -> GatewayRelease.image at reconcile time, with correct precedence over a direct image and a fail-closed (retry, never silently default) posture on unresolvable releases. The implementation cleanly mirrors the existing resolveDatabaseConfig pattern; my only substantive note is about terminal-error handling / status write-back, plus two cross-PR coordination items that need a maintainer merge-order decision.
Summary
selectGatewayImage / resolveReleaseImage implement database-backed version selection exactly as the new gateway-version-selection.spec.md describes: release precedence, direct-image fallback, empty -> platform default, and errors on unresolvable releases. Error wrapping (fmt.Errorf("...: %w", err)), context propagation, and gRPC client usage all follow established conventions, and there are no panic()s, no secret exposure, and no modified pre-existing test assertions (the tests are purely additive).
Findings
[Minor] Terminal release-resolution failures retry forever with no gateway status write-back (components/control-plane/internal/reconciler/reconciler.go L2110-2126)
A referenced release that will never resolve to a usable image - NotFound on a deleted/never-created release, or a release with an empty image - is returned as a plain reconcile error and retried indefinitely, and no gateway status is written so operators get no signal that the gateway is stuck on a bad release_id. conventions.spec.md distinguishes transient errors (retry) from terminal errors ("update resource status to Failed, do not retry") and requires status updates on error paths. This mirrors the existing resolveDatabaseConfig behavior (which is why I am scoring it Minor rather than Major, and the reconcile queue's per-key backoff blunts the hot-loop), but the empty-image case in particular is clearly terminal. Consider surfacing a Failed/Invalid gateway status for the terminal cases so the condition is observable; the transient/Unavailable path correctly stays a retry.
Cross-PR coordination
Two items need a maintainer decision on design/merge order:
-
The upstream OpenShell Helm-chart adoption change reworks exactly the layer this PR relies on to apply the "empty image -> platform default" fallback. This PR intentionally returns an empty string so the static-manifest layer substitutes
DefaultGatewayImage(); that adoption change removes the staticApplyManifestToNamespacedefault path and instead mapsGatewayConfig.Imageinto Helmimage.repository/image.tag(splitting the ref), applying no image values when it is empty and deferring to the chart default. TheGatewayConfig.Imageplumbing stays compatible, but the fallback contract asserted by this PR's spec ("Direct Image Fallback When No Release Referenced" -> platform default) moves and changes shape. The two owners should agree on where the platform-default fallback lives and validate this PR's precedence/fallback scenarios against the Helm values path, and decide merge order so the empty-selection default is not lost in the transition. -
The sibling GatewayRelease-reconciliation work is a functional dependency and shares an assumption that a maintainer should confirm. This PR's "version change through release reference is applied on reconcile" outcome depends on that work's release-image fan-out (
EnqueueForced, which clears the gateway phase) to re-reconcile already-Runninggateways when a release's image changes; on its own this PR only re-resolves when a gateway is otherwise reconciled. It also deliberately does not gate deployment on releasestatus, so a release the sibling work marksInvalidwould still be deployed here as long as itsimageis non-empty. Both halves are needed for end-to-end database-backed version selection, and the "do not gate on status" decision should be an explicit, agreed contract between the two - please coordinate merge order and confirm that assumption holds.
Findings Summary (ordered by severity, highest first)
- [Minor] Terminal release-resolution failures (missing release / empty image) retry indefinitely with no gateway status write-back - Control-Plane Conventions (L2110-2126)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / 404 handled |
Fail (intentional) - resolution failures are surfaced as retryable errors per the PR spec; terminal cases lack status write-back |
Proper context propagation (no context.TODO()) |
Pass |
| Status updated on error paths | Partial - error returned/traced, but no gateway status on terminal release failures |
| Reconcile pattern (not create-or-skip) | Pass |
| No secrets in logs or responses | Pass |
| Test Diff Scrutiny (no flipped pre-existing assertions) | Pass - tests are additive |
| Spec added and indexed | Pass |
| return "", fmt.Errorf("gateway configuration error: GatewayRelease %s returned empty payload", gw.ReleaseId) | ||
| } | ||
| if rel.Image == "" { | ||
| return "", fmt.Errorf("GatewayRelease %s has no image", gw.ReleaseId) |
There was a problem hiding this comment.
This resolution path treats every failure identically as a retryable reconcile error. A NotFound on a deleted/never-created release and a release with an empty image are terminal configuration errors that will not fix themselves without an operator changing release_id or the release, yet they are retried indefinitely and no gateway status is written to signal the stuck state. specs/standards/control-plane/conventions.spec.md asks for terminal errors to set a Failed status (and not spin) while transient errors retry. Consider distinguishing the terminal cases (missing release, empty image) with a gateway status write-back for observability; the transient/Unavailable path should stay a retry. (This mirrors the existing resolveDatabaseConfig pattern, hence Minor.)
2 participants
Summary
Resolves a Gateway's
release_idto the image published by its referencedGatewayReleaseat reconcile time (HYPERSHELL-174). Until nowrelease_idwas stored and required on a Gateway but never resolved — the control plane rendered the Deployment straight fromgw.Image(or the platform default), so theGatewayReleasetable had no effect on which image a gateway ran.This implements the resolution mandated by
data-model.spec.md("when both are set,release_idtakes precedence and the reconciler resolves it to an image").Behavior
release_idset → the reconciler resolves it viaGetGatewayReleaseand uses that release'simage. This takes precedence over any directimageon the Gateway.release_id, directimageset → uses the direct image (preserves today's behavior for pre-release gateways).Scope
In scope:
release_id→image resolution and precedence. Out of scope (sibling tasks): GatewayRelease image validation / status write-back (HYPERSHELL-173, #235), rollout/canary, and gating deployment on releasestatus(a possible future additive refinement — deliberately not gated today, since that would block all release-backed gateways until status write-back lands).Interaction with the phase gate
Version selection runs wherever a reconcile reaches image selection. Whether an already-
Runninggateway is re-reconciled after itsrelease_idis repointed is governed by the existing provisioning gate and the re-trigger mechanisms owned elsewhere (health spec + release fan-out in #235 / the convergence gate in #151). The spec's version-change requirement is scoped accordingly and does not over-claim a steady-state re-reconcile guarantee.Testing
gateway_version_selection_test.go) over an in-process gRPCGatewayReleaseService: release precedence over direct image, pure release-only resolution, direct-image fallback, empty→default, release-not-found, empty image, transient lookup error, and nil payload.go build ./...,go vet ./...,gofmt, andgo test -race ./internal/reconciler/all green.amber-reviewloop — APPROVE after scoping the version-change spec requirement and adding the pure-resolution test.Specs
specs/platform/gateway-version-selection.spec.mdand indexes it fromcontrol-plane.spec.md.Refs HYPERSHELL-174
🤖 Generated with Claude Code