Repository navigation
feat(helm): publish the CRDs as a separate cluster-readiness-engine-crds chart (ADR-107) - #560
Merged
Merged
Conversation
…rds chart Implements ADR-107 (#357). A plain helm upgrade never updated the NVCRE CRDs, so the new controller ran against the old schema and the API server silently pruned the fields it lacked. Charts: - Move the main chart's CRDs into a local crds subchart, enabled by crds.enabled (default true). The default renders exactly as before; crds.enabled=false turns the CRDs off wherever Helm renders the chart. - Add helm/cluster-readiness-engine-crds, which renders the same CRDs from templates/ so helm upgrade keeps them current, each with helm.sh/resource-policy: keep (crds.keep). It fails to render when files/ is empty. - make manifests writes both copies; both are committed, so verify-codegen catches drift. helm-lint, helm-package and helm-push cover both charts. nvcrectl setup init (decision 7), in order: - stop before the deps phase when any release that owns an NVCRE CRD is failed, pending or unreadable, including partial ownership; - skip the apply and pass --skip-crds only when one deployed release owns every CRD the chart being installed carries; - leave the CRDs alone when the release's values set crds.enabled=false; - otherwise apply them as before. Release: push, sign and attest the CRDs chart beside the main chart, and verify both at the release gate. Docs: deployment.md covers installing, upgrading and adopting with the CRDs chart, and moving an Argo CD install to it. ADR-107 is accepted, with the Argo CD handoff tested on Kind; ADR-064 notes the amendment. Signed-off-by: Kayne Tu <[email protected]>
Contributor
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 69.23% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 52 functions across 16 files. (2 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title names the main change: a new cluster-readiness-engine-crds Helm chart that publishes the CRDs separately, tied to ADR-107. The changeset matches this, including the new chart, the subchart m… Description check The description is closely related to the changeset. It explains the ADR-107 implementation, the main chart's CRD subchart, the new CRDs chart, the nvcrectl setup init ownership rules, the release wor… Linked Issues check Direct issue #357asks for the chart's CRDs to be published as a separatecluster-readiness-engine-crdsHelm chart, so that consumers can upgrade CRDs as an ordinary, orderable release. The change i…Out of Scope Changes check The changes follow from the separate-chart request in #357or support it:
- New CRDs chart, main-chartcrdssubchart,make manifests, Makefile packaging and push, and release workflow changes publ…Full details: Title check
-
Explanation
The title names the main change: a new cluster-readiness-engine-crds Helm chart that publishes the CRDs separately, tied to ADR-107. The changeset matches this, including the new chart, the subchart move, and the setup init CRD ownership logic. It is specific and conventional, so it passes.
Full details: Description check
-
Explanation
The description is closely related to the changeset. It explains the ADR-107 implementation, the main chart's CRD subchart, the new CRDs chart, the nvcrectl setup init ownership rules, the release workflow changes, the docs, and the testing performed. It is detailed and on-topic, so it passes.
Full details: Linked Issues check
-
Explanation
Direct issue
#357asks for the chart's CRDs to be published as a separatecluster-readiness-engine-crdsHelm chart, so that consumers can upgrade CRDs as an ordinary, orderable release. The change implements this request as follows:- New chart
helm/cluster-readiness-engine-crds/(Chart.yaml, values.yaml, templates/crds.yaml, templates/_helpers.tpl).templates/crds.yamlrenders the CRDs fromfiles/and fails whenfiles/is empty. This keepshelm upgradein charge of the CRDs, including field removal. crds.keep(defaulttrue) addshelm.sh/resource-policy: keep.- The main chart keeps its CRDs in a
crdssubchart controlled bycrds.enabled(defaulttrue), so the default install still delivers the CRDs. make manifestswrites both copies.test/helm/crds_test.go(TestCRDsChartRenders,TestCRDsSubchartFollowsEnabled,TestCRDsChartMatchesSubchart) renders both charts and checks that the two copies match.test/helm/release_size_test.gochecks the Helm release size budget.Makefilelint, package, and push targets cover both charts..github/workflows/release.ymlpublishes and attests the CRDs chart (attest-crds-chart,chart-attested, release gate).docs/operations/deployment.mdcovers Helm, Helmfile, Flux, and Argo CD ordering.
No concrete coding requirement from#357is unmet in the reviewed evidence.
Limit: the CRD YAML files underhelm/cluster-readiness-engine-crds/files/andhelm/cluster-readiness-engine/charts/crds/crds/are excluded by!helm/cluster-readiness-engine-crds/files/**and!helm/cluster-readiness-engine/charts/crds/crds/**(ignored_files). Their content was not inspected. The PASS rests on the reviewable chart structure, publishing workflow, and the consistency test, which does not depend on the exact CRD YAML values. Assessment uses the whole-PR summary; no repository inspection was performed.
- New chart
Full details: Out of Scope Changes check
-
Explanation
The changes follow from the separate-chart request in
#357or support it:- New CRDs chart, main-chart
crdssubchart,make manifests, Makefile packaging and push, and release workflow changes publish and attest both charts. - Release-gate
verify_chartrefactor,ATTEST_CRDSgate handling, and release-policy tests cover the second chart. pkg/setup/crd_ownership.go,crds.go,helm.go,helm_values.go, andsetup.gopreventsetup initfrom conflicting with the CRDs chart over the same CRDs. Publishing a second chart that owns the same CRDs creates this need, so these changes are a direct consequence of the request. They are the largest behavior change in the PR and go beyond the literal#357text, but no unrelated behavior is shown.jobset_ownership.goandrecovery_inventory.gochanges replace string literals with constants (managed-bylabel,apiextensionsGroup). These are small supporting refactors.test/helm/release_size_test.gochecks the Helm Secret size budget for both charts.- Documentation (
deployment.md,setup.md,faq.md, ADR-064, ADR-107,RELEASE.md),CLAUDE.md,AGENTS.md,.coderabbit.yaml, andcmd/integrationpath updates follow the moved CRD location.
No demonstrated unrelated change was found. Thepkg/setupownership logic is a judgment call of scope, not a demonstrated defect. Limit: the excluded CRD YAML files were not inspected. Assessment uses the whole-PR summary; no repository inspection was performed.
- New CRDs chart, main-chart
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 69.23% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 52 functions across 16 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
ndipebot
previously approved these changes
Oct 23, 2026
ndipebot
left a comment
ndipebot
left a comment
Collaborator
There was a problem hiding this comment.
I ran this with Helm 3.18.4 from a worktree at 79735fc9, and every mechanic ADR-107 claims holds when I actually run it.
helm template t helm/cluster-readiness-engine --include-crds gives 7 CRDs, 0 with --set crds.enabled=false. helm show crds gives 7 either way, which is exactly why pkg/setup has to read crds.enabled itself rather than infer it from the stream, and helm_values.go does. files/ is committed as a rename of the old top-level crds/ and not staged at package time, so test/helm's identity assertion renders real content instead of an empty tree. The fromYaml/toYaml round trip is lossless: all seven rendered spec blocks match charts/crds/crds/*.yaml after normalization, and maxLength: 65536, maxItems: 128 and minimum: -1 come back intact, no scientific notation. TestCRDsSubchartFollowsEnabled does pass --include-crds, so its goldens are 7 names vs [], not both empty.
helm dependency build --skip-refresh resolves the local crds dependency and leaves git status clean, helm lint passes on both charts, and helm package keeps charts/crds/crds/* in the main tgz and files/* in the CRDs one. go test ./pkg/setup/ ./test/helm/ passes. The only surviving references to helm/cluster-readiness-engine/crds/ are in ADRs 076, 077, 088, 094 and 097, which are records and correctly left alone.
The four rules in planCRDReconcile and confirmReleaseOwned line up with decision 7, and the stop sits after the confirmation prompt and before setupPhaseParams is built (setup.go:327-337), so nothing has touched the cluster by then. confirmReleaseOwned reads the chart's actual CRD set rather than a hardcoded seven, and owned-deployed-chart-adds-crd pins that. The 22 crd-ownership fixtures assert action, owner, skipCRDs, the status-read order and the full printed output, and gate_step_legacy_test.go gives the CRDs chart a digest distinct from the main chart's, so a check that only ever reached the main chart cannot pass on the wrong subject.
I modelled Helm's encodeRelease independently over the 1,889,347 bytes in files/ (JSON carrying chart.File.Data as base64 plus the rendered manifest, then gzip, then base64) and got 677,060 bytes. The ADR's 686 KB is honest and its "about 1.5x headroom" is right.
The Argo CD handoff you cut from six steps to two is grounded in named Argo CD source (!kube.IsCRD(target), shouldBeDeleted) rather than asserted, which is better than what the merged ADR proposed.
Two optional items inline: an unpaged full-CRD LIST in listGroupCRDs, and no CI guard on the release-Secret size the ADR itself calls close. Neither needs to block this.
/lgtm
/approve
planCRDReconcile needs only CRD names, labels and annotations, but listed every CRD in the cluster with its full schema in one unpaged request. It now lists PartialObjectMetadata and selects the NVCRE group by name suffix, which the API server's <plural>.<group> naming makes equivalent. TestReleaseSecretFitsBudget estimates both charts' release Secrets the way Helm's Secrets driver encodes them and fails above 850 KiB, so the headroom ADR-107 records breaks CI before it breaks an install. Signed-off-by: Kayne Tu <[email protected]>
Brings in #510 (networkAttachments). Its CRD changes landed only in the crds subchart's copy, so make manifests regenerated the CRDs chart's files/ to match. Signed-off-by: Kayne Tu <[email protected]>
ndipebot
previously approved these changes
Oct 25, 2026
ndipebot
left a comment
ndipebot
left a comment
Collaborator
There was a problem hiding this comment.
Both optional items landed.
listGroupCRDs now lists a metav1.PartialObjectMetadataList with the apiextensions.k8s.io/v1, Kind=CustomResourceDefinitionList GVK, which is the list kind controller-runtime routes to its metadata client, and no caller lost anything: planCRDReconcile, soleHelmOwner and crdOwnerSummary read only names, labels and annotations. I replaced the new suffix condition with false in a worktree and TestPlanCRDReconcile failed, so a filter that quietly returns nothing cannot land; other-group-ignored still passes.
TestReleaseSecretFitsBudget fires. I dropped releaseSecretBudget to 600 KiB and the CRDs chart case failed at 692712 bytes. At the real budget the CRDs chart is 692712 and the main chart 462992, leaving about 177 KB of headroom, close to the 686141 I had estimated by hand. Leaving --include-crds off matches Helm: a chart's crds/ directory is not part of the stored manifest, while the CRDs chart's files/ are rendered by templates/crds.yaml and so are. CI installs Helm at ci.yml:93, so requireHelm is not a silent skip.
After the origin/main merge in 47093b15 I re-ran bin/controller-gen into helm/cluster-readiness-engine/charts/crds/crds in a worktree at pr/560 and git status helm/ stayed clean. diff -rq helm/cluster-readiness-engine-crds/files helm/cluster-readiness-engine/charts/crds/crds is empty, both copies carry main's networkAttachments, and diff CLAUDE.md AGENTS.md is clean. go build ./..., go test ./pkg/setup/ -run TestPlanCRDReconcile and go test ./test/helm/ -run TestReleaseSecretFitsBudget all pass.
Two nits, both yours to take or leave. One is inline on the suffix filter. The other: checkNVCRECRDs (pkg/setup/status.go:253) and listCRDNamesByGroup (pkg/setup/setup.go:1639) still build an unstructured CustomResourceDefinitionList with full schemas, with no page size, to answer "does any NVCRE CRD exist" and "what are their names". Both could call the new listGroupCRDs, which would also drop the duplicated spec.group extraction. discoverGroupResources and findTrainJobCRD genuinely need spec.names, so they are not comparable. Fold those two in here, or leave them for whoever next touches that file?
Brings in #529, #543, #549 and #555. Their CRD changes landed only in the crds subchart's copy, so make manifests regenerated the CRDs chart's files/ to match. Signed-off-by: Kayne Tu <[email protected]>
A name ending in .nvcre.nvidia.com also matched CRDs in sub-groups such as extra.nvcre.nvidia.com. The group is the part of the name after the first dot, so compare that to the API group exactly. The sub-group-ignored case pins it: a failed release owning a sub-group CRD no longer stops setup init. Signed-off-by: Kayne Tu <[email protected]>
ndipebot
approved these changes
Oct 26, 2026
ndipebot
left a comment
ndipebot
left a comment
Collaborator
There was a problem hiding this comment.
strings.Cut on the first dot is the exact match I was after, and the new sub-group-ignored case earns its place. I put HasSuffix back in a worktree with the case kept and it failed with helm release other/widgets, which owns NVCRE CRDs, is failed; resolve it and rerun setup init, which is precisely the false positive the loose filter allowed: a CRD in extra.nvcre.nvidia.com owned by an unrelated failed release would have blocked setup init.
9 of 11 tasks
kangclzjc
added a commit
to kangclzjc/cluster-readiness-engine
that referenced
this pull request
Oct 27, 2026
…y-679ad1 NVIDIA#560 (ADR-107) moved the CRDs into the main chart's crds subchart, with a copy in the cluster-readiness-engine-crds chart. Two follow-ups make the merge correct: - make manifests regenerates both copies. Rename detection carried this branch's Certification CRD into charts/crds/crds/ only, which left the files/ copy at main's version, with the quantity() rules that 1.29 to 1.33 refuse. - test/crdcompat reads the subchart directory, because the old crds/ directory no longer exists. Signed-off-by: Kang Zhang <[email protected]>
16 of 22 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
Implements ADR-107 (#551). A plain
helm upgradenever updated the NVCRE CRDs, so the new controller ran against the old schema and the API server silently pruned the fields it lacked.Charts
crdssubchart (charts/crds/crds/), enabled bycrds.enabled, defaulttrue. The default renders exactly as before.crds.enabled=falseturns the CRDs off wherever Helm renders the chart.helm/cluster-readiness-engine-crds/renders the same CRDs fromtemplates/, sohelm upgradekeeps them current, including removing fields. Each CRD carrieshelm.sh/resource-policy: keep(crds.keep). The template fails whenfiles/is empty, so an empty chart cannot publish.make manifestswrites both copies and both are committed (Ebot's note 1 on docs(designs): add ADR-107 for a separate cluster-readiness-engine-crds chart #551):verify-codegencatches drift, andtest/helmrenders the real charts from the source tree.helm-lint,helm-packageandhelm-pushcover both charts;helm-pushaddsCRDS_CHART_DIGEST_FILE.nvcrectl
setup init(decision 7), first matching rule wins:failed,pending-*or unreadable: stop before thedepsphase changes anything. This includes partial ownership, such as a Helm 4 adoption conflict leaving six CRDs owned by a failed release and one unowned.deployedrelease owns every CRD the chart being installed carries: skip the apply and pass--skip-crds. The set comes fromhelm show crds, not a fixed seven, so a missing CRD or one a newer chart adds falls through.crds.enabled=false: leave the CRDs alone.Release:
helm-publishpushes both charts; a newattest-crds-chartjob signs the CRDs chart throughattest.yml;chart-attestedand the release gate check both. The release notes gain a CRDs-chart section and the CRD path change.Docs:
deployment.mdcovers installing, upgrading and adopting with the CRDs chart (Helm 4--take-ownership/--force-conflicts), Helmfile and Flux ordering, and moving an Argo CD install. ADR-107 is accepted, with Ebot's notes 2 and 3 fixed; ADR-064 notes the amendment.Review focus
!kube.IsCRD(target)), and cascading deletion skips CRDs andkeepresources (shouldBeDeleted), from v2.10.0 through v3.5.6. So the main Application never owns the CRDs, and "disable pruning around the switch" is unnecessary. ADR-107's Notes record the runs.pkg/setup/crd_ownership.go: the two-stage decision. Stops happen before thedepsphase; the full-set check runs after the read-onlyhelm show crds.release.yml: the gate's chart checks are now averify_chartfunction run once per chart, with the main chart's messages unchanged.Related Issue
Closes #357. AICR's adoption is tracked in NVIDIA/aicr#2525.
Type of Change
Component(s) Affected
Testing
make lint,make build,make test(unit and integration),go test ./test/helm/ ./test/releasepolicy/ ./test/docspolicy/,make verify-doc-linkshelm uninstall, adoption with--take-ownership.nvcrectl setup initwith a controller image from this branch, through every rule: stale owner (apply), deployed owner (skip; the CRDs'helmmanaged-fields timestamp did not move across a full run), missing CRD (apply, recreated), failed owner after a Helm 4nvcrectl-setupconflict (stop; main release revision unchanged), six CRDs owned by a failed release plus one unowned, with and withoutcrds.enabled=false(stop), the documented--force-conflictsrecovery, andcrds.enabled=falseset and kept (CRDs untouched).crds/tocharts/crds/crds/, noted in the release notes;helm show crdsreturns the CRDs as before.New golden files were generated for new cases only and reviewed; no existing golden changed. Release-policy tests were extended for the CRDs chart and mutation-checked: dropping it from the gate turns them red.
Checklist
git commit -s)make manifests generaterun (if*_types.gowas modified)