feat(recipes): replace AKS ib-node-config DaemonSet with nodewright tuning - #1698
Conversation
|
🌿 Preview your docs: https://nvidia-preview-feat-aks-ib-rdma-via-nodewright.docs.buildwithfern.com/aicr |
Recipe evidence check
Protected recipesRecipes with committed evidence (
Other affected recipes without evidence yet: 42These recipes are affected by this PR but carry no committed evidence pointer, so there is
How to refresh evidenceRun on a cluster matching the recipe's aicr snapshot -o snapshot.yaml
aicr validate \
-r recipes/overlays/<slug>.yaml \
-s snapshot.yaml \
--emit-attestation ./out \
--push ghcr.io/<your-fork>/aicr-evidence
# Copy to the per-source path printed in the emit 'copyTo' hint:
# recipes/evidence/<slug>/<source>/<bundle-digest>.yamlThis gate is warning-only and never blocks merge. See ADR-007 for the trust model. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAKS recipes now use Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Coverage Report ✅
Coverage BadgeMerging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
7186f16 to
5705691
Compare
ayuskauskas
changed the title
|
Reworked per review: no separate AKS manifest. The AKS accelerator overlays now reuse the shared |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/tuning/compute_test.go`:
- Around line 40-41: Update the AKS test cases in the compute test table to
include expected package versions, specifically asserting the nvidia-setup 0.5.0
pin for the changed rows. Extend the relevant test comparison logic to validate
both package names and versions, including the nvidia-tuned version where
applicable.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 34ae8668-0699-4c30-acb5-dcca7ad584b0
📒 Files selected for processing (9)
docs/integrator/components/nodewright.mddocs/user/container-images.mdpkg/tuning/compute_test.gorecipes/components/network-operator/manifests/ib-node-config-aks.yamlrecipes/components/nodewright-customizations/manifests/tuning.yamlrecipes/overlays/a100-aks-training.yamlrecipes/overlays/aks.yamlrecipes/overlays/h100-aks-inference.yamlrecipes/overlays/h100-aks-training.yaml
💤 Files with no reviewable changes (1)
- recipes/components/network-operator/manifests/ib-node-config-aks.yaml
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Solid replacement of the privileged ib-node-config DaemonSet — the AKS wiring exactly mirrors the EKS pattern, the live-cluster verification is thorough, and docs/BOM/tuning-docs are all regenerated. One high-severity concern blocks: the default aicr bundle path injects the wildcard operator: Exists toleration into the Skyhook CR, which is exactly the nodewright#296 deadlock the manifest comment warns about — and since nvidia-setup-full carries a reboot interrupt, a default AKS bundle deadlocks on first install where the old DaemonSet worked out of the box. A bundle-time validation warning (the framework already supports service conditions) and/or a user-docs note would resolve it. Two minor items: a stale comment in nfd-network-rule.yaml and a double blank line. CI green so far (Tier 1 matrix still in progress).
|
@ayuskauskas this PR now has merge conflicts with |
5705691 to
73a37a5
Compare
ayuskauskas
added
theme/recipes
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
recipes/overlays/aks.yaml (1)
56-76: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd the Nodewright tuning to the direct AKS overlays
recipes/overlays/aks-training.yamlandrecipes/overlays/aks-inference.yamlstill inherit fromakswithoutnodewright-customizations, so those AKS variants lose the RDMA/memlock setup now thatib-node-config-aksis gone. Add the component there, or document that these two recipes intentionally opt out.🤖 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 `@recipes/overlays/aks.yaml` around lines 56 - 76, Add nodewright-customizations to both aks-training.yaml and aks-inference.yaml so they retain the RDMA and MEMLOCK tuning inherited from the base AKS setup. Match the component configuration and dependency references used by the direct AKS overlay, or explicitly document the intentional opt-out in both recipes if that is the desired behavior.
🤖 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 `@recipes/registry.yaml`:
- Around line 266-274: The CheckWildcardAcceleratedToleration validation is too
permissive for a configuration that can cause an unrecoverable AKS installation
deadlock. Change its severity from warning to error so bundle generation is
blocked until users provide a keyed --accelerated-node-toleration value.
---
Outside diff comments:
In `@recipes/overlays/aks.yaml`:
- Around line 56-76: Add nodewright-customizations to both aks-training.yaml and
aks-inference.yaml so they retain the RDMA and MEMLOCK tuning inherited from the
base AKS setup. Match the component configuration and dependency references used
by the direct AKS overlay, or explicitly document the intentional opt-out in
both recipes if that is the desired behavior.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 2e9b26d9-84ce-42b2-903f-18bcbdf60644
📒 Files selected for processing (15)
docs/contributor/validator.mddocs/integrator/aks-gpu-setup.mddocs/integrator/components/nodewright.mddocs/user/container-images.mdpkg/bundler/validations/checks.gopkg/bundler/validations/checks_test.gopkg/tuning/compute_test.gorecipes/components/network-operator/manifests/ib-node-config-aks.yamlrecipes/components/network-operator/manifests/nfd-network-rule.yamlrecipes/components/nodewright-customizations/manifests/tuning.yamlrecipes/overlays/a100-aks-training.yamlrecipes/overlays/aks.yamlrecipes/overlays/h100-aks-inference.yamlrecipes/overlays/h100-aks-training.yamlrecipes/registry.yaml
💤 Files with no reviewable changes (1)
- recipes/components/network-operator/manifests/ib-node-config-aks.yaml
…uning Removes the privileged ib-node-config-aks DaemonSet from the AKS network-operator component and wires the AKS accelerator overlays (h100-aks-inference, h100-aks-training, a100-aks-training) to the same nodewright-customizations tuning manifest EKS uses, with service: aks. nvidia-setup is bumped to 0.5.0 in tuning.yaml, which adds the aks-h100 combination (NVIDIA/nodewright-packages#42): host IB RDMA setup (ib_umad/rdma_ucm module loading persisted via modules-load.d, memlock limits, LimitMEMLOCK=infinity drop-ins for containerd/kubelet), replacing what the DaemonSet did. 0.5.0 is otherwise identical to 0.4.0 for EKS, so the shared manifest keeps working for the existing services. On AKS the nvidia-setup-kernel package is a designed no-op (aks defaults omit KERNEL) and nvidia-tuned applies the aks-h100-{intent} profiles. Guards the AKS wildcard-toleration deadlock at bundle time: a new CheckWildcardAcceleratedToleration validation (registry-scoped to service: aks on nodewright-customizations) warns when the effective accelerated-node tolerations include a keyless wildcard — the default aicr bundle path injects one via DefaultTolerations(), and AKS admission collapses toleration lists to the wildcard, which breaks the nodewright drain exemption for its own package pods and deadlocks interrupt-bearing packages on first install (NVIDIA/nodewright#296). The caveat is documented in the shared manifest and in docs/integrator/aks-gpu-setup.md alongside the RDMA opt-out instructions. Verified end to end on AKS v1.35.5 with 2x Standard_ND96isr_H100_v5 running this package chain (kernel no-op, nvidia-tuned aks-h100-inference profile, IB modules + effective unlimited memlock, reboot persistence). Regenerates the tuning-status table (aks rows) and the container-images BOM, and updates the tuning structure test rows. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
73a37a5 to
b4850ef
Compare
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
LGTM. All review threads resolved and verified against the head commit:
CheckWildcardAcceleratedTolerationwired onnodewright-customizationsatseverity: error, with the disabled-component guard so thenodewrightcustomizations:enabled=falseopt-out isn't false-flagged — correct, not just strict.- Stale
nfd-network-rule.yamlDaemonSet comment rewritten to the NicClusterPolicy nodeAffinity intent. wantRowkept version-agnostic; pins stay CI-gated bymake tuning-check+TestCompute_VersionsMatchBOM.
The default-bundle deadlock concern is now fenced off by a blocking check with two documented escape hatches (keyed toleration / component disable), both live-verified. Coverage up (+2.31% on pkg/bundler/validations). Approving once the in-flight CI run is green.
2 participants
Summary
Replaces the privileged
ib-node-config-aksDaemonSet with nodewright: the AKS accelerator overlays now wire the samenodewright-customizationstuning manifest EKS uses (setup and tuning packages), withservice: aks, andtuning.yamlbumpsnvidia-setupto 0.5.0 which adds theaks-h100combination.Motivation / Context
The DaemonSet carried a TODO to remove it once nodewright gained AKS RDMA/MEMLOCK support. That support shipped in NVIDIA/nodewright-packages#42 (released as
nvidia-setup/0.5.0): the package performs the same host setup (persistentib_umad/rdma_ucmmodule loading, memlock limits,LimitMEMLOCK=infinitydrop-ins for containerd and kubelet), with nodewright handling service restarts and post-interrupt verification. This removes a privileged always-running DaemonSet in favor of lifecycle-managed, checked host configuration, and brings AKS onto the same tuning manifest as EKS (nvidia-tuned applies theaks-h100-{intent}profiles).Fixes: N/A (DaemonSet TODO)
Related: NVIDIA/nodewright-packages#42, NVIDIA/nodewright#296, #1697 (host GPU driver auto-detect; orthogonal, no file overlap)
Type of Change
Component(s) Affected
pkg/recipe)docs/,examples/)pkg/bundler/validations,pkg/tuningtestsImplementation Notes
tuning.yamlmoves tonvidia-setup 0.5.0for all consumers. 0.5.0 is identical to 0.4.0 for EKS (its only delta is the aks-h100 combination and anensure_kernelno-op whenKERNELis unset), so EKS overlays are unaffected beyond the pin.h100-aks-inference(intentinference),h100-aks-training(intentmultiNodeTraining), anda100-aks-training(h100 profile reuse with the same rationale and comment asa100-eks-training). Ubuntu variants inherit. The stale "Nodewright packages do not support service: aks" comments are removed.nvidia-setup-kernelis a designed no-op (aks defaults intentionally omitKERNEL; AKS manages its own Ubuntu kernel); this is documented in the manifest.acceleratedTolerations(for examplenvidia.com/gpu) must be injected on AKS.CheckWildcardAcceleratedTolerationvalidation inpkg/bundler/validations, registered onnodewright-customizationswithseverity: warningandconditions: service: [aks]. The defaultaicr bundlepath injects theDefaultTolerations()wildcard intoacceleratedTolerations, so a plain AKS bundle would otherwise hit the nodewright#296 deadlock silently; the warning surfaces in deployment notes with the--accelerated-node-tolerationremedy. The keyed-toleration requirement and RDMA opt-out flags are also documented indocs/integrator/aks-gpu-setup.md("InfiniBand RDMA Host Setup (nodewright)"), and the function is listed indocs/contributor/validator.md.aks.yamlnow includes--set nodewrightcustomizations:enabled=false.make tuning-docs(aks rows:nvidia-setup 0.5.0+nvidia-tuned 0.3.1, matching eks) andmake bom-docs.Testing
aks-h100-inferenceprofile, nvidia-setup-full IB RDMA + memlock, reboot interrupts) was verified end to end on a live AKS v1.35.5 cluster with 2xStandard_ND96isr_H100_v5: all four config files written, modules loaded and reloaded frommodules-load.dafter reboot,LimitMEMLOCK=infinityconfigured and effective on containerd/kubelet, fresh pods seeulimit -lunlimited,umad*/rdma_cmdevices present. Full evidence: nodewright-packages#42 verification comment.go test ./pkg/tuning/... ./tools/tuning/... ./pkg/bom/...pass (structure test gains the two aks rows).aicr bundleof an AKS H100 inference recipe emits the wildcard-toleration warning in deployment notes; with--accelerated-node-toleration nvidia.com/gpu:NoSchedulethe warning is gone and the rendered Skyhook CR carries the keyed toleration.pkg/bundler/validations: 84.6% → 86.9% (+2.3%);CheckWildcardAcceleratedTolerationat 100%.make qualifypasses on the rebased branch (test-coverage, lint, tuning-check, e2e — 23 chainsaw tests, scan, license-check).yamllintclean on the changed recipe files.