[HYPERSHELL-178] Standardize gateway health and readiness vocabulary by JuanmaBM · Pull Request #239 · openshift-online/hypershell

JuanmaBM · 2026-09-03T11:40:03Z

Summary

Standardizes the Gateway phase/status vocabulary into a single source of truth so the phase a client reads means the same thing in every component and cannot silently drift. No schema change — the health behavior (deployment readiness, route readiness, continuous health loop) already exists; this PR standardizes its representation.

Before this change the phase strings were duplicated as magic literals across the control plane, the API-server metric, and the console, with real drift (the metric omitted Pending; the console had lowercase/extra variants).

What changed

New shared package components/api-server/pkg/gatewayhealth — Phase constants (Pending/Provisioning/Running/Degraded/Failed), Phases()/PhaseStrings()/IsValidPhase(), and the canonical StatusHealthy constant. Imported by both the API server and the control plane (via the existing replace ../api-server directive).
API server validation — writes that set a gateway phase outside the canonical set are rejected (gRPC create/update → InvalidArgument; REST create/patch → 400). An absent/empty phase stays accepted, so the field remains optional and pre-existing rows are not locked out (a patch that doesn't touch phase skips validation).
Metrics — hypershell_gateways_total now derives its phase set from gatewayhealth.PhaseStrings(), fixing the prior omission of Pending.
Control plane — reconciler, health loop, and the forceSeedRecovery phase-read gate now use the shared constants instead of magic strings; the healthy case uses StatusHealthy.
Console — gateway-data.ts gains an exported gatewayCanonicalPhases mirroring the Go vocabulary; the polling/failed sets derive from it (behavior-preserving).
Spec — new specs/platform/gateway-phase-vocabulary.spec.md, cross-linked from openshell-gateway-health.spec.md.

Testing

pkg/gatewayhealth unit tests (valid/invalid/case-sensitivity, canonical order, defensive copy).
New phase_validation_test.go integration tests: invalid phase rejected on gRPC create/update + REST create; canonical phase accepted; absent phase accepted; and a legacy-record test proving a non-canonical stored phase can still be patched on unrelated fields (no lockout).
Full plugins/gateways integration suite, control-plane watcher/reconciler tests, go build/go vet, and repo make check all pass.
Reviewed via the project's amber-review skill (verdict: APPROVE after resolving the control-plane phase-gate migration).

HYPERSHELL-178

🤖 Generated with Claude Code

Introduce a single source of truth for the Gateway phase/status vocabulary so the phase a client reads means the same thing in every component and cannot silently drift. No schema change. - Add shared pkg/gatewayhealth (Phase constants, Phases/PhaseStrings/ IsValidPhase, StatusHealthy), imported by both the API server and the control plane. - API server now rejects writes that set a gateway phase outside the canonical set (gRPC create/update -> InvalidArgument; REST create/patch -> 400); absent/empty phase stays accepted. - Derive the per-phase metric set from gatewayhealth (fixes the prior omission of Pending). - Replace magic-string phase literals (reconciler, health loop, and the watcher forceSeedRecovery gate) and the "Healthy" status with the shared constants. - Align the console phase vocabulary (gateway-data.ts) to a canonical set mirroring the Go package (behavior-preserving). - Add gateway-phase-vocabulary.spec.md and cross-link it from the health spec. HYPERSHELL-178 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

coderabbitai · 2026-09-03T11:40:11Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Team

Run ID: 64bd72ec-2f4d-4294-8fc6-a12dfeede48e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

_{Comment @coderabbitai help to get the list of available commands.}

jsell-rh · 2026-09-03T11:41:05Z

Amber review

Status: Complete

View the submitted review.

jsell-rh

Verdict

This is a clean, well-scoped refactor that collapses duplicated Gateway phase/status string literals into a single shared gatewayhealth package, adds optional-but-canonical phase validation on the API-server write paths, and derives the per-phase metric (now including the previously-missing Pending) from that source of truth. The change is behavior-preserving, thoroughly tested (including a legacy-record test that proves pre-existing non-canonical rows are not locked out), and the accompanying spec is precise; I have only minor observations.

Summary

The PR introduces components/api-server/pkg/gatewayhealth (phase constants, Phases()/PhaseStrings()/IsValidPhase(), StatusHealthy) and wires it into the API server (validation + metric), the control plane (reconciler, health loop, watcher gate), and the console (gatewayCanonicalPhases). Validation is correctly gated so an absent/empty phase stays accepted, keeping the field optional and existing rows patchable on unrelated fields.

Strengths

Genuine single source of truth. Both Go modules import the same package via the existing replace ../api-server directive, and the console mirrors it with a documented "keep in sync" note. The metric and validation now derive from one list, so the earlier Pending omission cannot recur.
Migration safety is explicitly handled. validateGatewayPhaseValue/validateGatewayPhase only fire when a write sets phase, and empty is accepted. TestGatewayPatchNotTouchingPhaseAcceptsLegacyRecord proves a stored non-canonical phase does not lock a row out of unrelated patches.
Test diff scrutiny. The pre-existing assertions in grpc_integration_test.go and integration_test.go that flip "TestPhase"/"UpdatedPhase"/"test-phase" to canonical values are a tightened-shared-precondition signal, but the PR calls this out and backs it with the legacy-record + rejection tests rather than silently rewriting them. No guarantee was removed.

Minor observations

[Minor] metrics.go Collect() emits only canonical phases, so any gateway persisting a legacy/non-canonical or blank phase (a state this PR intentionally still permits at the service layer) is not counted in any series and the gauge can under-report the true total. Pre-existing behavior, but now that the vocabulary is canonicalized an unknown/other bucket would make the drift visible. Observability.
[Minor] In grpc_handler.go UpdateGateway, validateGatewayPhase(req.Phase) runs immediately after grpcutil.ValidateStringField("phase", *req.Phase, false) inside the same req.Phase != nil block; the nil/empty guard inside validateGatewayPhase is redundant on this path. Harmless, purely cosmetic. Style.

Cross-PR coordination

Two open pull requests require maintainer coordination with this change:

#211 introduces a console gateway-metrics dashboard that hardcodes its own phase set (GatewayPhaseCounts / gatewayPhases = Running, Provisioning, Degraded, Failed) inside packages/gateway-management-ui/src/metrics/, the same package this PR designates as the console-side source of truth. That set omits Pending, which this PR deliberately adds to the API-server metric and mandates as part of the canonical vocabulary. If both merge as-is, the dashboard will silently drop the Pending gauge series and reintroduce the duplicated, drifting phase list this PR is removing. A decision is needed on having #211 consume the canonical vocabulary (including Pending) rather than its own literal set, and on merge order so the later PR conforms.
#151 and #200 propose re-keying the control-plane provisioning gate off desired-state convergence and updating the Gateway health spec so a healthy phase no longer suppresses drift repair - directly targeting the phase gate (skip if Running/Provisioning/Degraded) and the health spec that this PR refactors into shared constants (and cross-links). The behavior this PR preserves is the exact behavior those PRs intend to replace. Maintainers should decide the gate model and the merge order; whichever lands second must rebase the gate/spec onto the agreed design rather than re-entrenching or re-removing it independently.

Findings Summary (ordered by severity, highest first)

[Minor] Per-phase gauge omits gateways holding a legacy/non-canonical/blank phase - Observability (metrics.go L82-L84)
[Minor] Redundant empty/nil phase guard on the gRPC update path - Style (grpc_handler.go L140-L143)

Convention Checklist

Convention	Result
No `panic()` in production code	Pass
Errors wrapped with `fmt.Errorf`/typed errors context	Pass
Input validated (canonical phase vocabulary)	Pass
No secrets in logs or responses	Pass
Reconcile pattern preserved	Pass
Image references consistent	N/A
Single source of truth for shared vocabulary	Pass
Test Diff Scrutiny (modified pre-existing assertions justified)	Pass
Conventional commit message	Pass
Spec added and cross-linked	Pass

jsell-rh · 2026-09-03T11:44:52Z

-	for phase := range known {
+	// Always emit the canonical phases so graphs never have gaps.
+	for _, phase := range gatewayhealth.PhaseStrings() {
 		ch <- prometheus.MustNewConstMetric(c.desc, prometheus.GaugeValue, float64(counts[phase]), phase)


[Minor - Observability] Collect() iterates only the canonical PhaseStrings(), so any gateway whose stored phase is legacy/non-canonical or blank (a state this PR intentionally still allows at the service layer, and which the legacy-record test relies on) is not represented in any series. The gauge can therefore under-report the true gateway total. Consider emitting an unknown/other bucket for counts whose phase is not in the canonical set, so drift stays visible rather than silently disappearing. Pre-existing behavior, so not blocking.

jsell-rh · 2026-09-03T11:44:52Z

 		if err := grpcutil.ValidateStringField("phase", *req.Phase, false); err != nil {
 			return nil, err
 		}
+		if err := validateGatewayPhase(req.Phase); err != nil {


[Minor - Style] This runs inside the if req.Phase != nil block, immediately after ValidateStringField("phase", *req.Phase, false), so the phase == nil guard inside validateGatewayPhase is redundant on this path. Harmless - just noting it; the create path (L68) legitimately needs the nil guard, so the shared helper is fine as-is.

jsell-rh reviewed Sep 3, 2026

View reviewed changes

This was referenced Sep 3, 2026

feat(web-console): add operational dashboard with live metrics #241

Open

feat(observability): measure gateway provision duration #243

Merged

Navigation Menu

Uh oh!

[HYPERSHELL-178] Standardize gateway health and readiness vocabulary - #239

[HYPERSHELL-178] Standardize gateway health and readiness vocabulary#239
JuanmaBM wants to merge 1 commit into
openshift-online:mainfrom
JuanmaBM:HYPERSHELL-178-standardize-gateway-health-readiness-conditions

JuanmaBM commented Sep 3, 2026

Uh oh!

coderabbitai Bot commented Sep 3, 2026

Review skipped

Uh oh!

jsell-rh commented Sep 3, 2026 •

edited

Loading

Uh oh!

jsell-rh left a comment

Uh oh!

jsell-rh Sep 3, 2026

Uh oh!

jsell-rh Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

JuanmaBM commented Sep 3, 2026

Summary

What changed

Testing

Uh oh!

coderabbitai Bot commented Sep 3, 2026

Review skipped

Uh oh!

jsell-rh commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Amber review

Uh oh!

jsell-rh left a comment

Choose a reason for hiding this comment

Verdict

Summary

Strengths

Minor observations

Cross-PR coordination

Findings Summary (ordered by severity, highest first)

Convention Checklist

Uh oh!

jsell-rh Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

jsell-rh Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jsell-rh commented Sep 3, 2026 •

edited

Loading