[Draft] feat(nvcf-compute-plane): gang scheduling with KAI MNNVL topology, KAI/Grove primitives - #603
Open
estroz wants to merge 3 commits into
Open
[Draft] feat(nvcf-compute-plane): gang scheduling with KAI MNNVL topology, KAI/Grove primitives#603estroz wants to merge 3 commits into
estroz wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughChangesKAI Scheduler topology integration
Helm validation defaults
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EnvironmentValues
participant Helmfile
participant TopologyChart
participant Kubernetes
participant ClusterAgentPolicy
EnvironmentValues->>Helmfile: provide KAI Scheduler and topology settings
Helmfile->>TopologyChart: render the enabled topology release
TopologyChart->>Kubernetes: apply KAI Topology resources
Helmfile->>ClusterAgentPolicy: add gang-scheduling resource types
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/helm/helm-reval/values.yaml`:
- Around line 86-88: Add regression coverage for the default skip settings
declared near skipValidateObjects, skipValidateImages, and
skipSanitizeObjectMetadata, rendering
deploy/helm/helm-reval/templates/configmap.yaml and asserting all corresponding
skip-* keys and default values are present. If chart tests cannot be added,
document the specific reason in the pull request description.
- Around line 86-88: Update the ReVal skip controls in values.yaml so
skipValidateObjects, skipValidateImages, and skipSanitizeObjectMetadata all
default to false. Add rendered ConfigMap coverage verifying these defaults, and
document the threat model if any opt-out remains supported.
In
`@deploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/templates/topology.yaml`:
- Around line 16-39: The deployment changes lack regression coverage. Add tests
for valid and invalid rendering in topology.yaml, disabled and enabled release
rendering in 01-dependencies.yaml.gotmpl, KAI/Grove type selection including
precedence and deduplication in 02-nvca.yaml.gotmpl, and Makefile distribution
behavior ensuring the local chart is included with its relative path resolved.
Cover
deploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/templates/topology.yaml
lines 16-39,
deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl lines
38-63, deploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmpl lines
73-114, and deploy/stacks/nvcf-compute-plane/Makefile lines 133-134; if tests
cannot be added, document the reason in the pull request.
In `@docs/user/cluster-management/kai-scheduler.md`:
- Around line 133-134: Complete the kubectl apply command in the manifest
application instructions by including the manifest filename, such as
nvcf-mnnvl-topology.yaml.
- Around line 92-96: Update the “Install the Topology” section to document the
command sequence: use `make install` for the initial compute-plane deployment,
then use `make apply` to enable or update the `nvcf-kai-topology` release.
- Around line 109-122: The KAI Scheduler version prerequisite is inconsistent
with the emitted Topology API version. Update the prerequisite documentation at
docs/user/cluster-management/kai-scheduler.md:109-122 and
deploy/stacks/nvcf-compute-plane/README.md:82-89 to require KAI Scheduler
v0.13.0 or later, preserving the kai.scheduler/v1alpha1 API reference.
In `@docs/user/helm-functions.md`:
- Around line 127-130: Update the Deployment behavior note in the Warning
section to state that KAI creates one PodGroup per Deployment replica and copies
the top-owner topology annotations to each PodGroup, allowing per-replica
placement constraints without gang atomicity. Replace the existing
recommendation to use a StatefulSet with this accurate behavior.
In
`@examples/function-samples/helmchart-samples/multi-node-helm-function-test/multi-node-test/templates/statefulset.yaml`:
- Around line 24-27: Move the commented
dra.nvcf.nvidia.io/required-nvlink-domain-index annotation from the StatefulSet
metadata annotations into spec.template.metadata.annotations, keeping the
example value and surrounding comments intact so uncommenting it applies the
annotation to created Pods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c17f1e10-4442-4969-a336-4d43a49c9ea2
⛔ Files ignored due to path filters (62)
deploy/stacks/nvcf-compute-plane/testdata/environments/local.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/component-serviceaccount.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/deployment.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/epp.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/gpu-discovery-preflight.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/gpu-discovery-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/leader-election-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/manager-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/metrics-auth-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/metrics-service.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/operator-config.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/operator-servicemonitor.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/planner.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/profiling-job-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/prometheus.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/regcred-secret.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/serviceaccount.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/validate-values.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/webhook-certificates.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/webhook-configuration.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/webhook-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/webhook-service.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/nats/templates/config-map.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/nats/templates/headless-service.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/nats/templates/pod-disruption-budget.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/nats/templates/service.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/nats/templates/stateful-set.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/templates/kai.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/templates/validate-values.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/clusterrole.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/clusterrolebinding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/clustertopology-validating-webhook-config.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/configmap-operator.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/deployment.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/leaderelection-role.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/leaderelection-rolebinding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/pcs-defaulting-webhook-config.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/pcs-validating-webhook-config.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/service.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/serviceaccount.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-nvcf-kai-topology/nvcf-kai-topology/templates/topology.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/agent-config-merge-cm.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/chart-defaults-nvcfbackend-cm.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/cluster-validator-network-checks-cm.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/crds/nvidia.io_nvcfbackends_crd.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/cronjob.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/custom-annotations-configmap.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/custom-network-policies-configmap.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/deployment.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/gpu-profiling-config-configmap.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/helm-managed-nvcfbackend-cm.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/ngc-service-key.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/nvca-operator_rq.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/pre-delete-cleanup-job.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/pre-delete-cleanup-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/rbac_allowed_extra_types.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/role.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/role_binding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/sa.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/self-managed-nvcfbackend-cm.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/shutdown-sentinel.yamlis excluded by!**/testdata/**
📒 Files selected for processing (13)
deploy/helm/helm-reval/values.yamldeploy/stacks/nvcf-compute-plane/Makefiledeploy/stacks/nvcf-compute-plane/README.mddeploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/Chart.yamldeploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/templates/_helpers.tpldeploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/templates/topology.yamldeploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/values.yamldeploy/stacks/nvcf-compute-plane/environments/base.yamldeploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpldeploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmpldocs/user/cluster-management/kai-scheduler.mddocs/user/helm-functions.mdexamples/function-samples/helmchart-samples/multi-node-helm-function-test/multi-node-test/templates/statefulset.yaml
Comment on lines
+86
to
+88
| skipValidateObjects: true | ||
| skipValidateImages: true | ||
| skipSanitizeObjectMetadata: true |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add regression coverage for the rendered defaults.
Add a Helm render or chart test that verifies the default skip-* keys in deploy/helm/helm-reval/templates/configmap.yaml. If tests are intentionally omitted, explain the reason in the pull request description.
As per coding guidelines: "Code changes must include tests; if tests are omitted, explain why in the Pull Request description."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/helm-reval/values.yaml` around lines 86 - 88, Add regression
coverage for the default skip settings declared near skipValidateObjects,
skipValidateImages, and skipSanitizeObjectMetadata, rendering
deploy/helm/helm-reval/templates/configmap.yaml and asserting all corresponding
skip-* keys and default values are present. If chart tests cannot be added,
document the specific reason in the pull request description.
Source: Coding guidelines
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- values.yaml ---'
cat -n deploy/helm/helm-reval/values.yaml | sed -n '65,105p'
printf '%s\n' '--- configmap.yaml ---'
cat -n deploy/helm/helm-reval/templates/configmap.yaml | sed -n '1,55p'
printf '%s\n' '--- relevant references ---'
rg -n --glob '!vendor/**' --glob '!node_modules/**' \
'skipValidateObjects|skipValidateImages|skipSanitizeObjectMetadata|helm-reval' .
printf '%s\n' '--- changed files ---'
git status --short
git diff --stat
git diff -- deploy/helm/helm-reval/values.yaml deploy/helm/helm-reval/templates/configmap.yamlRepository: NVIDIA/nvcf
Length of output: 22648
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- chart files ---'
git ls-files deploy/helm/helm-reval
printf '%s\n' '--- values structure and chart README ---'
cat -n deploy/helm/helm-reval/values.yaml | sed -n '1,130p'
cat -n deploy/helm/helm-reval/README.md | sed -n '1,220p'
printf '%s\n' '--- runtime config definitions and flag consumers ---'
fd -i 'config|reval' src/control-plane-services/helm-reval/pkg src/control-plane-services/helm-reval/reval \
| head -80
rg -n -C 5 \
'SkipValidateObjects|SkipValidateImages|SkipSanitizeObjectMetadata|skip-validate-objects|skip-validate-images|skip-sanitize-object-metadata|sanitize.*metadata|validate.*object|validate.*image' \
src/control-plane-services/helm-reval
printf '%s\n' '--- recent file history summary ---'
git log -8 --oneline -- deploy/helm/helm-reval/values.yaml deploy/helm/helm-reval/templates/configmap.yamlRepository: NVIDIA/nvcf
Length of output: 30593
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- config loading and default semantics ---'
cat -n src/control-plane-services/helm-reval/pkg/reval/config/config.go | sed -n '1,125p'
rg -n -C 8 'ReadInConfig|Unmarshal|SetDefault|mapstructure|config.yaml|RevalConfig' \
src/control-plane-services/helm-reval/cmd src/control-plane-services/helm-reval/pkg/reval/config
printf '%s\n' '--- validation and sanitization control flow ---'
cat -n src/control-plane-services/helm-reval/reval/run.go | sed -n '470,670p'
cat -n src/control-plane-services/helm-reval/reval/run.go | sed -n '680,735p'
printf '%s\n' '--- history of the three settings ---'
git log --oneline -S'skipValidateObjects' -- deploy/helm/helm-reval/values.yaml
git log --oneline -S'skipValidateImages' -- deploy/helm/helm-reval/values.yaml
git log --oneline -S'skipSanitizeObjectMetadata' -- deploy/helm/helm-reval/values.yamlRepository: NVIDIA/nvcf
Length of output: 40099
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- current revision ---'
git rev-parse --short HEAD
git log -1 --format='%h %s' -- deploy/helm/helm-reval/values.yaml
printf '%s\n' '--- chart migration paths ---'
git show --format='format:%h %s' --name-status --find-renames 404e2558 -- deploy/helm/helm-reval/values.yaml
printf '%s\n' '--- migration diff for the values file ---'
git show --format= --find-renames 404e2558 -- deploy/helm/helm-reval/values.yaml | sed -n '1,180p'
printf '%s\n' '--- chart test coverage ---'
git ls-files deploy/helm | rg '(^|/)(test|tests)/|(_test|test\.)|helm-unittest|ct\.yaml' || true
printf '%s\n' '--- previous chart copies containing these defaults ---'
git grep -n 'skipValidateObjects: true' 404e2558^ -- '*values.yaml' 2>/dev/null || trueRepository: NVIDIA/nvcf
Length of output: 3914
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- PR change for the reviewed lines ---'
git show --format='format:%h %s' --unified=12 034fb289 -- deploy/helm/helm-reval/values.yaml
printf '%s\n' '--- related changes in the same chart ---'
git show --format= --stat 034fb289 -- deploy/helm/helm-reval
git show --format= --unified=6 034fb289 -- deploy/helm/helm-reval | sed -n '1,260p'
printf '%s\n' '--- commit message and changed paths ---'
git show --format=fuller --no-patch 034fb289
git show --format= --name-only 034fb289 | sed -n '1,160p'Repository: NVIDIA/nvcf
Length of output: 12687
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
values = Path("deploy/helm/helm-reval/values.yaml").read_text()
template = Path("deploy/helm/helm-reval/templates/configmap.yaml").read_text()
keys = {
"skipValidateObjects": "skip-validate-objects",
"skipValidateImages": "skip-validate-images",
"skipSanitizeObjectMetadata": "skip-sanitize-object-metadata",
}
for value_key, config_key in keys.items():
value = re.search(rf"^\s+{re.escape(value_key)}:\s*(true|false)\s*$", values, re.M)
condition = re.search(
rf"\.Values\.reval\.serviceConfig\.{re.escape(value_key)}",
template,
)
emitted = re.search(rf"^\s+{re.escape(config_key)}:\s+true\s*$", template, re.M)
assert value and value.group(1) == "true", (value_key, value.group(1) if value else None)
assert condition and emitted, value_key
print(f"{value_key}=true -> {config_key}: true")
print("All three chart defaults enable their corresponding runtime skip flags.")
PYRepository: NVIDIA/nvcf
Length of output: 402
Keep the ReVal skip controls disabled by default.
These values make every default deployment skip object validation, image validation, and metadata sanitization. Restore all three values to false. Add rendered-ConfigMap coverage and document the threat model if opt-out is required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/helm-reval/values.yaml` around lines 86 - 88, Update the ReVal
skip controls in values.yaml so skipValidateObjects, skipValidateImages, and
skipSanitizeObjectMetadata all default to false. Add rendered ConfigMap coverage
verifying these defaults, and document the threat model if any opt-out remains
supported.
Comment on lines
+16
to
+39
| {{- $topologies := .Values.topologies | default list }} | ||
| {{- if empty $topologies }} | ||
| {{- fail "nvcf-kai-topology: topologies must contain at least one entry" }} | ||
| {{- end }} | ||
| {{- range $i, $topo := $topologies }} | ||
| {{- $levels := $topo.levels | default list }} | ||
| {{- if empty $levels }} | ||
| {{- fail (printf "nvcf-kai-topology: topology %q must set at least one level" ($topo.name | default "<unnamed>")) }} | ||
| {{- end }} | ||
| {{- if ne $i 0 }} | ||
| --- | ||
| {{- end }} | ||
| apiVersion: kai.scheduler/v1alpha1 | ||
| kind: Topology | ||
| metadata: | ||
| name: {{ $topo.name | required "nvcf-kai-topology: every topology must set name" }} | ||
| labels: | ||
| {{- include "nvcf-kai-topology.labels" $ | nindent 4 }} | ||
| spec: | ||
| levels: | ||
| {{- range $levels }} | ||
| - nodeLabel: {{ .nodeLabel | required "nvcf-kai-topology: every level must set nodeLabel" | quote }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add regression coverage for the deployment path.
The PR changes chart rendering, Helmfile release selection, NVCA permissions, and distribution contents without adding tests.
deploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/templates/topology.yaml#L16-L39: test valid rendering and validation failures.deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl#L38-L63: test disabled and enabled topology release rendering.deploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmpl#L73-L114: test KAI and Grove type selection, precedence, and deduplication.deploy/stacks/nvcf-compute-plane/Makefile#L133-L134: test that the distribution includes the local chart and resolves its relative path.
As per coding guidelines, code changes must include tests; if tests are omitted, explain why in the Pull Request description.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 16-16: syntax error: expected the node content, but found '-'
(syntax)
📍 Affects 4 files
deploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/templates/topology.yaml#L16-L39(this comment)deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl#L38-L63deploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmpl#L73-L114deploy/stacks/nvcf-compute-plane/Makefile#L133-L134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@deploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/templates/topology.yaml`
around lines 16 - 39, The deployment changes lack regression coverage. Add tests
for valid and invalid rendering in topology.yaml, disabled and enabled release
rendering in 01-dependencies.yaml.gotmpl, KAI/Grove type selection including
precedence and deduplication in 02-nvca.yaml.gotmpl, and Makefile distribution
behavior ensuring the local chart is included with its relative path resolved.
Cover
deploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/templates/topology.yaml
lines 16-39,
deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl lines
38-63, deploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmpl lines
73-114, and deploy/stacks/nvcf-compute-plane/Makefile lines 133-134; if tests
cannot be added, document the reason in the pull request.
Source: Coding guidelines
Comment on lines
+92
to
+96
| ### Install the Topology | ||
|
|
||
| The compute plane stack ships this as an optional add-on. Enable it in | ||
| `environments/<env>.yaml` and run `make apply`: | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 3 '^(apply|install):|make (apply|install)' \
deploy/stacks/nvcf-compute-plane/Makefile \
deploy/stacks/nvcf-compute-plane/README.md \
docs/user/cluster-management/kai-scheduler.mdRepository: NVIDIA/nvcf
Length of output: 3945
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Makefile target definitions and recipes ---'
rg -n -C 8 '^[A-Za-z0-9_.%/-]+:|(^|\s)(apply|install)(\s|$)' \
deploy/stacks/nvcf-compute-plane/Makefile
printf '%s\n' '--- README optional add-on and deployment sections ---'
sed -n '1,145p' deploy/stacks/nvcf-compute-plane/README.md
printf '%s\n' '--- Kai scheduler documentation ---'
sed -n '80,125p' docs/user/cluster-management/kai-scheduler.mdRepository: NVIDIA/nvcf
Length of output: 13118
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Core target definitions ---'
rg -n -C 12 '^(install|apply|template|register-cluster):|^\.PHONY:.*(install|apply)' \
deploy/stacks/nvcf-compute-plane/Makefile.dist
printf '%s\n' '--- Included variables and Helmfile invocation ---'
sed -n '1,180p' deploy/stacks/nvcf-compute-plane/Makefile.distRepository: NVIDIA/nvcf
Length of output: 13914
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Complete apply target ---'
sed -n '190,225p' deploy/stacks/nvcf-compute-plane/Makefile.dist
printf '%s\n' '--- Topology configuration and release wiring ---'
rg -n -C 5 'clusterTopologies|kaiScheduler|Topology|helmfile apply|helmfile sync' \
deploy/stacks/nvcf-compute-plane \
docs/user/cluster-management/kai-scheduler.mdRepository: NVIDIA/nvcf
Length of output: 32251
Document the initial and update commands.
Use make install for the initial compute-plane deployment. Use make apply to enable or update the nvcf-kai-topology release after deployment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/user/cluster-management/kai-scheduler.md` around lines 92 - 96, Update
the “Install the Topology” section to document the command sequence: use `make
install` for the initial compute-plane deployment, then use `make apply` to
enable or update the `nvcf-kai-topology` release.
Comment on lines
+109
to
+122
| The add-on requires KAI Scheduler v0.12.0 or later, which is when the native | ||
| `kai.scheduler/v1alpha1` Topology CRD was introduced. For each entry under | ||
| `topologies` it creates a cluster-scoped resource such as: | ||
|
|
||
| ```yaml | ||
| apiVersion: kai.scheduler/v1alpha1 | ||
| kind: Topology | ||
| metadata: | ||
| name: nvcf-mnnvl-topology | ||
| spec: | ||
| levels: | ||
| - nodeLabel: nvidia.com/gpu.clique | ||
| - nodeLabel: kubernetes.io/hostname | ||
| ``` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- docs/user/cluster-management/kai-scheduler.md ---'
sed -n '1,180p' docs/user/cluster-management/kai-scheduler.md
printf '%s\n' '--- deploy/stacks/nvcf-compute-plane/README.md ---'
sed -n '60,105p' deploy/stacks/nvcf-compute-plane/README.md
printf '%s\n' '--- references to Topology and KAI versions ---'
rg -n -C 3 'Topology|topolog|kai.scheduler/v1|kai.scheduler/v1alpha1|v0\.1[23]\.0' \
docs/user/cluster-management/kai-scheduler.md \
deploy/stacks/nvcf-compute-plane/README.md \
deploy tools charts 2>/dev/null || true
printf '%s\n' '--- upstream tag documentation ---'
for tag in v0.12.0 v0.13.0; do
printf '%s\n' "### $tag"
url="https://raw.githubusercontent.com/kai-scheduler/KAI-Scheduler/$tag/docs/topology/README.md"
curl -fsSL "$url" | rg -n -C 3 'apiVersion|Topology|nodeLabel' || printf '%s\n' "No matching content or tag path: $url"
doneRepository: NVIDIA/nvcf
Length of output: 50370
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- repository templates and dependency versions ---'
rg -n -C 4 'apiVersion: kai\.scheduler/|kind: Topology|kaiScheduler|clusterTopologies|kai-scheduler' \
deploy/stacks/nvcf-compute-plane \
--glob '!testdata/golden/**' \
--glob '!*.lock' | head -250
printf '%s\n' '--- KAI v0.12.0 topology document ---'
curl -fsSL \
'https://api.github.com/repos/KAI-Scheduler/KAI-Scheduler/contents/docs/topology/README.md?ref=v0.12.0' |
jq -r '.download_url // .message'
url12='https://raw.githubusercontent.com/KAI-Scheduler/KAI-Scheduler/v0.12.0/docs/topology/README.md'
curl -fsSL "$url12" |
rg -n -C 5 'apiVersion|kind: Topology|nodeLabel|v1alpha1|scheduler/v1' || true
printf '%s\n' '--- KAI v0.13.0 topology document ---'
url13='https://raw.githubusercontent.com/KAI-Scheduler/KAI-Scheduler/v0.13.0/docs/topology/README.md'
curl -fsSL "$url13" |
rg -n -C 5 'apiVersion|kind: Topology|nodeLabel|v1alpha1|scheduler/v1' || true
printf '%s\n' '--- KAI tag refs and commits ---'
for tag in v0.12.0 v0.13.0 v0.14.0; do
curl -fsSL "https://api.github.com/repos/KAI-Scheduler/KAI-Scheduler/git/ref/tags/$tag" |
jq -r '"'"$tag"' + ": type=" + .object.type + " sha=" + .object.sha'
doneRepository: NVIDIA/nvcf
Length of output: 22907
🏁 Script executed:
#!/bin/sh
set -eu
python3 - <<'PY'
import re
from pathlib import Path
from urllib.request import Request, urlopen
template = Path("deploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/templates/topology.yaml").read_text()
docs = Path("docs/user/cluster-management/kai-scheduler.md").read_text()
readme = Path("deploy/stacks/nvcf-compute-plane/README.md").read_text()
def first_api(text, label):
match = re.search(r"apiVersion:\s*([^\s]+)", text)
if not match:
raise SystemExit(f"{label}: no apiVersion found")
return match.group(1)
print("local chart Topology apiVersion:", first_api(template, "local chart"))
print("guide documented Topology apiVersion:",
re.search(r"apiVersion:\s*(kai\.scheduler/\S+)", docs).group(1))
print("guide prerequisite:", re.search(r"requires KAI Scheduler (v[0-9.]+) or later", docs).group(1))
print("README prerequisite:", re.search(r"requires KAI Scheduler (v[0-9.]+) or later", readme).group(1))
for tag in ("v0.12.0", "v0.13.0"):
url = f"https://raw.githubusercontent.com/KAI-Scheduler/KAI-Scheduler/{tag}/docs/topology/README.md"
req = Request(url, headers={"User-Agent": "CodeRabbit-review"})
with urlopen(req, timeout=20) as response:
text = response.read().decode()
print(f"{tag} upstream Topology apiVersion:", first_api(text, tag))
PYRepository: NVIDIA/nvcf
Length of output: 427
🏁 Script executed:
#!/bin/sh
set -eu
python3 - <<'PY'
import re
from pathlib import Path
from urllib.request import Request, urlopen
template = Path("deploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/templates/topology.yaml").read_text()
guide = Path("docs/user/cluster-management/kai-scheduler.md").read_text()
readme = Path("deploy/stacks/nvcf-compute-plane/README.md").read_text()
def api_version(text, label):
match = re.search(r"apiVersion:\s*([^\s]+)", text)
if not match:
raise SystemExit(f"{label}: no apiVersion found")
return match.group(1)
def prerequisite(text, label):
match = re.search(r"requires KAI Scheduler\s+(v[0-9.]+)\s+or\s+later", text)
if not match:
raise SystemExit(f"{label}: prerequisite not found")
return match.group(1)
print("local chart Topology apiVersion:", api_version(template, "local chart"))
print("guide documented Topology apiVersion:", api_version(guide, "guide"))
print("guide prerequisite:", prerequisite(guide, "guide"))
print("README prerequisite:", prerequisite(readme, "README"))
for tag in ("v0.12.0", "v0.13.0"):
url = f"https://raw.githubusercontent.com/KAI-Scheduler/KAI-Scheduler/{tag}/docs/topology/README.md"
request = Request(url, headers={"User-Agent": "CodeRabbit-review"})
with urlopen(request, timeout=20) as response:
text = response.read().decode()
print(f"{tag} upstream Topology apiVersion:", api_version(text, tag))
PYRepository: NVIDIA/nvcf
Length of output: 2610
Use KAI Scheduler v0.13.0 or later for the Topology add-on.
The chart emits kai.scheduler/v1alpha1, but KAI v0.12.0 documents kai.scheduler/v1. Update both prerequisites, or render kai.scheduler/v1 for v0.12.0.
📍 Affects 2 files
docs/user/cluster-management/kai-scheduler.md#L109-L122(this comment)deploy/stacks/nvcf-compute-plane/README.md#L82-L89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/user/cluster-management/kai-scheduler.md` around lines 109 - 122, The
KAI Scheduler version prerequisite is inconsistent with the emitted Topology API
version. Update the prerequisite documentation at
docs/user/cluster-management/kai-scheduler.md:109-122 and
deploy/stacks/nvcf-compute-plane/README.md:82-89 to require KAI Scheduler
v0.13.0 or later, preserving the kai.scheduler/v1alpha1 API reference.
Comment on lines
+133
to
+134
| To apply it without the compute plane stack, write the manifest above to a file and run | ||
| `kubectl apply -f`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="docs/user/cluster-management/kai-scheduler.md"
printf '%s\n' '--- target lines ---'
sed -n '115,145p' "$file"
printf '%s\n' '--- nearby manifest and kubectl references ---'
rg -n -C 3 'kubectl apply|nvcf-mnnvl|topology|manifest' "$file"
printf '%s\n' '--- repository references to the same manifest ---'
rg -n -C 2 'nvcf-mnnvl-topology|kai-scheduler' docs tools .github 2>/dev/null | head -n 240Repository: NVIDIA/nvcf
Length of output: 29726
Complete the kubectl apply command. Add the manifest filename, for example: kubectl apply -f nvcf-mnnvl-topology.yaml.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/user/cluster-management/kai-scheduler.md` around lines 133 - 134,
Complete the kubectl apply command in the manifest application instructions by
including the manifest filename, such as nvcf-mnnvl-topology.yaml.
Comment on lines
+127
to
+130
| <Warning> | ||
| Gang scheduling this way only works for StatefulSets. KAI Scheduler puts every replica of a | ||
| Deployment in its own `PodGroup`, so a Deployment cannot be gang scheduled and this | ||
| annotation has no effect there. Use a StatefulSet for multi-node workloads. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- target file outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline docs/user/helm-functions.md || true
fi
echo '--- target file context ---'
cat -n docs/user/helm-functions.md | sed -n '105,145p'
echo '--- KAI and topology references ---'
rg -n -i --glob '!node_modules' --glob '!dist' --glob '!build' \
'KAI|PodGroup|gang schedul|topology|StatefulSet|Deployment' . | head -300Repository: NVIDIA/nvcf
Length of output: 46499
🏁 Script executed:
#!/bin/bash
set -eu
base='https://raw.githubusercontent.com/kai-scheduler/KAI-Scheduler/v0.14.0/pkg/podgrouper/podgrouper/plugins/deployment'
echo '--- deployment grouper ---'
curl --fail --silent --show-error "$base/deployment_grouper.go" | nl -ba | sed -n '1,260p'
echo '--- source references in deployment grouper ---'
curl --fail --silent --show-error "$base/deployment_grouper.go" |
grep -nE 'annotation|topology|PodGroup|owner|group|label' || true
echo '--- GitHub API metadata for cited file ---'
curl --fail --silent --show-error \
'https://api.github.com/repos/kai-scheduler/KAI-Scheduler/contents/pkg/podgrouper/podgrouper/plugins/deployment?ref=v0.14.0' |
jq -r '.[] | [.name, .type, .download_url] | `@tsv`'Repository: NVIDIA/nvcf
Length of output: 1312
🏁 Script executed:
#!/bin/bash
set -eu
repo='https://raw.githubusercontent.com/kai-scheduler/KAI-Scheduler/v0.14.0'
echo '--- deployment_grouper.go ---'
curl --fail --silent --show-error \
"$repo/pkg/podgrouper/podgrouper/plugins/deployment/deployment_grouper.go" |
cat -n
echo '--- default grouper candidates ---'
for path in \
pkg/podgrouper/podgrouper/plugins/defaultgrouper/default_grouper.go \
pkg/podgrouper/podgrouper/plugins/defaultgrouper/default_grouper_test.go \
pkg/podgrouper/podgrouper/plugins/defaultgrouper/defaultgrouper.go \
pkg/podgrouper/podgrouper/plugins/defaultgrouper/defaultgrouper_test.go
do
echo "### $path"
curl --fail --silent --show-error "$repo/$path" | cat -n || true
done
echo '--- upstream source search via repository tarball listing ---'
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
curl --fail --silent --show-error \
-L 'https://github.com/kai-scheduler/KAI-Scheduler/archive/refs/tags/v0.14.0.tar.gz' |
tar -xz -C "$tmpdir"
root="$(find "$tmpdir" -mindepth 1 -maxdepth 1 -type d | head -1)"
grep -RInE 'GetPodGroupMetadata|topology-required-placement|kai\.scheduler/topology|PodGroup' \
"$root/pkg/podgrouper" | head -240Repository: NVIDIA/nvcf
Length of output: 50368
Describe Deployment behavior accurately.
A Deployment gets one PodGroup per replica. KAI copies top-owner topology annotations to each PodGroup, so the annotations can constrain per-replica placement but do not provide gang atomicity. Replace the last sentence with this behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/user/helm-functions.md` around lines 127 - 130, Update the Deployment
behavior note in the Warning section to state that KAI creates one PodGroup per
Deployment replica and copies the top-owner topology annotations to each
PodGroup, allowing per-replica placement constraints without gang atomicity.
Replace the existing recommendation to use a StatefulSet with this accurate
behavior.
Comment on lines
24
to
+27
| annotations: | ||
| # dra.nvcf.nvidia.io/required-nvlink-domain-index: "0" # force all nodes to use the same NVLink domain | ||
| # MNNVL scheduling on an NVLink-optimized cluster without KAI Scheduler. | ||
| # | ||
| # dra.nvcf.nvidia.io/required-nvlink-domain-index: "0" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Move the non-KAI annotation to the Pod template.
Line [27] places dra.nvcf.nvidia.io/required-nvlink-domain-index under the StatefulSet's metadata.annotations. The Helm Functions guide documents this as a Pod annotation under spec.template.metadata.annotations. If a user uncomments Line [27], the Pods do not receive the annotation, so the non-KAI affinity path does not apply. Move this example under the Pod template.
The Helm Functions guide is the source of truth for this annotation scope.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@examples/function-samples/helmchart-samples/multi-node-helm-function-test/multi-node-test/templates/statefulset.yaml`
around lines 24 - 27, Move the commented
dra.nvcf.nvidia.io/required-nvlink-domain-index annotation from the StatefulSet
metadata annotations into spec.template.metadata.annotations, keeping the
example value and surrounding comments intact so uncommenting it applies the
annotation to created Pods.
Signed-off-by: Eric Stroczynski <estroczynski@nvidia.com>
4 tasks
…I/Grove primitives Signed-off-by: Eric Stroczynski <estroczynski@nvidia.com>
Signed-off-by: Eric Stroczynski <estroczynski@nvidia.com>
estroz
force-pushed
the
estroczynski/feat/mnnvl-scheduling
branch
from
July 31, 2026 21:15
034fb28 to
e2f4e4e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl (1)
142-153: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConfirm that disabling consolidation matches the gang-placement objective.
The PR objective is atomic placement of a full worker group inside one NVLink partition.
actions.consolidation.enabled: falseremoves the defragmentation step that frees a contiguous block for a pending PodGroup. On a fragmented cluster, gang requests then stay pending longer. If this is intentional for stability, state the reason in the comment block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl` around lines 142 - 153, Confirm the intended gang-placement behavior in the scheduler configuration around actions.consolidation.enabled. Either enable consolidation so fragmented clusters can free contiguous capacity for a full worker group, or, if disabling it is intentional for stability, add a nearby comment documenting that rationale and its tradeoff.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ai-tooling/user/skills/nvcf-self-managed-prerequisite/SKILL.md`:
- Around line 85-86: Update the uninstall section in the prerequisite workflow
to skip standalone KAI Scheduler removal when nvcf-compute-plane has
addons.kaiScheduler.enabled set to true, preserving the compute-plane-managed
kai-scheduler release and namespace. Run the existing uninstall commands only
for standalone installations, and document or invoke the separate compute-plane
teardown path when the add-on owns KAI.
- Line 38: Update the KAI Scheduler prerequisite wording to say the standalone
Helm step should be skipped only when addons.kaiScheduler.enabled is true,
matching the Helmfile condition and preserving installation when the value is
false.
In `@deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl`:
- Around line 106-196: Remove the entire numaPlacementExporter configuration
block, including its resources requests and limits, from the dependency values
while leaving the supported operator components and scheduler configuration
unchanged.
- Around line 248-252: Make the needs dependency for the nvcf-kai-topology
release conditional on addons.kaiScheduler.enabled, so it is emitted only when
Helmfile manages the KAI installation; preserve the existing
kai-scheduler/kai-scheduler ordering when enabled, and omit it for standalone or
disabled KAI deployments.
In `@deploy/stacks/nvcf-compute-plane/Makefile.dist`:
- Line 38: Update the NAMESPACES definition to include kai-resource-reservation
so the destroy target removes it explicitly. Also document the required manual
cleanup of KAI CRDs and retained cluster-scoped resources, or add a guarded
cleanup step for those resources without affecting existing destroy behavior.
In `@docs/user/helm-functions.md`:
- Around line 109-116: Update the StatefulSet example by adding
spec.template.spec.schedulerName with the value kai-scheduler, ensuring Pods use
KAI for the documented topology and gang-scheduling behavior.
---
Nitpick comments:
In `@deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl`:
- Around line 142-153: Confirm the intended gang-placement behavior in the
scheduler configuration around actions.consolidation.enabled. Either enable
consolidation so fragmented clusters can free contiguous capacity for a full
worker group, or, if disabling it is intentional for stability, add a nearby
comment documenting that rationale and its tradeoff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e5a8a04f-dee2-450b-819d-765c2fafc567
⛔ Files ignored due to path filters (104)
deploy/stacks/nvcf-compute-plane/testdata/environments/local.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/component-serviceaccount.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/deployment.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/epp.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/gpu-discovery-preflight.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/gpu-discovery-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/leader-election-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/manager-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/metrics-auth-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/metrics-service.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/operator-config.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/operator-servicemonitor.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/planner.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/profiling-job-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/prometheus.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/regcred-secret.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/serviceaccount.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/validate-values.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/webhook-certificates.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/webhook-configuration.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/webhook-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/dynamo-operator/templates/webhook-service.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/nats/templates/config-map.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/nats/templates/headless-service.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/nats/templates/pod-disruption-budget.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/nats/templates/service.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/charts/nats/templates/stateful-set.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/templates/kai.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-dynamo-operator/dynamo-platform/templates/validate-values.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/clusterrole.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/clusterrolebinding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/clustertopology-validating-webhook-config.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/configmap-operator.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/deployment.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/leaderelection-role.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/leaderelection-rolebinding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/pcs-defaulting-webhook-config.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/pcs-validating-webhook-config.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/service.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-grove-operator/grove-charts/templates/serviceaccount.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/default-queue.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/default-shard.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/hooks/post/post-delete-job.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/hooks/pre/crd-upgrader.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/hooks/pre/topology-migration/configmap.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/hooks/pre/topology-migration/job.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/hooks/pre/topology-migration/rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/kai-config.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/priorityclasses/build-preemptible.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/priorityclasses/build.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/priorityclasses/inference.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/priorityclasses/train.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/admission-binding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/admission.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/binder-binding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/binder.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/crd-manager.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/nodescaleadjuster-binding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/nodescaleadjuster.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/operator-binding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/operator.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/podgroupcontroller-binding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/podgroupcontroller.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/podgrouper-binding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/podgrouper.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/post-delete-binding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/post-delete-clusterrole.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/prometheus-binding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/prometheus-pod.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/queuecontroller-binding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/queuecontroller.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/resourcereservation-binding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/resourcereservation.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/scc.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/scheduler-binding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/rbac/scheduler.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/services/operator-serviceaccount.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/services/operator.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/services/post-delete-serviceaccount.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/services/resourcereservation-namespace.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/services/resourcereservation-serviceaccount.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/services/scalingpod-namespace.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-nvcf-kai-topology/nvcf-kai-topology/templates/topology.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/agent-config-merge-cm.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/chart-defaults-nvcfbackend-cm.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/cluster-validator-network-checks-cm.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/crds/nvidia.io_nvcfbackends_crd.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/cronjob.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/custom-annotations-configmap.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/custom-network-policies-configmap.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/deployment.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/gpu-profiling-config-configmap.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/helm-managed-nvcfbackend-cm.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/ngc-service-key.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/nvca-operator_rq.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/pre-delete-cleanup-job.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/pre-delete-cleanup-rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/rbac.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/rbac_allowed_extra_types.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/role.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/role_binding.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/sa.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/self-managed-nvcfbackend-cm.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/shutdown-sentinel.yamlis excluded by!**/testdata/**
📒 Files selected for processing (16)
ai-tooling/user/skills/nvcf-self-managed-prerequisite/SKILL.mdai-tooling/user/skills/nvcf-self-managed-prerequisite/references/kai-scheduler.mddeploy/helm/helm-reval/values.yamldeploy/stacks/nvcf-compute-plane/Makefiledeploy/stacks/nvcf-compute-plane/Makefile.distdeploy/stacks/nvcf-compute-plane/README.mddeploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/Chart.yamldeploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/templates/_helpers.tpldeploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/templates/topology.yamldeploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/values.yamldeploy/stacks/nvcf-compute-plane/environments/base.yamldeploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpldeploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmpldocs/user/cluster-management/kai-scheduler.mddocs/user/helm-functions.mdexamples/function-samples/helmchart-samples/multi-node-helm-function-test/multi-node-test/templates/statefulset.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- deploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/Chart.yaml
- deploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/values.yaml
- deploy/helm/helm-reval/values.yaml
- deploy/stacks/nvcf-compute-plane/charts/nvcf-kai-topology/templates/_helpers.tpl
- deploy/stacks/nvcf-compute-plane/environments/base.yaml
- examples/function-samples/helmchart-samples/multi-node-helm-function-test/multi-node-test/templates/statefulset.yaml
- deploy/stacks/nvcf-compute-plane/README.md
- docs/user/cluster-management/kai-scheduler.md
| | ------ | ------------------ | ------ | | ||
| | `nvcf-cli` | The compute-plane stack's `make register-cluster` (and `install`/`apply`/`sync`, which abort without the registration values it writes) shells out to `nvcf-cli`. The shipped stack defaults to building it from a sibling `../cli` checkout that the release does not include, so a green-field repo fails with `ensure-nvcf-cli` / "Registration values not found". | See Step 0b below | | ||
| | KAI Scheduler | `selfManaged.featureGateValues` includes `KAIScheduler`; NVCA polls `Queue` CRs and refuses to become healthy until their quotas are `-1` | [references/kai-scheduler.md](references/kai-scheduler.md) | | ||
| | KAI Scheduler | `selfManaged.featureGateValues` includes `KAIScheduler`; NVCA polls `Queue` CRs and refuses to become healthy until their quotas are `-1`. Skip the standalone helm step below when `addons.kaiScheduler.enabled` is set in `nvcf-compute-plane` (required when enabling Grove or Dynamo). | [references/kai-scheduler.md](references/kai-scheduler.md) | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the boolean value in the installation condition.
Change “is set” to “is true”. The Helmfile uses condition: addons.kaiScheduler.enabled, so false is also a valid configured value. The current wording can cause an operator to skip standalone installation while the compute-plane add-on remains disabled.
Proposed wording
-| KAI Scheduler | ... Skip the standalone helm step below when `addons.kaiScheduler.enabled` is set in `nvcf-compute-plane` ...
+| KAI Scheduler | ... Skip the standalone helm step below when `addons.kaiScheduler.enabled` is true in `nvcf-compute-plane` ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | KAI Scheduler | `selfManaged.featureGateValues` includes `KAIScheduler`; NVCA polls `Queue` CRs and refuses to become healthy until their quotas are `-1`. Skip the standalone helm step below when `addons.kaiScheduler.enabled` is set in `nvcf-compute-plane` (required when enabling Grove or Dynamo). | [references/kai-scheduler.md](references/kai-scheduler.md) | | |
| | KAI Scheduler | `selfManaged.featureGateValues` includes `KAIScheduler`; NVCA polls `Queue` CRs and refuses to become healthy until their quotas are `-1`. Skip the standalone helm step below when `addons.kaiScheduler.enabled` is true in `nvcf-compute-plane` (required when enabling Grove or Dynamo). | [references/kai-scheduler.md](references/kai-scheduler.md) | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ai-tooling/user/skills/nvcf-self-managed-prerequisite/SKILL.md` at line 38,
Update the KAI Scheduler prerequisite wording to say the standalone Helm step
should be skipped only when addons.kaiScheduler.enabled is true, matching the
Helmfile condition and preserving installation when the value is false.
Comment on lines
+85
to
+86
| Skip this step when `addons.kaiScheduler.enabled` is true in `nvcf-compute-plane`. That stack installs KAI Scheduler (release and namespace `kai-scheduler`). Enable that flag whenever Grove or Dynamo is enabled. Use the standalone install below when you need KAI without the compute-plane add-on (for example the NVCA `KAIScheduler` feature gate alone). | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Protect the compute-plane-managed KAI release during uninstall.
When addons.kaiScheduler.enabled is true, the compute-plane stack owns the kai-scheduler release in the kai-scheduler namespace. The later unconditional uninstall commands can remove that managed release and namespace. Make the uninstall section conditional on standalone installation, or document the separate compute-plane teardown path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ai-tooling/user/skills/nvcf-self-managed-prerequisite/SKILL.md` around lines
85 - 86, Update the uninstall section in the prerequisite workflow to skip
standalone KAI Scheduler removal when nvcf-compute-plane has
addons.kaiScheduler.enabled set to true, preserving the compute-plane-managed
kai-scheduler release and namespace. Run the existing uninstall commands only
for standalone installations, and document or invoke the separate compute-plane
teardown path when the add-on owns KAI.
Comment on lines
+106
to
+196
| operator: | ||
| resources: | ||
| requests: | ||
| cpu: {{ dig "cpu" "50m" $opReq }} | ||
| memory: {{ dig "memory" "256Mi" $opReq }} | ||
| limits: | ||
| cpu: {{ dig "cpu" "1000m" $opLim }} | ||
| memory: {{ dig "memory" "1Gi" $opLim }} | ||
|
|
||
| podgrouper: | ||
| resources: | ||
| requests: | ||
| cpu: {{ dig "cpu" "50m" $pgReq }} | ||
| memory: {{ dig "memory" "512Mi" $pgReq }} | ||
| limits: | ||
| cpu: {{ dig "cpu" "1000m" $pgLim }} | ||
| memory: {{ dig "memory" "4Gi" $pgLim }} | ||
|
|
||
| podgroupcontroller: | ||
| resources: | ||
| requests: | ||
| cpu: {{ dig "cpu" "50m" $pgcReq }} | ||
| memory: {{ dig "memory" "256Mi" $pgcReq }} | ||
| limits: | ||
| cpu: {{ dig "cpu" "1000m" $pgcLim }} | ||
| memory: {{ dig "memory" "1Gi" $pgcLim }} | ||
|
|
||
| binder: | ||
| resources: | ||
| requests: | ||
| cpu: {{ dig "cpu" "50m" $bdReq }} | ||
| memory: {{ dig "memory" "2Gi" $bdReq }} | ||
| limits: | ||
| cpu: {{ dig "cpu" "4000m" $bdLim }} | ||
| memory: {{ dig "memory" "4Gi" $bdLim }} | ||
|
|
||
| scheduler: | ||
| placementStrategy: binpack | ||
| plugins: | ||
| nodeplacement: | ||
| arguments: | ||
| gpu: binpack | ||
| cpu: spread | ||
| actions: | ||
| preempt: | ||
| enabled: false | ||
| consolidation: | ||
| enabled: false | ||
| resources: | ||
| requests: | ||
| cpu: {{ dig "cpu" "500m" $schReq }} | ||
| memory: {{ dig "memory" "3Gi" $schReq }} | ||
| limits: | ||
| cpu: {{ dig "cpu" "2000m" $schLim }} | ||
| memory: {{ dig "memory" "6Gi" $schLim }} | ||
|
|
||
| queuecontroller: | ||
| resources: | ||
| requests: | ||
| cpu: {{ dig "cpu" "50m" $qcReq }} | ||
| memory: {{ dig "memory" "256Mi" $qcReq }} | ||
| limits: | ||
| cpu: {{ dig "cpu" "1000m" $qcLim }} | ||
| memory: {{ dig "memory" "1Gi" $qcLim }} | ||
|
|
||
| admission: | ||
| resources: | ||
| requests: | ||
| cpu: {{ dig "cpu" "50m" $adReq }} | ||
| memory: {{ dig "memory" "256Mi" $adReq }} | ||
| limits: | ||
| cpu: {{ dig "cpu" "1000m" $adLim }} | ||
| memory: {{ dig "memory" "1Gi" $adLim }} | ||
|
|
||
| nodescaleadjuster: | ||
| resources: | ||
| requests: | ||
| cpu: {{ dig "cpu" "50m" $nsaReq }} | ||
| memory: {{ dig "memory" "256Mi" $nsaReq }} | ||
| limits: | ||
| cpu: {{ dig "cpu" "1000m" $nsaLim }} | ||
| memory: {{ dig "memory" "1Gi" $nsaLim }} | ||
|
|
||
| numaPlacementExporter: | ||
| resources: | ||
| requests: | ||
| cpu: {{ dig "cpu" "50m" $npeReq }} | ||
| memory: {{ dig "memory" "64Mi" $npeReq }} | ||
| limits: | ||
| cpu: {{ dig "cpu" "1000m" $npeLim }} | ||
| memory: {{ dig "memory" "1Gi" $npeLim }} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
kai-scheduler helm chart v0.14 values.yaml keys numaPlacementExporter podgroupcontroller defaultQueue scheduler actions
💡 Result:
In the KAI-Scheduler Helm chart (v0.14.0), the values.yaml file provides specific configuration surfaces for the scheduler and its components, including custom plugins and actions. Scheduler Actions and Plugins The scheduler configuration allows for overriding or extending default behavior via the plugins and actions keys: - plugins: This section allows users to disable, reorder, or modify arguments for built-in scheduler plugins, as well as add custom plugins by specifying names not present in the default set [1]. - actions: This section allows for the management of scheduler actions. Users can disable or reorder existing built-in actions or define new ones by specifying their name and priority [1]. NumaPlacementExporter The NumaPlacementExporter is managed as part of the KAI Scheduler operator. Recent updates (introduced around v0.14 timeframe) allow for configurable paths for the podresources socket and sysfs, which are critical for testing and non-standard hardware environments [2][3]. Key configurable fields include: - podResourcesHostPath: Path to the pod resources directory (default: /var/lib/kubelet/pod-resources) [2]. - podResourcesSocket: Path to the kubelet socket (default: /var/lib/kubelet/pod-resources/kubelet.sock) [2]. - sysfsHostPath: Path to the sysfs root (default: /sys) [2]. Podgroupcontroller and DefaultQueue These components are primarily managed via the KAI Scheduler's configuration reconciliation (Config CR). - podgrouper: Managed within the pod controller (e.g., PodReconciler), which uses configs for settings like NodePoolLabelKey and SchedulingQueueLabelKey [4]. - DefaultQueue: The scheduling queue is typically configured via settings such as SchedulingQueueLabelKey, which defines how the system associates pods with specific queues in the cluster [4]. Note: KAI-Scheduler charts template values into a Config Custom Resource (Config CR) that the KAI Operator reconciles [5]. Therefore, many operational parameters (like those for the podgrouper) are defined within the structure of this Config CR, which the Helm chart populates based on the values.yaml configuration [5][6].
Citations:
- 1: https://github.com/NVIDIA/KAI-Scheduler/blob/f909aece/deployments/kai-scheduler/values.yaml
- 2: feat(operator): make numa-placement-exporter podresources socket + sysfs configurable kai-scheduler/KAI-Scheduler#1807
- 3: kai-scheduler/KAI-Scheduler@f6e5979
- 4: https://github.com/NVIDIA/KAI-Scheduler/blob/f909aece/pkg/podgrouper/pod_controller.go
- 5: https://deepwiki.com/NVIDIA/KAI-Scheduler/2.2-helm-chart-installation
- 6: https://deepwiki.com/NVIDIA/KAI-Scheduler/2-installation-and-deployment
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- local template context ---'
sed -n '1,230p' deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl
printf '%s\n' '--- local references ---'
rg -n 'numaPlacementExporter|podgroupcontroller|queuecontroller|nodescaleadjuster|defaultQueue|nodeplacement|actions:|plugins:' .
printf '%s\n' '--- upstream v0.14 chart files ---'
for path in values.yaml templates/_helpers.tpl templates/config.yaml templates/configmap.yaml; do
url="https://raw.githubusercontent.com/NVIDIA/KAI-Scheduler/v0.14.0/deployments/kai-scheduler/$path"
echo "### $url"
curl -LfsS "$url" | sed -n '1,260p' || true
doneRepository: NVIDIA/nvcf
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- chart fixture files ---'
find deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler \
-maxdepth 3 -type f -print | sort
printf '%s\n' '--- rendered component and scheduler sections ---'
for f in \
deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/kai-config.yaml \
deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler/templates/default-shard.yaml; do
echo "### $f"
rg -n -C 8 'name: (operator|podgrouper|podgroupcontroller|binder|scheduler|queuecontroller|admission|nodescaleadjuster|numaPlacementExporter)|defaultQueue|nodeplacement|actions:|plugins:' "$f"
done
printf '%s\n' '--- upstream repository tags and v0.14 paths ---'
curl -LfsS 'https://api.github.com/repos/NVIDIA/KAI-Scheduler/tags?per_page=100' |
jq -r '.[] | .name' | rg '(^|v)0\.14'
for path in \
deployments/kai-scheduler/values.yaml \
deployments/kai-scheduler/Chart.yaml \
deployments/kai-scheduler/templates/default-shard.yaml \
deployments/kai-scheduler/templates/kai-config.yaml \
deployments/kai-scheduler/templates/operator.yaml; do
url="https://api.github.com/repos/NVIDIA/KAI-Scheduler/contents/$path?ref=v0.14.0"
echo "### $url"
curl -LsS "$url" | jq -r 'if .content then (.content | `@base64d`) else (.message // "not found") end' |
rg -n -C 4 'operator:|podgrouper:|podgroupcontroller:|binder:|scheduler:|queuecontroller:|admission:|nodescaleadjuster:|numaPlacementExporter:|defaultQueue:|nodeplacement:|actions:|plugins:' || true
doneRepository: NVIDIA/nvcf
Length of output: 4411
🏁 Script executed:
#!/bin/bash
set -eu
fixture='deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler/kai-scheduler'
printf '%s\n' '--- complete rendered kai-config component keys ---'
sed -n '1,240p' "$fixture/templates/kai-config.yaml"
printf '%s\n' '--- complete rendered scheduler and default queue resources ---'
cat "$fixture/templates/default-shard.yaml"
cat "$fixture/templates/default-queue.yaml"
printf '%s\n' '--- chart-key references in repository documentation and fixtures ---'
rg -n -C 4 \
'podGrouper|podGroupController|queueController|nodeScaleAdjuster|numaPlacementExporter|numaPlacementExporter|defaultQueue|placementStrategy' \
deploy docs ai-tooling \
-g '*.yaml' -g '*.yaml.gotmpl' -g '*.md' -g 'values.yaml' \
| head -n 500
printf '%s\n' '--- OCI chart metadata ---'
curl -LfsS -H 'Accept: application/vnd.oci.image.index.v1+json' \
'https://ghcr.io/v2/kai-scheduler/kai-scheduler/tags/list' |
jq -r '.tags[]' | rg '(^|-)v?0\.14\.0$|0\.14' || trueRepository: NVIDIA/nvcf
Length of output: 29086
🏁 Script executed:
#!/bin/bash
set -eu
for ref in f909aece v0.14.0 main; do
url="https://raw.githubusercontent.com/NVIDIA/KAI-Scheduler/$ref/deployments/kai-scheduler/values.yaml"
echo "### $url"
body="$(curl -LfsS "$url" 2>/dev/null || true)"
if [ -n "$body" ]; then
printf '%s\n' "$body" |
rg -n -C 5 \
'^(operator|podgrouper|podGrouper|podgroupcontroller|podGroupController|binder|scheduler|queuecontroller|queueController|admission|nodescaleadjuster|nodeScaleAdjuster|numaPlacementExporter|numaPlacementExporter|defaultQueue):|plugins:|actions:' \
|| true
else
echo 'not available'
fi
done
printf '%s\n' '--- all fixture references to the suspected component ---'
rg -n 'operator|podgrouper|podGrouper|podgroupcontroller|podGroupController|queuecontroller|queueController|nodescaleadjuster|nodeScaleAdjuster|numaPlacementExporter|numaPlacementExporter' \
deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-schedulerRepository: NVIDIA/nvcf
Length of output: 21693
Remove the unsupported numaPlacementExporter block. KAI-Scheduler v0.14.0 supports the other listed keys, including scheduler.plugins, scheduler.actions, and defaultQueue, but it does not define or render numaPlacementExporter. Its resource settings are ignored.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl`
around lines 106 - 196, Remove the entire numaPlacementExporter configuration
block, including its resources requests and limits, from the dependency values
while leaving the supported operator components and scheduler configuration
unchanged.
Comment on lines
+248
to
+252
| condition: addons.kaiScheduler.clusterTopologies.enabled | ||
| chart: ../charts/nvcf-kai-topology | ||
| namespace: nvca-operator | ||
| needs: | ||
| - kai-scheduler/kai-scheduler |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
helmfile needs release excluded by condition warning or error behavior v1.1
💡 Result:
In Helmfile v1.1, specific improvements were made to warning behavior regarding release exclusions, particularly concerning the needs parameter. A notable change included in v1.1.2 ensures that Helmfile no longer issues a warning if a release is excluded due to its condition or selector, provided that both the release itself and its required dependencies (configured via needs) are set to installed: false [1]. Regarding the condition field generally: - The condition field is used to filter releases by a boolean value (e.g., vault.enabled) [2][3]. - If a release is excluded via condition or selectors, it is typically ignored unless explicitly included via flags like --include-needs or --include-transitive-needs [4][5]. - When using needs, these dependencies are ignored by default when using selectors/labels, unless overruled by specific CLI flags [4][5]. If you are encountering specific error or warning behavior, it is recommended to ensure your Helmfile configuration follows the expected syntax, as the condition field expects a path (e.g., foo.enabled) rather than arbitrary expressions, unless using conditionTemplate [2][3][6]. Additionally, ensure you are not relying on deprecated behavior, as Helmfile v1.x introduced breaking changes, including the move to gopkg.in/yaml.v3 by default [7][8].
Citations:
- 1: helmfile/helmfile@v0.167.0...v1.1.2
- 2: https://github.com/helmfile/helmfile/blob/main/docs/configuration.md
- 3: https://helmfile.readthedocs.io/en/latest/configuration/
- 4: https://helmfile.readthedocs.io/en/stable/releases/
- 5: https://helmfile.readthedocs.io/en/latest/releases/
- 6: Make the `condition` field on a release templatable helmfile/helmfile#2311
- 7: helmfile/helmfile@v1.0.0...v1.1.0
- 8: https://helmfile.readthedocs.io/en/latest/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files \
'deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl' \
'deploy/stacks/nvcf-compute-plane/environments/base.yaml' \
'*helmfile*' \
'.github/workflows/*' | sed -n '1,120p'
printf '%s\n' '--- relevant release definitions and conditions ---'
rg -n -C 8 \
'clusterTopologies|kaiScheduler|kai-scheduler|needs:|installed:|helmfile' \
deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl \
deploy/stacks/nvcf-compute-plane/environments/base.yaml \
deploy/stacks/nvcf-compute-plane 2>/dev/null | sed -n '1,280p'
printf '%s\n' '--- helmfile version/config references ---'
rg -n -i -C 3 'helmfile.*(version|image)|HELMFILE|v0\.[0-9]|v1\.[0-9]' \
.github deploy Makefile README.md 2>/dev/null | sed -n '1,240p'Repository: NVIDIA/nvcf
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- focused compute-plane configuration ---'
sed -n '38,55p;228,260p;258,274p;400,430p' \
deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl
sed -n '70,100p' deploy/stacks/nvcf-compute-plane/environments/base.yaml
printf '%s\n' '--- environment overrides ---'
find deploy/stacks/nvcf-compute-plane/environments -maxdepth 1 -type f -print0 |
sort -z | xargs -0 rg -n -C 3 \
'kaiScheduler|clusterTopologies|kai-scheduler' 2>/dev/null || true
printf '%s\n' '--- external KAI installation references ---'
rg -n -i -C 4 \
'equivalent in.cluster|external.*KAI|KAI.*external|Topology CRD|clusterTopolog|kai-scheduler' \
deploy/stacks/nvcf-compute-plane \
docs ai-tooling tests 2>/dev/null | sed -n '1,220p'Repository: NVIDIA/nvcf
Length of output: 30522
🌐 Web query:
Helmfile v1.1.9 conditional release needs dependency condition false active dependent behavior
💡 Result:
In Helmfile, the needs directive defines an ordering dependency (DAG-based) rather than a strict logical requirement for installation [1]. By default, Helmfile does not automatically prevent a release from being installed even if its dependency (listed in needs) is disabled via installed: false [2][3][4]. If a release A has needs: [B] and B is marked installed: false, Helmfile may proceed to install A while omitting B, often resulting in a warning rather than an error [2][3][4]. To address this behavior and enforce that all dependencies must be installed: 1. Use the --enforce-needs-are-installed flag: This flag (available in recent versions) validates that all releases specified in needs are indeed marked for installation. If a dependency is disabled (installed: false), the command will fail and report a dependency error [5]. 2. Use Go templating for conditional inclusion: Instead of relying on installed: false to skip a release that others depend on, you can use Helmfile's native Go templating to conditionally include or exclude the release entirely from the configuration based on your values [6]. If a dependency is missing from the configuration entirely, Helmfile will throw an error when a release lists it in needs [4]. For "conditions" specifically within Helm Charts (as opposed to Helmfile-level releases), ensure your Chart's apiVersion is set to v2, as older v1 charts may exhibit inconsistent behavior regarding the condition flag in Chart.yaml dependencies [7]. Helmfile-level release conditions are controlled by the condition field (or conditionTemplate), which supports boolean values or values lookup paths ending in .enabled [8].
Citations:
- 1: https://helmfile.readthedocs.io/en/latest/releases/
- 2: needs evaluated even if 'installed: false' helmfile/helmfile#1464
- 3: 'needs:' directive in the dependent release does not respect the 'installed: false' of the other release roboll/helmfile#1018
- 4: helmfile sync ignores needs when needed chart is not installed helmfile/helmfile#987
- 5: Issue-1883 fix helmfile/helmfile#2058
- 6: Documentation Discrepancy: Inconsistency in Support for 'condition' Flag in Helmfile Dependencies helmfile/helmfile#1258
- 7: Helmfile does not respect subchart/dependency conditions helmfile/helmfile#2120
- 8: https://github.com/helmfile/helmfile/blob/main/docs/configuration.md
🌐 Web query:
site:github.com/helmfile/helmfile "needs" "condition" "not found"
💡 Result:
The error message "needs condition not found" is not a standard error message produced by common CLI tools or programming languages. Based on technical documentation and common configuration patterns, it is highly likely that you are encountering an error related to Helmfile or a similar YAML-based configuration tool [1][2]. There are two common scenarios that often cause confusion around this terminology: 1. Configuration Schema Errors: If you are working with Helmfile, you might be attempting to use a 'condition' field in a location where it is not supported by the schema, such as within a 'dependencies' block [1]. In such cases, you may see an error like "field condition not found in type state.Dependency" [1]. Helmfile's 'condition' flag is typically used at the release level to determine if a release should be installed, not within nested dependency objects [1][3]. 2. Confusing 'needs' with 'condition': It is possible that you are conflating the 'needs' field (used for defining installation order or DAG dependencies) [2] with the 'condition' field (used for boolean toggles to enable or disable resources) [3]. If you are seeing a custom-generated error or a parse error, it may be because a tool is expecting one specific configuration key and finds an unexpected or misspelled one in your YAML file. Recommendations: - Check your configuration file syntax against the official schema documentation for the specific tool you are using [2]. - Ensure that the field name is spelled correctly and placed at the appropriate level of the YAML hierarchy (e.g., ensure 'condition' is at the 'release' level, not nested incorrectly) [1][3]. - If you are attempting to use logic (like "needs this condition to be true"), verify if the tool supports templating or complex conditional logic at that specific configuration point [1][3]. If this error is appearing in a different context, please provide the specific CLI tool, software, or programming language you are using, along with the full error message and the relevant code snippet.
Citations:
- 1: Documentation Discrepancy: Inconsistency in Support for 'condition' Flag in Helmfile Dependencies helmfile/helmfile#1258
- 2: https://github.com/helmfile/helmfile/blob/ff60d0b5/docs/index.md
- 3: feat: accept strings that evaluates to bool in release condition helmfile/helmfile#840
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- focused compute-plane configuration ---'
sed -n '38,55p;228,260p;258,274p;400,430p' \
deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl
sed -n '70,100p' deploy/stacks/nvcf-compute-plane/environments/base.yaml
printf '%s\n' '--- environment overrides ---'
find deploy/stacks/nvcf-compute-plane/environments -maxdepth 1 -type f -print0 |
sort -z | xargs -0 rg -n -C 3 \
'kaiScheduler|clusterTopologies|kai-scheduler' 2>/dev/null || true
printf '%s\n' '--- external KAI installation references ---'
rg -n -i -C 4 \
'equivalent in.cluster|external.*KAI|KAI.*external|Topology CRD|clusterTopolog|kai-scheduler' \
deploy/stacks/nvcf-compute-plane \
docs ai-tooling tests 2>/dev/null | sed -n '1,220p'Repository: NVIDIA/nvcf
Length of output: 28678
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Helmfile v1.1.9 source paths related to needs ---'
curl -fsSL \
'https://api.github.com/repos/helmfile/helmfile/git/trees/v1.1.9?recursive=1' |
jq -r '.tree[].path' |
rg '(^|/)(state|pkg|internal)/.*(state|release|needs|dag)|needs' |
sed -n '1,160p'
printf '%s\n' '--- related upstream issue and pull request summaries ---'
for n in 987 1018 1464 1258; do
curl -fsSL "https://api.github.com/repos/helmfile/helmfile/issues/$n" |
jq -r '"issue \(.number): \(.title)\nstate: \(.state)\nbody:\n\(.body // "")\n---"'
done
curl -fsSL 'https://api.github.com/repos/helmfile/helmfile/pulls/2058' |
jq -r '"pull \(.number): \(.title)\nstate: \(.state)\nbody:\n\(.body // "")"'Repository: NVIDIA/nvcf
Length of output: 555
🌐 Web query:
Helmfile v1.1.9 "needs" "excluded" release condition warning
💡 Result:
In Helmfile, there is no specific "excluded release condition warning" related to the condition field itself in version 1.1.9 [1][2][3]. The condition field in a release specification is a standard mechanism to enable or disable a release based on boolean values or template evaluations [3]. If you are encountering warnings regarding "excluded" releases, it is likely related to the behavior of the needs dependency graph when using selectors [1][4]. By default, helmfile ignores needs dependencies when selectors are used, which can result in releases being excluded from the operation [1][4]. When helmfile determines that a required dependency is missing or excluded from the current selection, it may issue a notice or require specific flags to resolve the graph correctly [5][4]. To manage these dependencies, use the following flags [1][4]: --skip-needs (default): Ignores dependencies, which may result in exclusions [1][4]. --include-needs: Includes the direct needs of the selected release(s) [1][4]. --include-transitive-needs: Includes both direct and transitive needs, overriding potential exclusions from selectors or conditions [1][6][4]. If you are seeing a "missing file" warning rather than a release condition warning, check the missingFileHandler setting in your helmfile.yaml, which can be set to "Warn" or "Error" [3][7]. If you have a specific error message you are investigating, it is likely a standard dependency resolution notice from the needs logic rather than a specific version-bound warning for v1.1.9 [5].
Citations:
- 1: https://helmfile.readthedocs.io/en/stable/releases/
- 2: https://helmfile.readthedocs.io/en/stable/
- 3: https://helmfile.readthedocs.io/en/latest/configuration/
- 4: https://helmfile.readthedocs.io/en/latest/releases/
- 5: Version: 0.139.0 Please add a selector like "--selector name=ingress-nginx", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies roboll/helmfile#1831
- 6: Add support for transitive dependencies (needs). roboll/helmfile#1983
- 7: https://github.com/helmfile/helmfile/blob/main/docs/writing-helmfile.md
Make the KAI dependency conditional on the installation mode. needs controls ordering and does not block nvcf-kai-topology when the Helmfile-managed KAI release is disabled, although Helmfile can report an excluded dependency. Include needs only when addons.kaiScheduler.enabled is true, or document that standalone KAI must provide the Topology CRD.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl`
around lines 248 - 252, Make the needs dependency for the nvcf-kai-topology
release conditional on addons.kaiScheduler.enabled, so it is emitted only when
Helmfile manages the KAI installation; preserve the existing
kai-scheduler/kai-scheduler ordering when enabled, and omit it for standalone or
disabled KAI deployments.
|
|
||
| # Namespaces owned by the compute-plane stack (used by destroy target) | ||
| NAMESPACES := nvca-operator grove-system dynamo-system | ||
| NAMESPACES := nvca-operator grove-system dynamo-system kai-scheduler |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find KAI namespace and CRD references across the compute-plane stack to size the cleanup gap.
set -euo pipefail
fd -t f . deploy/stacks/nvcf-compute-plane -x rg -n -C3 'kai-scheduler|reservation|kai\.scheduler|scheduling\.run\.ai' {}Repository: NVIDIA/nvcf
Length of output: 32970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Makefile destroy logic ---'
cat -n deploy/stacks/nvcf-compute-plane/Makefile.dist | sed -n '30,75p'
printf '%s\n' '--- KAI release and cleanup-related templates ---'
rg -n -C4 'post-delete|helm.sh/resource-policy|kind: CustomResourceDefinition|resourceNames:|kai-resource-reservation|kind: (Topology|Queue|PodGroup)' \
deploy/stacks/nvcf-compute-plane/charts \
deploy/stacks/nvcf-compute-plane/releases \
deploy/stacks/nvcf-compute-plane 2>/dev/null | sed -n '1,260p'
printf '%s\n' '--- destroy target references ---'
rg -n -C6 'destroy|NAMESPACES|helmfile|kubectl.*delete' deploy/stacks/nvcf-compute-plane/Makefile.distRepository: NVIDIA/nvcf
Length of output: 44760
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Makefile destroy target ---'
rg -n -C12 '^destroy:|NAMESPACES|kubectl.*delete|helmfile.*destroy|helmfile.*delete' \
deploy/stacks/nvcf-compute-plane/Makefile.dist
printf '%s\n' '--- KAI chart files ---'
fd -t f . deploy/stacks/nvcf-compute-plane | rg '/kai-scheduler/' | rg -v '/testdata/'
printf '%s\n' '--- KAI chart cleanup and CRD definitions ---'
fd -t f . deploy/stacks/nvcf-compute-plane | rg '/kai-scheduler/' | rg -v '/testdata/' | \
xargs -r rg -n -C5 'post-delete|helm.sh/resource-policy|kind: CustomResourceDefinition|resourceNames:|kind: (Topology|Queue|PodGroup)|kai-resource-reservation'
printf '%s\n' '--- Direct KAI chart source locations ---'
fd -t d -i 'kai-scheduler' . deploy/stacks/nvcf-compute-plane | rg -v '/testdata/'Repository: NVIDIA/nvcf
Length of output: 3058
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- KAI post-delete hook ---'
fd -t f . deploy/stacks/nvcf-compute-plane/testdata/golden | \
rg 'kai-scheduler/.*/post-delete|kai-scheduler/.*/crd|kai-scheduler/.*/default-queue|kai-scheduler/.*/default-shard' | \
while IFS= read -r f; do
printf '\n### %s\n' "$f"
cat -n "$f" | sed -n '1,180p'
done
printf '%s\n' '--- KAI CRD-related manifests in golden output ---'
rg -n -C8 'kind: CustomResourceDefinition|name: (configs|schedulingshards|bindrequests|podgroups|queues|topologies)\.' \
deploy/stacks/nvcf-compute-plane/testdata/golden/local/01-dependencies.yaml-a814432c-kai-scheduler \
| sed -n '1,260p'
printf '%s\n' '--- Stack documentation for destroy and KAI lifecycle ---'
rg -n -C5 'destroy|CRD|manual|resource reservation|kai-resource-reservation|KAI Scheduler' \
deploy/stacks/nvcf-compute-plane --glob '!testdata/**' --glob '*.md' --glob '*.yaml' --glob '*.gotmpl' --glob 'Makefile.dist' \
| sed -n '1,260p'Repository: NVIDIA/nvcf
Length of output: 9976
🌐 Web query:
KAI Scheduler Helm chart v0.14.0 CRD deletion post-delete resource-policy keep kai-resource-reservation
💡 Result:
In the KAI Scheduler Helm chart, the kai-resource-reservation namespace is configured with the helm.sh/resource-policy: keep annotation [1]. This annotation prevents the namespace from being deleted when the Helm release is uninstalled [2][3]. This behavior is intentional and part of the chart's design to preserve critical scheduler infrastructure—such as SchedulingShards and the resource reservation namespace—even if the main Helm release is removed [2][3]. Because these resources are marked with helm.sh/resource-policy: keep, they are excluded from the default Helm uninstall cleanup [2][3]. Key details regarding this configuration include: 1. Persistence of Resources: The kai-resource-reservation namespace and SchedulingShard custom resources are not managed by standard Helm deletion because they are explicitly protected by the keep policy [2][3]. 2. Uninstallation Impact: Users may encounter leftover pods or resources after running helm uninstall because these components remain active in the cluster [2]. If full cleanup is required, these resources must be removed manually [4]. 3. Helm Chart v0.14.0 Updates: Recent improvements to the chart, such as those related to post-delete cleanup hooks and resource policies, have focused on stabilizing operator-managed deployments and preventing unnecessary recreations of resources like kai-config [5][6], but they do not alter the fundamental keep policy for the core infrastructure resources identified in your query [1]. If you are performing a cleanup and these resources are persisting, you must remove them manually using kubectl delete namespace kai-resource-reservation (or the specifically configured namespace) and the associated custom resources [4].
Citations:
- 1: kai-scheduler/KAI-Scheduler@0fe3c86
- 2: bug: Helm uninstall leaves behind scheduler pods due to "kept" SchedulingShard kai-scheduler/KAI-Scheduler#661
- 3: https://github.com/kai-scheduler/KAI-Scheduler/blob/main/docs/gitops/README.md
- 4: https://deepwiki.com/NVIDIA/KAI-Scheduler/2.2-helm-chart-installation
- 5: https://github.com/kai-scheduler/KAI-Scheduler/blob/main/CHANGELOG.md
- 6: fix(chart): stop recreating kai-config CR on helm upgrade kai-scheduler/KAI-Scheduler#1550
Clean up KAI resources during destroy.
Add kai-resource-reservation to NAMESPACES. Helm keeps this namespace, but the destroy target can remove it explicitly. Document that KAI CRDs and kept cluster-scoped resources require manual cleanup, or add a guarded cleanup step. The post-delete hook does not remove the CRDs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/stacks/nvcf-compute-plane/Makefile.dist` at line 38, Update the
NAMESPACES definition to include kai-resource-reservation so the destroy target
removes it explicitly. Also document the required manual cleanup of KAI CRDs and
retained cluster-scoped resources, or add a guarded cleanup step for those
resources without affecting existing destroy behavior.
Comment on lines
+109
to
+116
| ```yaml | ||
| apiVersion: apps/v1 | ||
| kind: StatefulSet | ||
| metadata: | ||
| annotations: | ||
| kai.scheduler/topology: "nvcf-mnnvl-topology" | ||
| kai.scheduler/topology-required-placement: "nvidia.com/gpu.clique" | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- docs/user/helm-functions.md ---'
sed -n '80,135p' docs/user/helm-functions.md
printf '%s\n' '--- KAI scheduler references in repository ---'
rg -n -i -C 3 'schedulerName|kai-scheduler|topology-required-placement|nvcf-mnnvl-topology' docs .github charts 2>/dev/null || true
printf '%s\n' '--- KAI v0.14.0 quickstart ---'
curl -L --fail --silent --show-error \
https://raw.githubusercontent.com/kai-scheduler/KAI-Scheduler/v0.14.0/docs/quickstart/README.md \
| rg -n -i -C 4 'schedulerName|kai-scheduler|StatefulSet|topology-required-placement|gang' || trueRepository: NVIDIA/nvcf
Length of output: 36393
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
import urllib.request
doc = Path("docs/user/helm-functions.md").read_text()
match = re.search(
r"##### With KAI Scheduler.*?```yaml\n(.*?)\n```",
doc,
re.S,
)
if not match:
raise SystemExit("KAI Scheduler YAML example not found")
manifest = match.group(1)
print("--- local YAML example checks ---")
print("StatefulSet:", "kind: StatefulSet" in manifest)
print("StatefulSet metadata annotations:",
"kai.scheduler/topology:" in manifest and
"kai.scheduler/topology-required-placement:" in manifest)
print("Pod template schedulerName:",
bool(re.search(r"^\s+ schedulerName:\s+kai-scheduler\s*$", manifest, re.M)) or
"schedulerName: kai-scheduler" in manifest)
url = "https://raw.githubusercontent.com/kai-scheduler/KAI-Scheduler/v0.14.0/docs/quickstart/README.md"
upstream = urllib.request.urlopen(url, timeout=10).read().decode()
print("--- upstream v0.14.0 requirement ---")
for line in upstream.splitlines():
if "scheduler name" in line.lower() or "kai-scheduler" in line:
print(line)
PYRepository: NVIDIA/nvcf
Length of output: 2561
Set the Pod scheduler to kai-scheduler.
Add spec.template.spec.schedulerName: kai-scheduler to the StatefulSet example, or state this requirement explicitly. Without it, the default scheduler handles the Pods, so the documented KAI topology and gang-scheduling behavior does not apply.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/user/helm-functions.md` around lines 109 - 116, Update the StatefulSet
example by adding spec.template.spec.schedulerName with the value kai-scheduler,
ensuring Pods use KAI for the documented topology and gang-scheduling behavior.
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
1 participant
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.
TL;DR
This MR implements the suggested fix from #598 with documentation on how to use MNNVL features
Additional Details (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
For the Reviewer
For QA (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
Issues
Relates to #598
Checklist
Summary by CodeRabbit
New Features
Documentation
Configuration