Skip to content

fix(bundler): carry localformat createNamespace into helmfile.yaml#901

Merged
mchmarny merged 1 commit into
NVIDIA:mainfrom
yuanchen8911:fix/helmfile-create-namespace-conflict
May 14, 2026
Merged

fix(bundler): carry localformat createNamespace into helmfile.yaml#901
mchmarny merged 1 commit into
NVIDIA:mainfrom
yuanchen8911:fix/helmfile-create-namespace-conflict

Conversation

@yuanchen8911
Copy link
Copy Markdown
Contributor

@yuanchen8911 yuanchen8911 commented May 14, 2026

Summary

Fixes an issue introduced by PR #899: the new --deployer helmfile path ignored localformat's per-folder chart-owns-Namespace decision and always relied on helmDefaults.createNamespace: true, causing helmfile apply to collide with charts that ship their own kind: Namespace resource (e.g., the os-talos privileged-namespace pattern). Expose Folder.CreateNamespace and emit a per-release createNamespace: false override for those folders.

Motivation / Context

The localformat writer already encodes the right decision into install.sh (effectiveCreateNamespace = caller intent && no chart-owned Namespace), originally to keep helm's --create-namespace away from charts that own their target Namespace. The helmfile deployer added in PR #899 bypasses install.sh and emits its own release graph, so it never saw that signal and fell back to the global default.

Failure mode at helmfile apply time: helm creates the namespace out-of-band via --create-namespace (no release ownership labels), then the chart's own Namespace template can't claim it because Helm 3 refuses to import a namespace it created out-of-band. Install fails.

The bug is dormant in main today (no current leaf overlay opts into os-talos, and no other in-tree pre-manifest declares a Namespace), so existing recipe shapes are unaffected. It would activate as soon as a leaf overlay opts in or a new pre-manifest declares a Namespace.

Fixes: post-merge follow-up to #899; surfaced during cross-review.
Related: #632, #899

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Refactoring (no functional changes)
  • Build/CI/tooling

Component(s) Affected

  • CLI (cmd/aicr, pkg/cli)
  • API server (cmd/aicrd, pkg/api, pkg/server)
  • Recipe engine / data (pkg/recipe)
  • Bundlers (pkg/bundler, pkg/component/*)
  • Collectors / snapshotter (pkg/collector, pkg/snapshotter)
  • Validator (pkg/validator)
  • Core libraries (pkg/errors, pkg/k8s)
  • Docs/examples (docs/, examples/)

Implementation Notes

  • localformat.Folder gains a CreateNamespace bool field. Populated by writeLocalHelmFolder from the existing effectiveCreateNamespace value (no new detection logic); upstream_helm.go and vendor_folder.go set it to true since those paths don't render AICR templates and the chart-owns-Namespace heuristic doesn't apply.
  • helmfile.Release gains CreateNamespace *bool with yaml:"createNamespace,omitempty". buildHelmfile only emits the field when the folder's CreateNamespace is false — helmDefaults.createNamespace: true continues to cover the common case, so the helmfile.yaml stays minimal for unaffected recipes.
  • Pointer (rather than bool) so a deliberate false survives YAML marshaling and absence omits the field.

Testing

make qualify

All 21 chainsaw tests pass. Two new unit tests added:

  • TestBuildHelmfile_CreateNamespaceFromFolder — synthetic folders with mixed CreateNamespace values; verifies the per-release override is emitted only when the folder's flag is false.
  • TestGenerate_NamespaceOwningPreManifest — end-to-end: a pre-manifest containing kind: Namespace flows through localformat.WritebuildHelmfile and surfaces as createNamespace: false on the injected <name>-pre release in the rendered helmfile.yaml.

Per-package coverage: pkg/bundler/deployer/helmfile 91.9% → 92.5% (+0.6%). pkg/bundler/deployer/localformat unchanged.

Empirical check (helmfile v1.5.1) — pre-fix bundle for a synthetic Namespace-owning pre-manifest emitted helmDefaults.createNamespace: true with no override; post-fix bundle emits createNamespace: false on the affected pre-release while every other release uses the default. helmfile show-dag against current main bundles continues to pass (no DAG regression).

Risk Assessment

  • Low — Isolated change, well-tested, easy to revert
  • Medium — Touches multiple components or has broader impact
  • High — Breaking change, affects critical paths, or complex rollout

Rollout notes: Backwards-compatible. The helmfile.yaml shape changes only for releases whose folder declared a Namespace resource — no such recipe ships today. Existing goldens are unchanged.

Checklist

  • Tests pass locally (make test with -race)
  • Linter passes (make lint)
  • I did not skip/disable tests to make CI green
  • I added/updated tests for new functionality
  • I updated docs if user-facing behavior changed (no user-facing behavior change)
  • Changes follow existing patterns in the codebase
  • Commits are cryptographically signed (git commit -S)

The helmfile deployer set a global helmDefaults.createNamespace: true
and ignored localformat's per-folder chart-owns-Namespace decision.
When a recipe ships a pre-manifest containing a kind: Namespace (e.g.,
the os-talos privileged-namespace pattern), helmfile apply would invoke
helm install --create-namespace, creating the namespace out-of-band
without release ownership labels; the chart's own Namespace template
then collides because Helm 3 refuses to import a pre-existing namespace
that lacks the release's ownership metadata.

The localformat writer already encodes the right decision into
install.sh (effectiveCreateNamespace = caller intent && no chart-owned
Namespace), but it didn't expose the result on the returned Folder, so
deployers that bypass install.sh (helmfile) couldn't see it.

Expose Folder.CreateNamespace and have the helmfile deployer emit a
per-release `createNamespace: false` for folders where the chart owns
the namespace. helmDefaults.createNamespace: true stays the global
default; the override is only rendered for the affected release, so
existing recipe shapes are unchanged.

The bug is dormant in main today (no current leaf overlay opts into
os-talos), but the fix prevents the regression as soon as one does or
a new pre-manifest declares a Namespace.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 14, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR extends the Helm deployment pipeline to support per-release control of namespace creation. A new CreateNamespace field is added to the Folder struct to represent whether Helm should create the target namespace, and a corresponding CreateNamespace *bool field is added to the Release struct for helmfile rendering. The field is populated differently across folder types: local Helm auto-detects from rendered manifests, upstream and vendored Helm folders explicitly set it to true. The helmfile generator now emits per-release createNamespace: false overrides when a folder indicates namespace ownership. Two tests verify the behavior: one unit test confirms buildHelmfile sets the override correctly, and one integration test validates end-to-end generation with namespace-owning pre-manifests.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • NVIDIA/aicr#868: Modifies namespace-creation logic in local_helm.go to detect Namespace manifests in rendered output, overlapping at the same code paths used here.
  • NVIDIA/aicr#899: Modifies the Helmfile deployer's Release model in releases.go and helmDefaults.createNamespace handling, with this PR extending support for per-release overrides.

Suggested labels

area/tests, size/M

Suggested reviewers

  • lalitadithya
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly describes the main change: exposing localformat's createNamespace decision to the helmfile deployer path.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description comprehensively explains the bug fix, including the root cause (helmfile deployer ignoring localformat's per-folder CreateNamespace decision), the failure mode, the implementation approach, testing, and risk assessment.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@yuanchen8911 yuanchen8911 requested a review from lockwobr May 14, 2026 23:44
@yuanchen8911 yuanchen8911 requested review from mchmarny and removed request for mchmarny May 14, 2026 23:48
@mchmarny mchmarny enabled auto-merge (squash) May 14, 2026 23:50
@mchmarny mchmarny merged commit 9588f12 into NVIDIA:main May 14, 2026
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants