Repository navigation
fix(bridge): report real 1P helm release health and verify release on cold-start hydration - #990
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements first-party (1P) component health evaluation and release verification during cold-start state hydration. It introduces the ComponentHealth method to the HelmManager interface, enabling the reconciler to determine health based on actual Helm release status, and updates HydrateComponentState to verify that a Ready=True CR has an actively deployed release. A critical compilation error was identified in helm_manager.go because release.Status does not have an IsPending() method. Additionally, defensive nil-checks are recommended for m.healthVerifiedAt to prevent runtime panics on zero-initialized managers.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust health checking mechanism for first-party (1P) Helm-managed components. It adds a ComponentHealth method to the HelmManager interface and implements it in K8sHelmManager to report the real-time status of Helm releases (such as deployed, pending, failed, or stuck). The reconciler is updated to evaluate 1P component health using this new mechanism, caching successful verdicts to throttle API calls, and handling transient status errors gracefully. Additionally, cold-start state hydration now verifies the actual release status before trusting a Ready=True custom resource status, forcing a re-actuation if the release is missing or failed. There are no review comments, so no feedback is provided.
b64ea29 to
e8168c8
Compare
e8168c8 to
3c3d699
Compare
d1049d8 to
33ca576
Compare
… cold-start hydration 1P components were marked Ready=True/ENABLED unconditionally because BridgeProber.ProbeHealth returns healthy for every non-3P type, even while the async Helm install failed on every retry. On cold start, that Ready=True condition plus the pre-persisted values ConfigMap made HydrateComponentState record the component as actuated, so a component with no Helm release was never installed until its generation changed. - Add HelmActionRunner.Status (action.NewStatus) and extract the shared action-config initialization into newActionConfig. - Add HelmManager.ComponentHealth: reports HelmUpgradeFailed, HelmRunnerUninitialized, HelmReleaseNotFound, HelmStatusUnavailable, HelmReleaseNotReady, or ComponentHealthy from the real release state. - evaluateHealth uses ComponentHealth for 1P components when a HelmManager is configured; the prober remains the fallback otherwise. - HydrateComponentState verifies the release is deployed (outside m.mu) before trusting a Ready CR as proof of actuation; the deactivation path and the deployed-release no-revision-bump guarantee are unchanged. BUG=b/548435582 TAG=agy
Address review findings on 1P health and cold-start hydration: - Track async upgrades per component (sequence-guarded) so ComponentHealth reports HelmUpgradePending (Ready=False, Degraded=False) instead of the previously deployed revision while an upgrade is queued or running. - Keep previous Ready/Degraded conditions on transient Helm status errors (HelmStatusUnavailable) and log a warning. - Hydration distrusts the CR only on ErrReleaseNotFound or a terminal non-deployed status; transient errors and pending-* releases trust it. Pending releases without an in-flight upgrade surface as HelmReleaseStuck (Degraded=True) with documented manual recovery. - Cache healthy verdicts for 1 minute, invalidated on upgrade enqueue and completion, to throttle Helm Status calls. - Classify not-found strictly via errors.Is(driver.ErrReleaseNotFound). - Export condition reason constants from reconciler.go and replace literals. - README: document 'deployed' as manifests applied (no --wait), new reasons, cache, and stuck-release recovery. BUG=b/548435582 TAG=agy
Initialize healthVerifiedAt on first write, matching upgradeErrs and pendingUpgrades, so a manager not built via NewK8sHelmManager cannot panic. BUG=b/548435582 TAG=agy CONV=baca6192-74d3-4579-9f73-0a3263d9a2a7
33ca576 to
2279dc4
Compare
preserveHealthConditions now replaces or appends via a shared upsertCondition helper (also used by setCondition), so it cannot emit duplicate Ready/Degraded entries if res.Conditions is pre-populated. Comment link: #990 (comment) BUG=b/548435582 TAG=agy CONV=baca6192-74d3-4579-9f73-0a3263d9a2a7
Summary
xmc-bridge now reports the real health of 1P components from their Helm release, instead of always reporting "healthy". On a cold start it also checks that the release exists before trusting a stale
Readycondition.Stack 2/4: CMS dependencies (0) → this PR → dependency-gated actuation (C1) → dependency-gated deactivation (C2).
Motivation & Context
Found on a live cluster:
BridgeProber.ProbeHealthreturned healthy for every 1P component. A Component showedReady=Truewhile its asynchelm installfailed every 5s.Ready=Trueat generation N as "actuated" even though no Helm release existed, so the workload was never installed until the generation changed.Key Changes
HelmActionRunner.Status(action.NewStatus). Action-config setup is shared innewActionConfig.HelmManager.ComponentHealthchecks, in order:HelmUpgradeFailedHelmUpgradePendingHelmReleaseNotFoundpending-*HelmReleaseStuckdeployedHelmReleaseNotReadyHelmStatusUnavailabledeployedComponentHealthyHealthy results are cached for 1 minute, and the cache is invalidated on every enqueue or error.
In-flight upgrade tracking: a pending mark is set on enqueue and cleared by the worker, with a per-request sequence number. This prevents a false
Ready=Truewhile the previous revision still readsdeployed.evaluateHealthusesComponentHealthfor 1P components. A transientHelmStatusUnavailablekeeps the previous conditions instead of flapping.Hydration distrusts the CR only on
ErrReleaseNotFoundor a terminal non-deployed status. Transient errors andpending-*keep trusting the CR, which avoids needless revision bumps.Reason constants are exported next to the
Condition*constants. The README documents the health semantics, cache, recovery from stuck releases, and thatdeployedmeans manifests applied (no--wait), not pod readiness.Assumptions
pending-*releases (manualhelm rollback/uninstallis documented). — Helm records no lock owner, so an automatic rollback could clobber another actor's operation. — Operators must intervene when a bridge crashes mid-upgrade.Verification & Testing
bazel test //pkg/components/... //cmd/xmc_bridge/...(9/9 pass),./.githooks/pre-commit(pass). Size vs parent: 287 prod / 893 total.ComponentHealthtable test;K8sHelmManagerin the reconciler test;Statusagainst an httptest API server with the memory driver;...): after an out-of-bandhelm uninstall xmc-cmp-ingestion-gateway, the Component reportedReady=False/HelmReleaseNotFound(previously a falseReady=True). Afterkubectl rollout restartof the bridge, hydration detected the missing release and the bridge reinstalled it; the Component becameENABLED/Ready=True.Related Issues / Bugs
Found while working on bug 548435582.
TAG=agy
CONV=baca6192-74d3-4579-9f73-0a3263d9a2a7