fix(bundler): the helm readiness gate blocks and re-runs on upgrade - #2863
EronWright wants to merge 6 commits into
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>
|
Welcome to AICR, @EronWright! Thanks for your first pull request. Before review, please ensure:
A maintainer will review this soon. |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: NVIDIA/aicr/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 (17)
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. YAML rewriting is shared between normalization and hook removal. New readiness-gate Helm fixtures include charts, deployment scripts, RBAC, a hook Job, and documentation. Tests verify phase-scoped annotation 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 Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
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.
|
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 |
2 participants
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)