Skip to content

feat(recipes): backfill CRD Established=True for 6 operator-owning health checks #1919

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
njhensley merged 3 commits into from
Jul 31, 2026
Merged

feat(recipes): backfill CRD Established=True for 6 operator-owning health checks #1919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
68c378a
feat(recipes): backfill CRD Established=True for 6 operator-owning he…
mohityadav8 Jul 26, 2026
eea7665
Merge branch 'main' into feat/1246-backfill-crd-established
njhensley Jul 31, 2026
d2c6910
Merge branch 'main' into feat/1246-backfill-crd-established
njhensley Jul 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 36 additions & 4 deletions recipes/checks/gatekeeper/health-check.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,14 @@
# Gatekeeper Health Check
#
# Validates that gatekeeper-controller-manager and gatekeeper-audit are
# running and healthy in the gatekeeper-system namespace. Guards against
# vacuous pass on empty namespace by asserting both Deployments exist with
# at least one available replica, then asserts no pods are stuck in
# Pending, Failed, or Unknown phases.
# running and healthy in the gatekeeper-system namespace. Also asserts
# the two CRDs the chart installs unconditionally in crds/ —
# constrainttemplates.templates.gatekeeper.sh and
# configs.config.gatekeeper.sh — are Established=True (verified against
# chart 3.22.2; re-verify on defaultVersion bump). Guards against
# vacuous pass on empty namespace by asserting both Deployments exist
# with at least one available replica, then asserts no pods are stuck
# in Pending, Failed, or Unknown phases.
apiVersion: chainsaw.kyverno.io/v1alpha1
kind: Test
metadata:
Expand All @@ -27,6 +31,34 @@ spec:
timeouts:
assert: 5m
steps:
- name: validate-crds-established

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor — The promised inline TODO doesn't exist in the file

The PR body says gatekeeper is "Flagged inline with a TODO to confirm via helm template gatekeeper/gatekeeper --version 3.22.2", and commit 594c904 repeats "flagged inline". Lines 30-49 contain zero comments — the step is bare assert: blocks, and the file header (lines 15-21) was not amended either. kueue's new block (lines 33-36) does carry its provenance comment, so the pattern was known and simply omitted here.

Blast radius: Traceability only — I verified both names are correct (see the summary), so there is no runtime defect. The effect is that the PR's own stated merge precondition is unenforceable and evaporates once the PR description scrolls out of view.

Fix: Since the names are now verified against chart 3.22.2, the better fix is a provenance comment in kueue's style rather than a TODO — e.g. # Verified against chart 3.22.2 (recipes/registry.yaml): both CRDs ship in the chart's unconditional crds/ dir, so no values toggle can suppress them. Re-verify on defaultVersion bump. Alternatively, drop the claim from the PR body.

# Verified against chart 3.22.2 (recipes/registry.yaml): rendered
# `helm template gatekeeper/gatekeeper --version 3.22.2 --include-crds`
# and confirmed both constrainttemplates.templates.gatekeeper.sh and
# configs.config.gatekeeper.sh are present (17 CRDs total; the
# feature-conditional mutation/expansion/external-data CRDs also
# render under default values, confirming these two unconditional
# core CRDs are the correct minimal assertion set). Re-verify on
# defaultVersion bump — tracked systemically in #1948.
try:
- assert:
resource:
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: constrainttemplates.templates.gatekeeper.sh

@njhensley njhensley Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor — gatekeeper's two CRD names are the only asserted names with zero in-tree evidence backing

constrainttemplates.templates.gatekeeper.sh and configs.config.gatekeeper.sh are asserted Established=True, but a repo-wide search finds no in-tree corroboration: no conformance evidence dump, no manifest instantiating a ConstraintTemplate/Config CR, and components/gatekeeper/values.yaml never references either group. Every other check in this PR is anchored by real evidence (kueue manifests, kubeflow assert-crds.yaml + Go lifecycle code, network-operator NicClusterPolicy CR, NIM live kubectl dump).

Blast radius: gatekeeper health check only; a wrong plural/group fails closed on a healthy gatekeeper install (false negative). Practical risk low — both are canonical upstream names installed unconditionally by the chart's crds/ dir. This is a verification-gap/fragility finding, not a confirmed defect.

Fix: Verify both names against the open-policy-agent/gatekeeper 3.22.2 chart crds/ and record the corroboration (a conformance reference or an assert-crds fixture) so the assertion has the same in-tree anchor as its five siblings. Tracked with F2 in #1948.

status:
(conditions[?type == 'Established']):
- status: "True"
- assert:
resource:
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: configs.config.gatekeeper.sh

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nitpick — "cannot function without" is an imprecise filter for the gatekeeper CRD pair

The PR body justifies the pair as "the two CRDs gatekeeper cannot function without, regardless of feature flags". That holds for constrainttemplates.templates.gatekeeper.sh (the ConstraintTemplate controller generates the per-constraint CRDs and is the core admission path), but is imprecise for configs.config.gatekeeper.sh — Gatekeeper enforces policy perfectly well with no Config CR present; the Config kind governs optional sync / exempt-namespace / match behavior.

Blast radius: None functionally — the assertion is harmless and passes, since the CRD ships unconditionally in the chart's crds/ directory (which is also why helm show crds surfaces it: Helm never templates crds/, so no values toggle can suppress it). It matters only because this rationale is what a future maintainer will reuse when deciding which CRDs to assert for the next operator, and "cannot function without" is the wrong filter to inherit.

Fix: Reword to something like "the two CRDs the chart installs unconditionally in crds/" — which is both accurate and the criterion that actually generalizes. Fold into the header comment the finding above already asks for.

status:
(conditions[?type == 'Established']):
- status: "True"
- name: validate-controller-manager-deployment
try:
- assert:
Expand Down
38 changes: 35 additions & 3 deletions recipes/checks/k8s-nim-operator/health-check.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,12 @@
# NIM Operator Health Check
#
# Validates that the NVIDIA NIM Operator is running and healthy in the
# nvidia-nim namespace. Checks that the k8s-nim-operator deployment has
# at least one available replica and that no pods in the namespace are
# stuck in Pending, Failed, or Unknown phases.
# nvidia-nim namespace. Also asserts nimservices / nimcaches /
# nimpipelines.apps.nvidia.com are Established=True (verified against
# chart 3.1.0; re-verify on defaultVersion bump). Checks that the
# k8s-nim-operator deployment has at least one available replica and
# that no pods in the namespace are stuck in Pending, Failed, or
# Unknown phases.
apiVersion: chainsaw.kyverno.io/v1alpha1
kind: Test
metadata:
Expand All @@ -26,6 +29,35 @@ spec:
timeouts:
assert: 5m
steps:
- name: validate-crds-established

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor — File headers left stale after the first step changed (5 files)

The convention in the checks PR #1245 landed is that the file header enumerates every step the file performs: gpu-operator lines 18-21 list the CRD assert as numbered item 2, cert-manager lines 26-28 say "Also asserts the three core CRDs are Established=True...", nodewright-operator and dynamo-platform likewise as item 1. Here all five touched headers still describe only deployment-replica + pod-phase checks while the file's first step is now a CRD assert — k8s-nim-operator (15-20), gatekeeper (15-21), kubeflow-trainer (15-20), network-operator (15-20), and kueue (15-22, which got an inline block comment but no header update).

Blast radius: Documentation drift, zero runtime effect. The practical cost is on chart bumps: CRD plural names are chart-version-coupled, and nothing in-tree tells a Renovate bumper of k8s-nim-operator 3.1.0 / kubeflow-trainer 2.2.0 / network-operator 26.1.1 / gatekeeper 3.22.2 that these asserts must be re-verified.

Fix: Extend each file's header to list the new CRD step, naming the verification source and the pinned chart version — matching gpu-operator/cert-manager, and the "re-verify on chart bump" note in the slinky-slurm-operator-crds header (lines 21-29).

try:
- assert:
resource:
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: nimservices.apps.nvidia.com
status:
(conditions[?type == 'Established']):
- status: "True"
- assert:
resource:
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: nimcaches.apps.nvidia.com
status:
(conditions[?type == 'Established']):
- status: "True"
- assert:
resource:
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: nimpipelines.apps.nvidia.com
status:
(conditions[?type == 'Established']):
- status: "True"
- name: validate-deployment-exists
try:
# Guard against vacuous pass on empty namespace: verify the
Expand Down
49 changes: 45 additions & 4 deletions recipes/checks/kubeflow-trainer/health-check.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,17 @@

# Kubeflow Trainer Health Check
#
# Validates that the Kubeflow Trainer is running and healthy in the kubeflow
# namespace. Checks that the kubeflow-trainer-controller-manager deployment has at least one
# available replica and that no pods in the namespace are stuck in Pending,
# Failed, or Unknown phases.
# Validates that the Kubeflow Trainer is running and healthy in the
# kubeflow namespace. Also asserts trainjobs.trainer.kubeflow.org,
# trainingruntimes.trainer.kubeflow.org, and
# clustertrainingruntimes.trainer.kubeflow.org are Established=True
# (verified against chart 2.2.0; re-verify on defaultVersion bump) —
# clustertrainingruntimes is load-bearing because platform-kubeflow.yaml
# applies a ClusterTrainingRuntime CR
# (torch-distributed-cluster-training-runtime.yaml) via manifestFiles.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
# Checks that the kubeflow-trainer-controller-manager deployment has at
# least one available replica and that no pods in the namespace are
# stuck in Pending, Failed, or Unknown phases.
apiVersion: chainsaw.kyverno.io/v1alpha1
kind: Test
metadata:
Expand All @@ -26,6 +33,40 @@ spec:
timeouts:
assert: 5m
steps:
- name: validate-crds-established

@njhensley njhensley Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor — CRD-established step runs first and no name was live-validated — a wrong name fails the whole per-component check only after the 5m assert timeout

validate-crds-established is the first step (before deployment/pod checks) and spec.timeouts.assert is 5m. An assert against a non-existent CRD name retries until the 5m timeout, then fails the entire chainsaw Test. Because the PR states none of the 6 names were live-validated, a typo/rename reports the component unhealthy even when the operator is fully functional.

Blast radius: Per-component only — each health-check.yaml is an independent chainsaw Test, so one bad file fails just its own component's gate and per-file revert is trivial. Cost is a 5m stall plus a confident false 'unhealthy' verdict for that component.

Fix: Live-validate the CRD names before merge (the same gate #1245 used), or lower the assert timeout for the CRD-established step so a wrong name fails fast rather than after 5m.

# trainjobs + trainingruntimes + clustertrainingruntimes verified

@yuanchen8911 yuanchen8911 Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification for the one assertion here without in-tree evidence: trainjobs is covered by the kind-cluster chainsaw assert and clustertrainingruntimes is load-bearing via platform-kubeflow's manifestFiles, but trainingruntimes had neither. Rendered oci://ghcr.io/kubeflow/charts/kubeflow-trainer --version 2.2.0 --include-crds: all three asserted CRDs ship (plus jobsets.jobset.x-k8s.io, correctly not asserted). With that, the PR body's "left out pending confirmation" caveat for this component can be dropped — please sync the body with the diff.

@yuanchen8911 yuanchen8911 Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The YAML side of this is done — e7497aa closes out the file-level items and I've resolved those threads. What remains before approval is the PR body sync this comment asked for: the body is now stale in four places and contradicts the diff.

  • The kubeflow-trainer evidence row still cites only the TrainJob kind-cluster assert, and the caveat line still says TrainingRuntime/ClusterTrainingRuntime are "left out pending confirmation" — the diff asserts all three, and the render above confirms all three ship in chart 2.2.0.
  • The gatekeeper evidence row still says "Not yet verified in-tree" with the TODO — contradicted by the verification note e7497aa itself added to the YAML.
  • The Risk Assessment still claims gatekeeper's two CRD names are unconfirmed against the live chart.
  • The Rollout notes still ask for a pre-merge helm-template verification pass — that gate has been satisfied.

Please update the body to describe the PR's current state.

One process nit: the branch caught up with main via a merge commit (f363931); per the repo's branch-hygiene guidance please rebase onto origin/main and squash before the final push.

@mohityadav8 mohityadav8 Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuanchen8911 done

@yuanchen8911 yuanchen8911 Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Body sync verified, and the squash preserved the reviewed content exactly (all six health-check files byte-identical to e7497aa).

One new blocker: the squash was built on a stale base, so the diff now reverts .settings.yaml's awscli pin from 1.45.57 to 1.45.56 — undoing #1975, merged this morning. Please rebase onto origin/main (the stale hunk drops out), squash, and force-push — that also clears the merge-commit nit. Will approve once pushed.

@mohityadav8 mohityadav8 Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuanchen8911 done evrthing

# against chart 2.2.0 (recipes/registry.yaml). clustertrainingruntimes
# is the CRD platform-kubeflow.yaml actually instantiates a CR
# against — see the kubeflow-trainer entry's manifestFiles block
# in registry.yaml.
try:
- assert:
resource:
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: trainjobs.trainer.kubeflow.org
status:
(conditions[?type == 'Established']):
- status: "True"
- assert:
resource:
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: clustertrainingruntimes.trainer.kubeflow.org
status:
(conditions[?type == 'Established']):
- status: "True"
- assert:
resource:
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: trainingruntimes.trainer.kubeflow.org
status:
(conditions[?type == 'Established']):
- status: "True"
- name: validate-deployment-exists
try:
# Guard against vacuous pass on empty namespace: verify the
Expand Down
46 changes: 41 additions & 5 deletions recipes/checks/kueue/health-check.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,14 @@
# Kueue Health Check
#
# Validates that Kueue is running and healthy in the kueue-system
# namespace, and that the default quota CRs shipped with the component
# (ResourceFlavor default-flavor, ClusterQueue cluster-queue, LocalQueue
# default/default) were admitted and reconciled to Active — a functional
# proof (CRDs installed, controller reconciling, and flavor/queue resolution)
# within the assert/error-only allowlist.
# namespace. Asserts resourceflavors / clusterqueues /
# localqueues.kueue.x-k8s.io are Established=True (verified against
# chart 0.18.2; re-verify on defaultVersion bump), and that the default
# quota CRs shipped with the component (ResourceFlavor default-flavor,
# ClusterQueue cluster-queue, LocalQueue default/default) were admitted
# and reconciled to Active — a functional proof (CRDs installed,
# controller reconciling, and flavor/queue resolution) within the
# assert/error-only allowlist.
apiVersion: chainsaw.kyverno.io/v1alpha1
kind: Test
metadata:
Expand All @@ -28,6 +31,39 @@ spec:
timeouts:
assert: 5m
steps:
- name: validate-crds-established
try:
# kueue.x-k8s.io group verified in-tree: the manifests this
# component ships (recipes/components/kueue/manifests/*.yaml)
# use apiVersion kueue.x-k8s.io/v1beta2 for ResourceFlavor,
# ClusterQueue, and LocalQueue. Chart 0.18.2 (recipes/registry.yaml).
- assert:
resource:
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: resourceflavors.kueue.x-k8s.io
status:
(conditions[?type == 'Established']):
- status: "True"
- assert:
resource:
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: clusterqueues.kueue.x-k8s.io
status:
(conditions[?type == 'Established']):
- status: "True"
- assert:
resource:
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: localqueues.kueue.x-k8s.io
status:
(conditions[?type == 'Established']):
- status: "True"
- name: validate-deployment-exists
try:
# Guard against vacuous pass on empty namespace: verify the
Expand Down
26 changes: 22 additions & 4 deletions recipes/checks/network-operator/health-check.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,17 @@

# Network Operator Health Check
#
# Validates that the NVIDIA Network Operator is running and healthy in the
# nvidia-network-operator namespace. Checks that the network-operator
# deployment has at least one available replica and that no pods in the
# namespace are stuck in Pending, Failed, or Unknown phases.
# Validates that the NVIDIA Network Operator is running and healthy in
# the nvidia-network-operator namespace. Also asserts
# nicclusterpolicies.mellanox.com is Established=True (verified against
# chart 26.4.1; re-verify on defaultVersion bump). Chart 26.4.1 also
# ships nicnodepolicies.mellanox.com, deliberately NOT asserted here:
# no in-tree recipe instantiates a CR against it, so it fails the same
# load-bearing-only bar the rest of this PR's assertions are held to.
# Checks that the
# network-operator deployment has at least one available replica and
# that no pods in the namespace are stuck in Pending, Failed, or
# Unknown phases.
apiVersion: chainsaw.kyverno.io/v1alpha1
kind: Test
metadata:
Expand All @@ -26,6 +33,17 @@ spec:
timeouts:
assert: 5m
steps:
- name: validate-crds-established
try:
- assert:
resource:
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: nicclusterpolicies.mellanox.com
status:
(conditions[?type == 'Established']):
- status: "True"
- name: validate-deployment-exists
try:
# Guard against vacuous pass on empty namespace: verify the
Expand Down
7 changes: 7 additions & 0 deletions recipes/checks/slinky-slurm-operator/health-check.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@
# running and healthy in the `slinky` namespace. Checks that both
# deployments have at least one available replica and that no pods in
# the namespace are stuck in Pending, Failed, or Unknown phases.
#
# No CRD Established=True block here (negative finding for #1246): the
# slinky-slurm-operator chart is deployed with crds.enabled: false (see

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nitpick — Negative-finding comment cites the weaker of two sources

The comment says the chart "is deployed with crds.enabled: false (see recipes/registry.yaml, slinky-slurm-operator entry)". The actual setting is at recipes/components/slinky-slurm-operator/values.yaml:29-30; registry.yaml:703-708 mentions the flag only inside a prose reminder to re-verify it on version bumps. Not a dead pointer, just the weaker of the two sources.

Blast radius: Documentation precision only. I verified the substance of the negative finding is entirely correct: crds.enabled: false is set, and recipes/checks/slinky-slurm-operator-crds/health-check.yaml does assert Established=True for all six slinky.slurm.net CRDs (accountings / controllers / loginsets / nodesets / restapis / tokens). Good call to document the omission rather than leave it silent.

Fix: Point at recipes/components/slinky-slurm-operator/values.yaml as the primary citation, optionally keeping registry.yaml as the secondary reference for the re-verify-on-bump note.

# the `crds.enabled: false` key in
# components/slinky-slurm-operator/values.yaml; registry.yaml's
# slinky-slurm-operator entry only carries the same flag as a prose
# reminder to re-verify it on version bumps).
apiVersion: chainsaw.kyverno.io/v1alpha1
kind: Test
metadata:
Expand Down
Loading