[garciadias]

helm upgrade exiting non-zero on a trust deploy has never meant one thing. Most of the time it's the xnat-init post-upgrade hook timing out (#565) on a release that deployed perfectly well; occasionally it's a real failure where nothing reached the cluster. deploy-trust-k8s admitted as much in a comment — "often a non-fatal post-install hook (e.g. xnat-init — see #565), but it may be a genuine Helm failure" — and re-exited the raw code, leaving a human to read the output and judge. The auto-upgrade agent in #1640 can't read output, so the deploy now judges itself.

What changed

make deploy classifies the helm exit from cluster evidence and returns one of three codes:

Exit	Meaning
0	The upgrade landed — helm exited 0, or its wait expired but the new revision is live, every workload rolled out and xnat-init completed
1	A real failure — the revision never advanced (the cluster is still on the old spec), a workload did not roll out, or the evidence was unreadable
3	The release is live and rolled out, but xnat-init did not complete

3 is still a failure as far as make is concerned. It just says something different from 1, so an unattended caller can report "upgraded, init incomplete" and point the operator at make xnat-init instead of guessing, and a human gets the same distinction without reading a log.

The calls worth arguing with

The verdict does not trust Ready. That's the specific blindness that let #1228 hide for five consecutive upgrades: the chart was correct, the new spec never reached the running pod, and the old pods were Ready throughout. So a workload counts as rolled out only when its controller has observed the new generation (observedGeneration >= generation) and every replica is both updated and ready.

An unadvanced revision is always a failure. Helm applies manifests before it runs hooks, so if the release revision didn't move, nothing was deployed — whatever else the cluster says.

Missing evidence fails closed. An unreadable release or an unreadable workload list is reported as 1, never as a quiet pass. The same reasoning scopes the hook query with app.kubernetes.io/instance so another release's xnat-init in the same namespace can't answer for this one.

The classification sits in deploy, not in deploy-trust-k8s, so upgrade-trust-k8s and every other caller inherit it rather than each growing its own interpretation. deploy-trust-k8s still patches the kit secrets before re-exiting, so #595 holds on a failure too.

Also fixes a real bug found on the way: the revision probe called helm status without --namespace, which reads the default namespace, finds no release, and makes every upgrade look like a first install.

Testing

decide() takes the evidence as a dict, so the whole decision is testable with no cluster — 36 new tests, and the chart suite is 456 green. I mutation-checked the three guards that carry the weight: dropping the generation check (4 failures), failing open on unreadable evidence (3 failures), and unwiring the Makefile (1 failure).

What I can't test here is a real cluster: the parsing of kubectl get -o json for Deployment/StatefulSet/DaemonSet status fields is covered by fixtures, not by a live rollout. That's the part worth a second pair of eyes.

Closes #1645. Unblocks #1644.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

[atriaybagur]

Thanks for this, @garciadias. You've pinned the problem down well: a helm upgrade exit that means two different things is something #1640 can't work with. The pieces of this PR are good too: decide() takes plain evidence and is easy to test, the check doesn't trust Ready on its own (the right lesson from #1228), the job query is limited to this release, and the classification sits in deploy so every caller gets it. Patching the secrets on failure (#595) is kept as well.

I'm requesting changes for two reasons. First, the verdict can report exit 0 on upgrades that didn't land. Second, I think there's a much smaller design that removes the ambiguity instead of classifying it after the fact. Detail inline. Summary below.

1. Fails open: "revision advanced" doesn't mean "spec applied"

Helm creates the new revision before it applies the manifests (pkg/action/upgrade.go v3.16.4: Releases.Create(upgradedRelease) at L376, KubeClient.Update at L429). If the API server rejects the apply (an immutable StatefulSet field, an admission webhook, a quota), revision N+1 is kept and only marked failed, and no hook runs. In that state:

• the workloads are unchanged, so they all count as rolled out;
• the previous xnat-init job was deleted by hook-succeeded, and gather() reads "no job" as "it completed";

so the verdict is 0 converged-after-wait.

The same result comes from the other two post-upgrade hooks. xnat-init has weight 2, omop-vocab-load 5 and trust-seed 6. If xnat-init succeeds (and is deleted) and then the vocab load or the seed fails or times out, the verdict is also exit 0.

A third case: if the pre-upgrade helm status probe can't read the cluster, rev="" makes revision_before=None, and that skips the "revision didn't advance" check entirely.

Repro with this PR's own gather() + decide() (only the cluster calls are faked)

import json, deploy_verdict as dv
deploy = {"kind": "Deployment", "metadata": {"name": "trust-api", "generation": 3},
          "spec": {"replicas": 1}, "status": {"observedGeneration": 3, "updatedReplicas": 1, "readyReplicas": 1}}
answers = {"status": json.dumps({"version": 8, "info": {"status": "failed"}}),
           "deploy,statefulset,daemonset": json.dumps({"items": [deploy]}),
           "job": json.dumps({"items": []})}
dv._run = lambda args: next((True, o) for k, o in answers.items() if k in args)
print(dv.decide(dv.gather(["helm"], ["kubectl"], "flip-trust", helm_rc=1, revision_before=7)))
# -> Verdict(code=0, reason='converged-after-wait', ...)

answers["status"] = json.dumps({"version": 7, "info": {"status": "deployed"}})   # revision did NOT move
print(dv.decide(dv.gather(["helm"], ["kubectl"], "flip-trust", helm_rc=1, revision_before=None)))
# -> Verdict(code=0, reason='converged-after-wait', ...)   (probe blip)

The tests don't catch these because they feed decide() hand-written evidence that makes the same assumption. There's no case for "revision advanced, status failed, no hook ran".

2. It rebuilds what Helm already has

• Rollout. deploy runs helm upgrade without --wait, so Helm only waits on hooks (Helm 4's default is hookOnly), and the PR re-implements the rollout check in Python. Helm's own checker (pkg/kube/ready.go) already compares observedGeneration with generation for Deployments, StatefulSets and DaemonSets. For Deployments it also checks the new ReplicaSet, and for StatefulSets currentRevision == updateRevision. That's stricter than _rolled_out, which accepts a Deployment where ready == desired still counts a surge pod from the old ReplicaSet. There's also an asymmetry: the rollout check only runs when helm failed. On helm_rc == 0 the verdict passes straight away, and without --wait exit 0 doesn't prove the pods rolled out. So a #1228-style "new spec never reached the pod" upgrade still exits 0.
• What happened. Helm already records, per revision, info.status, info.description and every hook's last_run.phase in helm status -o json, which this script already calls for version. That's the native evidence for "manifests applied, which hook failed", and it replaces both the revision comparison and the "no job means it completed" inference.

Suggested approach: separate the steps so each exit means one thing

#565 exists because xnat-init runs as a Helm hook inside the same timeout as everything else. Taking it out of Helm's path removes the ambiguity at the source:

deploy: preflight
	…namespace + migrate-strategy as today…
	$(HELM) upgrade --install $(RELEASE_NAME) $(CHART_DIR) --namespace $(NAMESPACE) \
		--wait --timeout $(HELM_TIMEOUT) $(VALUES_OPTS) --set xnat.initJob.enabled=false
	@$(MAKE) xnat-init || { echo "⚠️  release live, xnat-init incomplete (#565)"; exit 3; }

• Helm fails → exit 1. The apply was rejected, a workload didn't roll out, or a vocab/seed hook failed. Helm's own waiter judges rollout, on success and on failure.
• make xnat-init fails → exit 3. The release is live and rolled out, and only XNAT's configuration step failed.
• The xnat-init target already does this job. It renders the job with --set xnat.initJob.enabled=true, deletes a stale job and waits on it, and its help text already says "use … when initJob.enabled=false".
• The hook stays the chart default, so a plain helm install still gets it. Only the Makefile path turns it off.
• Optional extra: when helm fails, helm status -o json | jq '.hooks[] | select(.last_run.phase != "Succeeded")' could name the failing hook for the operator.

	This PR	This PR, verdict based on Helm's record + --wait	Split step
Size	~310 lines of Python + ~400 lines of tests	~50 lines	~10 Makefile lines + a wiring test
Apply rejected (immutable field, webhook)	0 ❌	1 ✅	1 ✅
vocab-load / seed hook fails	0 ❌	1 / 3 ✅	1 ✅
Pre-upgrade probe can't read the cluster	0 ❌	✅ (no probe needed)	✅ (no probe)
helm exits 0 but pods never rolled out	0 ❌	1 ✅	1 ✅
Who judges rollout	our Python	Helm	Helm

Two things to check before switching. I haven't run either on a cluster.

1. With --wait, every workload must become Ready within HELM_TIMEOUT. Anything that legitimately sits Pending, such as a GPU pod waiting for node scale-up (#1428) or an FL client with no kit, would fail the deploy. This PR would also report those as rollout-incomplete (exit 1), so the outcome is the same, but it now shows up on every deploy.
2. xnat-init moves from first among the hooks to after all of them (it's weight 2 today, before vocab-load and seed). Neither looks like it depends on XNAT, but the kind E2E job should confirm nothing relies on the old order.

If either is a problem, the middle column is the fallback: keep this structure, but base the verdict on Helm's own record and add --wait.

Smaller points (inline)

• revision_after == 0 can never be true, because Helm revisions start at 1.
• The script calls bare helm / kubectl instead of the Makefile's $(HELM_BIN) / $(KUBECTL). That will matter for the Helm 4 work (#1525).
• The exit-3 message says to re-run xnat-init. When that hook fails, Helm also skips the vocab-load and seed hooks, so make xnat-init alone doesn't finish the upgrade.
• The new @ at the start of the recipe line stops the helm upgrade command from being echoed.
• The PR body calls the missing --namespace "a real bug found on the way", but that probe is new in this PR. develop's deploy has no probe.

[garciadias]

Pushed 04ebc0d, which takes the other route rather than patching the classifier.

The verdict script was a second opinion on something helm had already decided, re-derived after the fact from the release revision and pod counts. Every failure mode raised here traces back to that: the revision check cannot distinguish "apply rejected" from "revision moved anyway", and unreadable evidence has to guess which way to fail.

xnat-init is the only hook that made a failed deploy ambiguous (#565), so deploy now disables it for the upgrade and runs the existing xnat-init target as a separate step after. Helm runs with --wait, so helm judges apply and rollout on both paths — its waiter already requires observedGeneration to have caught up and every replica to be updated as well as ready, so the FLIP#1228 case (five upgrades that never reached the running pod while the old pods stayed Ready) fails the upgrade instead of passing it, and the check is helm's rather than a reimplementation.

Exit codes now mean one thing each by construction: 1 helm failed, 3 the release is live but XNAT config did not finish, 0 all landed. deploy_verdict.py is deleted. On the failure path scripts/failing_hooks.py names the hooks that did not succeed, as diagnostics that cannot change the exit code.

The hook annotation is unchanged, so a plain helm install still configures XNAT; only the Makefile opts out.

One thing worth flagging that came out of this: make -n deploy was deploying for real, because GNU make executes any recipe line containing $(MAKE) even under -n. The recursive calls go through a SUBMAKE variable now, and the tests read make -n output, so that stays honest.

545 chart tests pass, helm lint clean, and the four guards were mutation-checked (drop --wait, drop the hook disable, collapse exit 3 into 1, treat a skipped hook as succeeded — each fails exactly one test).

[atriaybagur]

Thanks @garciadias, this is the right shape. The verdict script is gone, Helm judges apply and rollout, and each exit code means one thing. Catching that make -n deploy would deploy for real was a good find too. I checked the hook-order concern from my last review: neither trust-seed nor omop-vocab-load references XNAT, so running xnat-init after them is fine.

What's left is small. Details inline where the lines are in the diff:

1. The README's exit-1 row says "The trust is on its previous release". That's only true when the apply was rejected. A rollout timeout under --wait, or a failed omop-vocab-load / trust-seed hook, happens after the new spec is live.
2. make deploy with xnat.enabled=false now exits 3. The chart renders no xnat-init job then, so the target's kubectl apply fails with error: no objects passed to apply (I checked both locally). The step should be skipped when XNAT is off.
3. --wait now fails a deploy on any workload that legitimately stays Pending, such as a GPU pod waiting for a node or an FL client with no staged kit. A sentence in the README would cover it.
4. Docs outside the diff that still describe the hook:
   • the root AGENTS.md (the trust/deploy/helm entry) says HELM_TIMEOUT covers "the xnat-init Helm hook in deploy". It now bounds the --wait rollout, each remaining hook and the separate xnat-init wait;
   • docs/source/troubleshooting/trust-node-kubernetes.rst:241 has the symptom "Deploy fails with resource Job/…xnat-init not ready", but a slow init now shows up as a kubectl wait timeout from the xnat-init step, with exit 3.
5. The PR body still describes deploy_verdict.py, decide() and the 36 tests. Your comment explains the switch, but the body is what gets merged.
6. CI: ruff format --check fails on tests/test_deploy_split.py. The subprocess.run(...) in test_unreadable_input_is_silent_rather_than_wrong should be on one line, so ruff format on the file fixes it. The docs failure was a Hugging Face 499 flake; I re-ran it and it's green.
7. Optional: SUBMAKE is only needed because the failing-hooks call shares a line with helm upgrade (see the inline note).

CI doesn't run this path: kind E2E uses helm install directly. So before merging, it would be worth one real make deploy on a throwaway cluster showing exit 0, then exit 3 with xnat-init forced to fail.

[atriaybagur]

Optional: SUBMAKE is only needed because the failing-hooks sub-make shares a line with helm upgrade. If the || { … } branch ran the pipe inline ($(HELM) status … -o json 2>/dev/null | python3 $(CHART_DIR)/scripts/failing_hooks.py || true; exit 1), the xnat-init call could stay plain $(MAKE) on its own line. That's safe under -n, because the sub-make inherits -n and the line does nothing else. The variable, and the comment explaining it, would go away.

[garciadias]

Left as it is, though I take the point. xnat-init is now three sub-make calls rather than one, and each needs a distinct exit code (1 for helm, 1 for the rollout, 3 for init), so the handling cannot move out of the line without $? being lost across recipe shells. That makes the inline pipe you suggest safe only for the failing-hooks branch. Happy to do it if you would rather, but it looked like churn on a line that already has a comment explaining why it is spelled this way.

[garciadias]

Both of the risks you flagged as untested are now tested against a live cluster. One of them changed the design, so this is longer than a "done" — the short version is that your rewrite is in, deploy_verdict.py is deleted, and --wait had to be replaced rather than kept.

Hook ordering: you were right, no issue. I built a chart mirroring the real weights (xnat-init 2, omop-vocab-load 5, trust-seed 6) and upgraded with the weight-2 hook disabled, exactly as the new deploy path does. Both later hooks ran and recorded Succeeded. Neither template references XNAT at all, so there was no coupling to break.

GPU pods under --wait: this one bit. A single Pending pod fails the whole upgrade — UPGRADE FAILED: context deadline exceeded, exit 1, release left in failed state, while every other workload was healthy and Running. Helm 3 has no per-resource exclusion for --wait; it is all-or-nothing.

That matters more than "the deploy is slower and louder", because two supported configurations sit Pending by design:

• an flClient on a scale-from-zero GPU node pool — which is what values-aks.yaml recommends, since the whole point is not billing an idle GPU between FL jobs;
• an flClient deployed before its kit is staged.

For an operator at a keyboard that is merely annoying. For the unattended agent in #1640, which is the consumer this exit code exists for, it means every scheduled upgrade at such a site fails and asks for a human — the opposite of the point.

So I kept your structure and bounded what the wait covers. scripts/rollout_wait.py waits with kubectl rollout status, which applies the same readiness rules Helm does (observedGeneration vs generation, the new ReplicaSet for a Deployment, currentRevision == updateRevision for a StatefulSet), so Ready old pods still don't satisfy it and the #1228 case still fails.

I want to be straight about the trade-off, because it partly walks back something you asked for: this is a small piece of our own rollout checking, which you wanted gone. I think the distinction that matters is the one your review was actually about — the old script re-derived a verdict from revision numbers and pod counts, and inferred success from "revision advanced", which was simply wrong. This doesn't re-derive anything or second-guess helm; it only decides which workloads the wait covers, and delegates the readiness judgement to kubectl. If you'd still rather have plain --wait and treat a Pending fl-client as a real failure, say so and I'll switch it — it's a one-line change and your call as much as mine.

The scope is a deny list, not an allow list: anything not explicitly optional is waited on. Forgetting to exclude an optional workload gives a loud timeout; forgetting to add a core one to an allow list would silently stop checking it, which is the #1228 failure mode again. Excluded workloads are still reported when unready — they just can't change the exit code.

Not waited on	Why unready is a legitimate steady state
fl-client	Idle on a scale-from-zero GPU pool, or awaiting its kit
monailabel	Optional, off by default
grafana, loki, alloy	Observability; the trust serves while dashboards are down

Also fails closed: an unreadable cluster, or a release with no workloads found, exits 1 rather than reporting success.

On your main point — you were right and the PR was wrong. Helm creates revision N+1 before applying manifests, so "revision advanced" never meant "the spec applied", and your four exit-0-on-failure cases reproduced against gather()/decide(). deploy_verdict.py and its ~400 lines of tests are gone (04ebc0d34), replaced by the split-step path you described. The scoped wait is ead00753a.

Tests: 459 in the chart suite, up from 423 on develop. I mutation-tested the new code — inverting the deny list to an allow list, emptying the exclusions, ignoring rollout failures, dropping StatefulSets from discovery, dropping the release selector, and failing open on an empty release — all six are caught.

One gap I'd rather flag than paper over: the --wait failure above was reproduced on a live cluster, but the replacement has only unit coverage — the cluster I was using went away and neither kind nor minikube will bootstrap a control plane on this host right now. The logic is covered and mutation-tested, but it has not yet run end-to-end against a real rollout. Worth a look on your side if you have a cluster handy.