Repository navigation
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 <kaynet@nvidia.com>
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⛔ Files ignored due to path filters: 77
⚙️ Run configuration
⛔ Files ignored due to path filters: 77
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
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| 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. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description check The description clearly explains the separate CRDs chart, setup ownership behavior, release changes, documentation updates, and testing. It is directly related to the changeset. Title check The title clearly and concisely identifies the primary change: publishing CRDs as a separate Helm chart under ADR-107. Linked Issues check Directly linked issue #357requires publishing the CRDs as a separatecluster-readiness-engine-crdsHelm chart so consumers can upgrade CRDs as an ordinary, orderable release. The reviewed head cont…Out of Scope Changes check The changes support the CRDs-chart work requested by #357. The new chart, the main chart'scrdssubchart,make manifests, the Makefile packaging and push targets, and the release workflow publish …Full details: Linked Issues check
-
Explanation
Directly linked issue
#357requires publishing the CRDs as a separatecluster-readiness-engine-crdsHelm chart so consumers can upgrade CRDs as an ordinary, orderable release. The reviewed head containshelm/cluster-readiness-engine-crds/withChart.yaml,values.yaml,templates/crds.yaml,templates/_helpers.tpl, and seven CRD files underfiles/(one per NVCRE CRD).templates/crds.yamlrenders those files, and the summary states that rendering fails whenfiles/is empty. Thecrds.keepvalue defaults totrue, which addshelm.sh/resource-policy: keep. The release workflow publishes and attests the chart (helm-publish,attest-crds-chart, release gate).test/helm/crds_test.gochecks that the standalone and subchart CRD copies match. The main chart keeps the CRDs in acrdssubchart enabled bycrds.enabled, so the default install still delivers the CRDs. Helm, Helmfile, Flux, and Argo CD ordering are covered indocs/operations/deployment.md. No concrete coding requirement from#357is unmet. Limit: this assessment relies on the whole-PR summary and the file listing at head. The incremental changes since the previous reviewed head were not re-read hunk by hunk. Acrds.godiff from the previous head returned no content, so that file did not change in the incremental range.
Full details: Out of Scope Changes check
-
Explanation
The changes support the CRDs-chart work requested by
#357. The new chart, the main chart'scrdssubchart,make manifests, the Makefile packaging and push targets, and the release workflow publish and attest both charts. Thenvcrectl setup initownership logic (pkg/setup/crd_ownership.go,helm.go,helm_values.go,setup.go) prevents the main chart and the CRDs chart from fighting over the same CRDs during setup. That is a direct consequence of publishing a second chart that owns the same CRDs. The documentation and ADR-107 changes describe the same install, upgrade, and adoption paths. TheapiextensionsGroupand Helm managed-by constant refactors injobset_ownership.goandrecovery_inventory.goare small supporting changes. The release-size test intest/helm/release_size_test.gochecks that both charts stay within the Helm Secret size budget that the chart split depends on. The.coderabbit.yaml,AGENTS.md, andCLAUDE.mdpath updates follow the moved CRD locations. No demonstrated unrelated behavior change is present.
-
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. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
⚔️ Resolve merge conflicts: 💡
-
- Resolve merge conflict in branch
claude/cluster-readiness-engine-impl-2aacca
- Resolve merge conflict in branch
🧪 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
left a comment
ndipebot
left a comment
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 <kaynet@nvidia.com>
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 <kaynet@nvidia.com>
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)