Repository navigation
fix(bundler): the helm readiness gate blocks and re-runs on upgrade - #2863
Merged
mchmarny merged 13 commits intoSep 22, 2026
Merged
Conversation
gatemanifest.Render picks the readiness gate's annotations with full knowledge
of the target deployer: Helm hooks for helm, argocd.argoproj.io/sync-options
for Argo CD, and -- documented in its own comment -- deliberately no
helm.sh/hook for Argo, because Argo maps one to a PostSync hook that never
fires under syncPolicy.automated. writeLocalHelmFolder then strips the Helm
ones back off on the way to disk.
Under --deployer helm that leaves the gate shipping as a bare Job, and both
annotations it lost were load-bearing:
- helm.sh/hook: post-install,post-upgrade is what makes the gate BLOCK.
deploy.sh passes --wait without --wait-for-jobs, and says why in its own
comment: "Helm --wait blocks on hook completion regardless of
--wait-for-jobs". True of a hook. A bare Job under --wait alone returns as
soon as the object exists, so dependents start against a cluster the gate
has not finished checking -- which is the entire thing a gate exists to
prevent.
- helm.sh/hook-delete-policy: before-hook-creation is what makes it RE-RUN.
A Job's spec.template is immutable, so an identical manifest is a no-op
patch. Without the delete-and-recreate the gate asserts once, at install,
and every subsequent upgrade ships unverified.
There is no golden for a helm readiness bundle, which is why this has been
invisible. This adds one.
The fix is keyed on the PHASE, not on the deployer -- localformat gains no
deployer knowledge, and the invariant in its doc.go, that it never grows
per-deployer branches, still holds. The readiness folder is simply the only
folder localformat writes that localformat's caller authored: every other one
carries recipe content whose author cannot know the consuming deployer, and
stripping those stays exactly as it was. That distinction is not cosmetic --
31 recipe files carry hook annotations whose mandated trio includes
before-hook-creation, and doc.go records what keeping those did under plain
Helm before NVIDIA#1835.
Mechanically: the yaml.v3 round-trip moves out of stripHelmHooks into a shared
rewriteYAMLDocs, and normalizeYAMLDocs exposes it without the hook filter. The
round-trip is not incidental -- it is the only YAML-validity check a rendered
manifest gets, and the consistent re-encode is what keeps every golden stable
-- so the exempt path skips the filter, never the rewrite.
That is also what makes this a no-op for Argo CD: its gate carries
sync-options, which the filter never touched, so normalize-only reproduces the
same bytes. git diff --exit-code over the argocd and argocd-helm testdata is
empty, and is the regression proof.
Terraform's gate is separate: it is annotation-free by construction and does
not change here.
Signed-off-by: Eron Wright <eronwright@gmail.com>
Contributor
|
Welcome to AICR, @EronWright! Thanks for your first pull request. Before review, please ensure:
A maintainer will review this soon. |
3 tasks done
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/aicr/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe local Helm writer now preserves Helm hook annotations for readiness folders while continuing to strip them from pre and post folders. Shared YAML rewriting supports both normalization and hook removal. New readiness-gate fixtures define charts, scripts, RBAC, a hook Job, deployment orchestration, and documentation. Tests verify phase-scoped handling and compare the generated bundle with a golden fixture. Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: High 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
mchmarny
enabled auto-merge (squash)
September 20, 2026 12:05
mchmarny
dismissed
their stale review
September 20, 2026 13:54
Withdrawing approval because current-head CI deterministically fails TestBundleGolden_ReadinessGate after #2855 changed the generated install script; see the follow-up comment for the required golden refresh.
Member
|
I removed my approval because the current-head CI failure is deterministic and needs a branch update. The new readiness-gate golden was generated before #2855 changed The PR was green at GOFLAGS=-mod=readonly go test ./pkg/bundler/deployer/helm -run '^TestBundleGolden_ReadinessGate$' -count=1 |
NVIDIA#2855 added the dry-run stale-archive guard to install-upstream-helm.sh.tmpl after this branch captured its golden, so TestBundleGolden_ReadinessGate compared freshly generated output against the pre-NVIDIA#2855 install.sh and failed. Regenerated with -update; the only file that moves is testdata/readiness_gate/001-foo/install.sh, which now carries the same DRY_RUN_FLAG guard and comment the other helm goldens already have. No production code changes. Signed-off-by: Eron Wright <eronwright@gmail.com>
auto-merge was automatically disabled
September 20, 2026 19:06
Head branch was pushed to by a user without write access
Contributor
Author
|
Thanks — diagnosis confirmed, and fixed in 36bf781. Regenerated the readiness-gate golden with Verified on the pushed head: GOFLAGS=-mod=readonly go test ./pkg/bundler/deployer/helm -run '^TestBundleGolden_ReadinessGate$' -count=1 # ok
go test -race ./pkg/bundler/... # ok
|
EronWright
added a commit
to turfbuild/turf-examples
that referenced
this pull request
Sep 21, 2026
…er under it Regenerated bundle/ from turfbuild/aicr#3 (the terraform deployer as submitted, stacked on NVIDIA/aicr#2863) with the README's own command and the pinned recipe. Three things the submission dropped from the fork's earlier superset, and the example now reflects each honestly: the shim is a terraform_data, referenced whole by replace_triggered_by, so hashicorp/helm is the bundle's only provider; no create_before_destroy, so a replaced cluster replaces its releases as plain destroy-then-create; no content digests, so an edit to bundled chart content does not reach the plan and the gate re-runs only on its own upgrade. The last two move from the README's feature list to its failure list. Two other things moved under the same recipe: the embedded component data (nodewright drops webhook.removalImage, nvsentinel gains the syslog monitor block), and the bundle now carries bundle-info.yaml. Measured on turf-engine 1ce29f8, whose sidecar serves the built-in provider, so the bundle runs exactly as generated: up from empty converges in two rounds (6 then 25 applied). Then the part the example exists for — replace the cluster: all sixteen releases are condemned in the round of the replace, including the ten inside modules that defer whole, the deletes complete through the old cluster before its destroy does, and round two re-creates them in the new one (28 then 34 applied, 0 failed). tofu plan against the engine's statefile says No changes after each converge; down removes 31/31. Terraform's experimental deferral still refuses the shim, now by the name terraform_data.cluster; stock Terraform plans 31 to add in one round.
mchmarny
enabled auto-merge (squash)
September 22, 2026 12:34
EronWright
added a commit
to turfbuild/turf-examples
that referenced
this pull request
Sep 25, 2026
* An AICR recipe is a graph, and flattening it costs a quarter of the run Nothing in use-cases/datacenter/aicr-stack was written by hand. The .tf files are generated from one NVIDIA AI Cluster Runtime recipe — service=kind, accelerator=h100, intent=inference — which resolves to 14 components, 16 dependency edges and a DAG four levels deep. The generator turns dependencyRefs into depends_on between module calls and ignores the recipe's precomputed deploymentOrder, because that field is a linearisation of the graph and the engine it is being handed to schedules graphs. That distinction is the example's subject. AICR states it is not a cluster provisioner, so the cluster is a precondition in everything it renders, and of the five renderers it ships only flux carries the real edges: helm emits a line of sixteen, argocd emits sync-waves 1/5/9/13, helmfile emits nested level files. Both of the latter are barriers. Terraform is the one consumer that can hold the graph and create the cluster underneath it. The cost of a barrier is measured, not asserted, and from a single run: helmfile reports a per-release DURATION, so both scheduling models are computed over one sync's own numbers. The wave model predicts 204s against that run's observed 213s, which validates it; the DAG over the same durations is 152s. The clearest case is kube-prometheus-stack waiting 58 seconds for nodewright-operator, a component with no edges into or out of it anywhere in this recipe. Two findings carried over from the NGC stack and confirmed at this size: the whole kind recipe pulls anonymously from seven registries with no key of any kind, and on a node with no NVIDIA hardware the GPU Operator creates no operand DaemonSets while nvsentinel and nvidia-dra-driver-gpu sit cleanly at desired=0. One NFD label is still the entire seam. The honest failure is that stock Terraform cannot plan this tree. Every module carries replace_triggered_by as its containment shim, and the experimental deferred-actions path fails with "no change found" when that references a resource which is itself deferred — reproduced on v1.17.0-alpha20260827 and on a source build of main, and needing both halves, since dropping either the lifecycle block or the depends_on makes it pass. Measured on the Restate engine only; unlike ngc-stack this example has not been run on the shipping MCP engine, and the README says so. * The generator moved upstream, so the example is a wrapper now Everything under bundle/ is now emitted by `aicr bundle --deployer terraform` rather than by the bespoke generator that produced the first version of this example. The hand-written Terraform is down to main.tf and versions.tf, about forty lines, doing the one thing AICR will not: putting the cluster in the graph. Regenerating is one command, and it reproduces the committed bytes exactly — the deployer emits already-fmt-clean HCL, so `git diff` after a regenerate is the check that this directory is current. That retires two of the three decisions this PR was left open on: where the generator lives (upstream, as turfbuild/aicr#1), and whether committed bytes match generated ones (they do). Two findings came out of moving it, both now in the failure list: A module that configures its own provider cannot be composed. The default bundle is a root module — it declares `provider "helm"` and takes the connection as variables, which is right for an existing cluster. Calling that as a child module is a different shape: Terraform accepts it but then forbids depends_on/count/for_each on the call, and the engine refuses it outright with a message that says so and says why it refuses rather than skips. bundle/ is therefore generated with --terraform-child-module, and the root's helm provider — bound to kind_cluster.dc — is inherited. The replace_triggered_by deferral bug survives verbatim into generated output: same three modules, same error, now reachable as `--terraform-cluster-rollover`, which is off by default upstream for exactly this reason. Regenerate without it and experimental Terraform converges this tree too, at the cost of the containment. kai-scheduler's `wait = false` is no longer a hardcode in a bespoke generator: AICR keeps that fact in one table now shared by deploy.sh, the helmfile deployer and this one, and the module call carries the reason inline. Re-run from empty state on turf-engine 0a94e62: round 1 plans 22 addresses and defers 16, round 2 applies 27, converged in 2 rounds, down removes 33 in one phase. All 16 releases deployed; ClusterPolicy ready/NoGPUNodes, no pci-* labels, the same six DaemonSets at desired=0. No wall-clock figure is quoted any more. Two clean runs of this tree differed by a factor of three, in round two, which is dominated by pulling sixteen charts' worth of images into a seconds-old cluster. That measures a laptop, not the engine. The DAG-vs-waves number stays, because it is derived from a single run's own per-release durations. The values.yaml changes are recipe data, not this example: the fork is at a newer upstream AICR than the one that produced the first bundle, and several components gained tolerations. * One module call per component, so depends_on means what it says Regenerated against the reshaped terraform deployer. The bundle's own folders are byte-identical; only the .tf layer moved. 16 releases now render as 14 module calls. A component's post-manifest release lives inside its module, so `depends_on = [module.agentgateway_crds]` covers it without naming the tail. Re-run end to end on turf-engine 0076f5d: round 1 applies 6 and defers 24, round 2 applies 25, converged in 2 rounds; 16 releases deployed, ClusterPolicy ready, 0 pci-* labels, 6 DaemonSets at desired=0; down removes all 31 in one phase. The converged total drops 33 -> 31 because there are 14 containment shims rather than 16. The first plan got noisier, and the README now says why: an empty count=0 slot whose dependency is deferred is itself deferred before its count is evaluated, so cert-manager shows post and readiness slots it does not have. They collapse in round two. Also records a measured limitation of gates under Terraform. helm_release tracks a local chart's path, version and values, not its rendered manifests, so editing a template produces no diff at all; and a values change does upgrade the release but leaves the Job untouched, since spec.template is immutable. A gate therefore verifies once, at creation. Signed-off-by: Eron Wright <eronwright@gmail.com> * The gate is absent by choice, and the README said the wrong reason It claimed no component in this recipe ships a readiness.yaml. gpu-operator does: regenerating with --readiness-hooks yields 012-gpu-operator-readiness, a slot on module.gpu_operator that its three dependents already wait for. The actual reason the flag is off is the gate image. AICR tags it with the bundler version and publishes it only on release tags; a source build stamps `dev` and falls back to ghcr.io/nvidia/aicr-gate:dev, a local-only tag you are meant to build and kind load (404 on ghcr). --deployer terraform exists only on a fork branch, so no released aicr can emit both the terraform bundle and a pullable gate, and committing the flag on would ship a bundle that ImagePullBackOffs anywhere but the machine that built it. Signed-off-by: Eron Wright <eronwright@gmail.com> * A gate now re-runs when its component changes Regenerated: the component module's readiness slot gained a replace_triggered_by on the component's own release. The bundle still ships with --readiness-hooks off, so the slot is count=0 here, but the module is the one upstream now emits. Nothing reaches a gate Job by accident. spec.template is immutable, so a values change upgrades the release and leaves the Job alone; and helm_release tracks a local chart's path, version and values, not its rendered manifests, so editing a template produces no plan at all. Replacing the release is uninstall plus install, which creates a new Job. Measured with the flag on: re-applying unchanged is 0 of 32 addresses; changing gpu-operator's values is 2 of 32 — helm_release.this updated and helm_release.readiness[0] replaced — and the other thirteen gates stay put. The gate itself polls ClusterPolicy through a stability window and passes at T+30s. Signed-off-by: Eron Wright <eronwright@gmail.com> * A gate re-verifies without being replaced, and content reaches the plan Regenerated against the fix in turfbuild/aicr#1. Two changes to the component module, both about a change failing to reach the cluster. Every slot installing a chart from this bundle now carries a digest of that folder's bytes in the release's description. helm_release records a local chart's path, version and values, never its rendered manifests, so without it editing a template under NNN-<component>-post/ produces no plan at all — terraform apply reports "No changes" while the bundle on disk differs from what is deployed. Two -post wrappers here get one: agentgateway-crds and agentgateway. The readiness slot no longer uses replace_triggered_by on the component's release. That shape could not survive this bundle: --terraform-cluster-rollover gives every slot create_before_destroy and the gate inherits it, so the replacement installed while the old release still held the name, in the same cluster, and helm refused it — then the next plan said no changes, so the gate had silently stopped asserting. Instead the gate Job is a helm post-install,post-upgrade hook that helm deletes and re-runs on every upgrade and blocks on, and the release upgrades when either its own digest or the component's metadata.revision moves. The bundle still ships with --readiness-hooks off, so the slot is count=0 here. Measured on kind with the flag on, Terraform v1.16.2 and turf-engine, rollover both off and on: re-apply unchanged No changes / 0 of 18 change gpu-operator 0 to add, 2 to change, 0 to destroy — this and readiness[0] BOTH updated in place — and a new Job UID then settles No changes / 0 of 18 edit a -post template no diff at all, the hole above move its digest post[0] updated in place, and the ConfigMap lands replace the cluster 32 added / 15 destroyed, 17 releases, gate re-ran there Both engines agree, because the fix is in the generated HCL rather than in either one. Signed-off-by: Eron Wright <eronwright@gmail.com> * The bundle is what the submission generates, and the cluster rolls over under it Regenerated bundle/ from turfbuild/aicr#3 (the terraform deployer as submitted, stacked on NVIDIA/aicr#2863) with the README's own command and the pinned recipe. Three things the submission dropped from the fork's earlier superset, and the example now reflects each honestly: the shim is a terraform_data, referenced whole by replace_triggered_by, so hashicorp/helm is the bundle's only provider; no create_before_destroy, so a replaced cluster replaces its releases as plain destroy-then-create; no content digests, so an edit to bundled chart content does not reach the plan and the gate re-runs only on its own upgrade. The last two move from the README's feature list to its failure list. Two other things moved under the same recipe: the embedded component data (nodewright drops webhook.removalImage, nvsentinel gains the syslog monitor block), and the bundle now carries bundle-info.yaml. Measured on turf-engine 1ce29f8, whose sidecar serves the built-in provider, so the bundle runs exactly as generated: up from empty converges in two rounds (6 then 25 applied). Then the part the example exists for — replace the cluster: all sixteen releases are condemned in the round of the replace, including the ten inside modules that defer whole, the deletes complete through the old cluster before its destroy does, and round two re-creates them in the new one (28 then 34 applied, 0 failed). tofu plan against the engine's statefile says No changes after each converge; down removes 31/31. Terraform's experimental deferral still refuses the shim, now by the name terraform_data.cluster; stock Terraform plans 31 to add in one round. --------- Signed-off-by: Eron Wright <eronwright@gmail.com>
13 of 25 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The readiness gate is emitted without its
helm.sh/hookannotations, so under--deployer helmit neither blocks the deploy nor re-runs on upgrade. This keeps them.Motivation / Context
gatemanifest.Renderemitshelm.sh/hook: post-install,post-upgradeandhelm.sh/hook-delete-policy: before-hook-creationfor--deployer helm.localformat'sstripHelmHooks, applied to every folder it writes, removes them again — the gate Job ships withannotations: {}.A Job blocks
helm upgradeif it is a hook (--wait) or if Helm is told to wait for a bare one (--wait-for-jobs).deploy.shdrops--wait-for-jobsbecause the gate is a hook; the gate is not a hook, because the annotations were stripped. The bundle has neither mechanism.So the gate does not block — dependents install against a cluster it has not finished checking — and does not re-run, since a Job's
spec.templateis immutable and nothing deletes it.helm upgradereports success either way.Not a regression: byte-identical output at #1110, where the gate was introduced, and at current
main. #1110's notes describe this path as--wait --wait-for-jobs, but the merged template only ever passed--wait.Fixes: #2862
Related: #904, #2408 (the same failure, fixed for Argo CD only)
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
Keyed on the injection phase, not the deployer.
localformat/doc.goinvariant 1 forbids per-deployer branches inWrite, and this adds none: the readiness folder is the only folderlocalformatwrites whose content its own caller authored. Recipe content, whose author cannot know the target deployer, is stripped exactly as before.Not a blanket exemption for non-Argo deployers. That reopens #1835 — 31 recipe files carry 96 hook annotations including
before-hook-creation, which under plain Helm made resources fire-and-forget, skipped by upgrade, left behind by uninstall, and delete-recreated on every upgrade.The exempt path still round-trips the YAML.
stripHelmHooksalso re-encodes at a consistent indent (what keeps goldens stable) and rejects unparseable input (the only YAML check a manifest gets before a cluster sees it). The round-trip moves into a sharedrewriteYAMLDocs; the exemption skips the filter, never the rewrite — which is why Argo CD's bytes do not move.Testing
The new golden catches the bug.
TestBundleGolden_ReadinessGatefails with the fix reverted and passes with it, and pins both annotations by name as well as by golden bytes.pkg/bundler/deployer/helm/testdata/previously had no readiness golden at all, which is why nothing caught this.The Argo goldens must not move — they don't.
git diff --exit-codeoverargocd/testdataandargocdhelm/testdatais clean andTestBundleGolden*pass unchanged; they are not-updated here.The exemption is phase-scoped.
TestWrite_HookExemptionIsScopedToReadinessputs a readiness folder beside a hook-annotated post folder and asserts the gate keeps its annotations and the post wrapper is still stripped. A blanket exemption passes the first half and reopens #1835.Coverage:
localformat81.9%,helm88.1%.normalizeYAMLDocsandstripHelmHooksare at 100%.Risk Assessment
Rollout notes: the only output that changes is the readiness folder of a
--deployer helm --readiness-hooksbundle, which regains two annotations it was always meant to ship with. Argo CD output is byte-identical; the Flux and Helmfile gates carry no annotations to keep. No flags, config or API surface.Checklist
make testwith-race)make lint)git commit -S)