fix(flux): show the failure cause on the HelmRelease details card - #2008
Conversation
helm-controller mirrors one of Released, TestSuccess or Remediated into
Ready verbatim, so a failed upgrade's error was replaced by the rollback
that remediated it ("Helm rollback to previous release ... succeeded") —
information about the previous, working release. A stalled release is
worse still: Ready keeps the "reconciliation in progress" placeholder, so
a release that failed months ago was reported as "Last reconciled".
HelmRelease.findFailureCauseCondition() returns the failing Released or
TestSuccess condition, gated on Ready being uninformative (a verbatim
mirror of the remediation or of a True Stalled, or Unknown on a stalled
release) so that a newer, unrelated blocker stays visible. The card takes
its Status, Message and failure time from there, keeps the remediation as
a single line, and names the attempted revision. The AI chat prompt and
the tree search use the same condition.
| const isStalled = stalledCondition?.status === 'True'; | ||
| const readyIsUninformative = | ||
| mirrors(this.findRemediatedCondition()) || | ||
| (isStalled && mirrors(stalledCondition)) || |
There was a problem hiding this comment.
This clause weakens the "mirror test keeps a newer blocker visible" guarantee the doc comment relies on. helm-controller reports a terminal error by writing the same reason/message into both Stalled and Ready, so consider: an upgrade fails (Released=False/UpgradeFailed), it is remediated, and the user then pushes a spec the controller rejects terminally (e.g. an invalid chart reference / terminal values error). Ready now carries that current, informative terminal message — but because it mirrors Stalled, it is classified as uninformative and the card/AI prompt/tree search all fall back to the older Released error, hiding the reason the object is stuck right now. Restricting this clause to the genuinely uninformative stall (stalledCondition.reason === 'RetriesExceeded') would keep the intended behaviour while leaving terminal errors visible.
There was a problem hiding this comment.
Agreed, fixed in 8ba0206. The clause now requires hasExhaustedRetries() — a Stalled condition whose reason is RetriesExceeded, whose message only counts attempts ("Failed to upgrade after N attempt(s)"). A terminal stall mirrored into Ready is left alone, so Ready keeps stating why the object is stuck right now. Covered by a new test (returns nothing when Ready reports a terminal stall), and the doc comment says why the mirror is restricted.
Worth noting the clause was speculative to begin with: scanning gazelle, glean and graveler, every live Stalled is RetriesExceeded, and in those objects Ready either mirrors Released or sits at Unknown — never mirrors Stalled. So restricting it costs nothing observable.
| metadata.Status = ( | ||
| <> | ||
| {phrase} <DateComponent value={cause.lastTransitionTime} relative /> | ||
| {stalledCondition?.status === 'True' ? ' (retries exhausted)' : ''} |
There was a problem hiding this comment.
Any Stalled=True is rendered as "(retries exhausted)", but helm-controller also sets Stalled for terminal errors that were never retried (invalid chart reference, terminal values error — reason is then not RetriesExceeded). In that case the Status line asserts something untrue about why the release stopped. Gating on stalledCondition.reason === 'RetriesExceeded' (falling back to no suffix, or a neutral "stopped retrying") keeps the wording honest.
There was a problem hiding this comment.
Fixed in 8ba0206: (retries exhausted) now only for hasExhaustedRetries(), (stalled) for any other stall reason. Test: does not claim the retries ran out on a terminal stall.
Note the reachable path for the neutral wording is narrow — with the sibling fix, a terminal stall mirrored into Ready no longer enters the failure branch at all, so (stalled) shows only when Ready is the Unknown placeholder on a terminally stalled release. The test pins that shape.
| resource: FluxObject, | ||
| readyStatus: FluxResourceStatus['readyStatus'], | ||
| ) { | ||
| if (resource instanceof HelmRelease) { |
There was a problem hiding this comment.
Unlike getFailureMessage in findMatchingNodes.ts, this path has no isSuspended() guard. A suspended HelmRelease keeps its conditions frozen, so a release that was suspended after a remediated/stalled failure now renders the button as "Troubleshoot with AI" with an explain-this-error prompt even though nothing is currently failing — and for the stalled/Ready=Unknown variant that is a behaviour change (it used to get the neutral "show me basic details" prompt). Skipping the failure context when resource.isSuspended() would match the tree-search reasoning.
There was a problem hiding this comment.
I would rather not add the guard here, because it changes behaviour beyond the stalled case and in the wrong direction.
readyStatus comes from getOrCalculateFluxStatus(), which reads the Ready condition regardless of spec.suspend. So on main a suspended, failing resource of any kind already gets troubleshoot and the explain-this-error prompt. An isSuspended() guard would newly suppress that for every kind — a regression relative to today, not a restoration of it.
The divergence from findMatchingNodes is deliberate: search ranges over every node at once, where a frozen stale message produces hits the user did not ask about, whereas the card is one resource the user opened, and it deliberately shows the last known failure for suspended objects (somebody who suspends a HelmRelease because it broke is exactly who needs the error). Keeping the button aligned with the card it sits under seems better than making them disagree.
That leaves the delta you identified — suspended + stalled + Ready=Unknown now offers troubleshooting instead of basic details — which I think is the right side of the trade. I have documented the reasoning next to getFailureContext rather than changing the behaviour; happy to add the guard if you would still prefer symmetry with search.
| return undefined; | ||
| } | ||
|
|
||
| if (resource instanceof HelmRelease) { |
There was a problem hiding this comment.
This branch is unreachable for exactly the case the PR reproduces live: a stalled release has Ready = Unknown/Progressing, so the readyCondition?.status !== 'False' early return above fires and findFailureCauseCondition() is never consulted — the failure message stays unsearchable. The card then says "Upgrade failed … (retries exhausted)" and shows the controller error, but searching the tree for any word of that error finds nothing, even though card, AI prompt and search are meant to use the same condition. Consider consulting the failure cause first for HelmReleases and keeping the Ready-must-be-False gate only for the other kinds.
There was a problem hiding this comment.
Good catch — that was a real bug, not just dead code: the branch never ran for the stalled release the PR reproduces live, so the card showed the error while searching for any word of it found nothing. Fixed in 8ba0206 by consulting findFailureCauseCondition() before the Ready-must-be-False gate, which still applies to the other kinds. The selector does its own gating and only ever returns a currently-failing condition, so nothing stale becomes searchable. The suspended-resource guard stays first.
New test matches a stalled HelmRelease, whose Ready condition is not False fails without the reorder. I could not reproduce it end to end in the app: both stalled HelmReleases on gazelle sit outside the tree this view builds, so searching their names returns 0 hits either way. The Ready=False path is unaffected — searching "does not have an artifact" in the gazelle tree still finds appcatalog-giantswarm-operations-platform-test (1/1).
Addresses review feedback: - Only a `RetriesExceeded` stall counts as an uninformative Ready mirror. helm-controller also stalls on terminal errors it never retried and mirrors those into Ready, where they state the current reason the object is stuck; substituting an older release error would hide them. - The Status line therefore says "(retries exhausted)" only for that reason, and "(stalled)" otherwise, instead of asserting a retry count that never happened. - The tree search consults the failure cause before the Ready-status gate. A stalled release keeps Ready at Unknown, so the branch was unreachable for exactly the case the card reports: its error was unsearchable. `isStalled()`/`hasExhaustedRetries()` keep the reason string in the resource class rather than exporting it — the k8s index re-exports named symbols only, so an exported constant read as undefined in flux-react and rendered every stall as "(stalled)".
1 participant
What does this PR do?
The HelmRelease details card in the Flux UI derived its Status and Message rows
from the
Readycondition alone. helm-controller summarizes a release bymirroring one of
Released,TestSuccessorRemediatedintoReadyverbatim, so a failed upgrade showed its real error only until the failure was
remediated: the rollback then overwrote
Ready, and the card switched toinformation about the previous, working release.
HelmRelease.findFailureCauseCondition()returns the failingReleased(orTestSuccess) condition, which keeps the error until the next release attempt.Where several are failing, the most recent transition wins — an upgrade that
succeeded with failing Helm tests leaves
Releasedtrue andTestSuccessfalse, and a failed upgrade can leave a stale failing
TestSuccessbehind.Substitution requires
Readyto be uninformative: a verbatim mirror ofRemediatedor of aTrueStalled, orUnknownon a stalled release.Testing for a mirror rather than for an allow-list of remediation reasons is what
keeps a newer, unrelated blocker visible — when an object fails again after the
rollback for a reason that never reaches a release attempt (
ArtifactFailed,DependencyNotReady, a missingvaluesFromSecret, a denied release),Readycarries that current blocker, no longer equals the remediation, and the older —
now misleading — error is not shown. The mirror test also covers remediation
flavours a reason list would miss: a failed install is remediated by an
uninstall, and the rollback itself can fail (
Remediatedis thenFalse, andstill mirrored into
Ready).Nothing is substituted while the release is ready or a fresh reconciliation is in
flight:
spec.test.ignoreFailureskeeps a release ready withTestSuccessfailing, and a progressing object can carry a stale failing
Releasedfrom theprevious generation.
What is the effect of this change to users?
For a failed release, the card now shows the error and the time the release
actually failed, plus an
Attemptedrow naming the revision it tried (so therunning
Chart Versionand the version the error talks about can be told apart)and a one-line
Remediationrow — expanded to the full message only when theremediation itself failed. The AI chat button and the resource tree's search over
failure messages use the same condition, so neither asks about nor matches on the
rollback message any more, and the button offers to troubleshoot whenever a
release failure is known rather than only when
ReadyisFalse.Note that the long controller error now wraps over several lines, so a failing
card is visibly taller than before. Status badges, card colours and tree failure
propagation are untouched.
How does it look like?
Verified against live objects on
gazelle.org-giantswarm-production/operations-hello-world-test— a stalled release(
Stalled=True/RetriesExceeded,Ready=Unknown/Progressing,Released=False/UpgradeFailed):vingress.elbv2.k8s.awsdenied the request: invalid ingress class: IngressClassnginxnot foundflux-giantswarm/appcatalog-giantswarm-operations-platform-test(
Ready=False/SourceNotReady) is unchanged, i.e. the fallback still applies:buildStatusMetadatais untouched, so the seven other Flux kinds the cardrenders cannot regress.
Any background context you can provide?
Reported from a live
HelmReleasewhose upgrade failed on a CRD validationerror: the informative message appeared briefly and was then replaced by the
rollback's. The full status of that object is the basis of the test fixtures.
No currently-failing HelmRelease on
gazelle,gleanorgraveleris in therollback-masked state right now, so that path is covered by unit tests; the
stalled variant above is the live reproduction.
Do the docs need to be updated?
No.
Should this change be mentioned in the release notes?