feat(helm-prereqs): automate DPF (DOCA Platform Framework) install in setup.sh#3578
Open
shayan1995 wants to merge 1 commit into
Open
feat(helm-prereqs): automate DPF (DOCA Platform Framework) install in setup.sh#3578shayan1995 wants to merge 1 commit into
shayan1995 wants to merge 1 commit into
Conversation
Contributor
Summary by CodeRabbit
WalkthroughAdds DPF as the default DPU provisioning stack, including prerequisite Helm releases, operator resources, setup orchestration, two-phase NICo Core enablement, RBAC, health checks, teardown handling, and configuration and operational documentation. ChangesDPF provisioning integration
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant setup.sh
participant preflight.sh
participant helmfile
participant dpf-operator
participant carbide-api
participant nico-api
setup.sh->>preflight.sh: Validate DPF flags, tools, variables, and Core values
setup.sh->>helmfile: Install DPF prerequisite releases
setup.sh->>dpf-operator: Install operator and apply DPF resources
setup.sh->>carbide-api: Set BMC credential and enable DPF
setup.sh->>nico-api: Restart service for DPF SDK initialization
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
shayan1995
force-pushed
the
feat/setup-dpf-install
branch
from
b4958b8 to
c31fb7a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
helm-prereqs/operators/dpf/dpu-cluster-vip-service.yaml.tmpl (1)
32-42: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer
EndpointSlicefor the selector-less Service.
v1 Endpointsis deprecated in Kubernetes 1.33+, and this relies on control-plane mirroring of a manually authored Endpoints object. Create adiscovery.k8s.io/v1EndpointSlice directly, labeled fordpu-cluster-vip-loadbalancer, and verify the repository’s minimum Kubernetes version. (kubernetes.io)🤖 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 `@helm-prereqs/operators/dpf/dpu-cluster-vip-service.yaml.tmpl` around lines 32 - 42, Replace the manually authored v1 Endpoints resource with a discovery.k8s.io/v1 EndpointSlice for dpu-cluster-vip-loadbalancer, preserving the dummy VIP address and port while labeling the slice for the selector-less Service. Verify and honor the repository’s minimum Kubernetes version before applying this API change.Sources: Path instructions, MCP tools
🤖 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 `@docs/manuals/dpf.md`:
- Around line 75-78: Update the explanatory text in the DPF SDK initialization
documentation to consistently hyphenate “site-wide,” including the phrase
“site-wide BMC root credentials.”
- Around line 1027-1040: Add an explicit DPU-service mapping table at both
docs/manuals/dpf.md lines 1027-1040 and helm-prereqs/README.md lines 354-364,
mapping each configured service name (dpu_agent, dhcp_server, fmds, and otel) to
its corresponding forge-* or carbide-* image/artifact. Keep the existing build
and publish instructions, and ensure both tables use consistent mappings.
In `@helm-prereqs/clean.sh`:
- Around line 97-128: Scope the teardown commands for tenantcontrolplane,
datastores.kamaji.clastix.io, applications.argoproj.io, and
appprojects.argoproj.io to resources owned by this installation using exact
names or installation labels instead of --all -A. Update the finalizer loop over
_kind and _res to discover only owned resources and patch each resource in its
actual namespace rather than hard-coding dpf-operator-system. Apply the same
ownership restriction to the broad RBAC cleanup.
In `@helm-prereqs/health-check.sh`:
- Around line 249-253: Update the DPF health-check logic around DPF_NS discovery
and the DPU query to distinguish an explicit --skip-dpf opt-out from an
unhealthy or partial installation. Detect any existing DPF namespace or release
independently, and treat failures from kubectl observation commands as explicit
errors rather than converting them to an empty namespace or zero DPUs; reserve
the skip message only for a confirmed opt-out.
In `@helm-prereqs/helmfile.yaml`:
- Around line 156-165: Update both preSync CRD-apply hooks for Kamaji to replace
the silent “|| true” fallback with a clear failure message written to stderr,
including the chart or CRD context. Preserve the existing command pipeline and
allow the hook to continue as currently intended after reporting the failure.
In `@helm-prereqs/operators/dpf/dpucluster.yaml.tmpl`:
- Around line 30-34: Update the DPUCluster node-selection configuration around
nodeSelector so control-plane nodes are matched regardless of whether
node-role.kubernetes.io/control-plane has an empty or "true" value. Prefer an
Exists-based node affinity rule if the CRD supports affinity; otherwise expose
the selector value as a configurable template setting instead of hardcoding an
empty string.
In `@helm-prereqs/README.md`:
- Around line 315-320: Update the DPF documentation to match the per-service
defaults defined by dpf_services.rs: dts uses public DOCA NGC, doca_hbn uses
public DOCA images, and dpu_agent, dhcp_server, fmds, and otelcol use the
private carbide-dev Helm/image registries with nico-pull-secret. Apply this
matrix at helm-prereqs/README.md lines 315-320 and 368-376, and
docs/manuals/dpf.md lines 1027-1031 and 1051-1055, replacing blanket public-NGC
wording without changing unrelated setup guidance.
In `@helm-prereqs/setup.sh`:
- Around line 548-568: Track whether _kamaji_patch_failurepolicy Ignore
successfully relaxed Kamaji’s webhook policy, and add EXIT cleanup that restores
Fail whenever that state remains active, including errexit paths before the
explicit restoration. Clear the tracked state after restoring Fail so cleanup is
idempotent, while preserving the existing successful setup flow.
- Around line 587-613: Update the secret-generation commands, including
_dpf_argo_repo_secret and the corresponding BMC-password handling around the
other affected block, so credentials never appear in kubectl process arguments.
Generate manifests using stdin or permission-restricted temporary files, then
apply them through kubectl while preserving the existing secret names,
namespaces, labels, and values.
- Around line 665-670: Update the NICO_DPF_SRC_DIR cleanup logic in the
surrounding setup flow to refuse deletion of any existing path that is not an
expected Git clone, including arbitrary system or repository directories.
Validate the configured path and Git-repository state before rm -rf, and exit
with the existing safety error for invalid or non-Git directories; preserve
cleanup only for the intended clone directory.
- Around line 817-883: Update the BMC-root Job manifest before its template to
set activeDeadlineSeconds to the polling timeout, and in the failure path after
polling explicitly delete the Job and wait until it is gone before deleting
dpf-bmc-root-pw and dpf-admincli-cert or returning failure. Preserve the
existing success path and cleanup behavior.
In `@helm/charts/nico-api/values.yaml`:
- Around line 224-226: Remove the configurable dpf.operatorNamespace override
from the values configuration and keep the DPF namespace fixed consistently with
the prerequisite manifests at dpf-operator-system, ensuring NICo API RBAC
remains aligned with the operator and CR namespace.
---
Nitpick comments:
In `@helm-prereqs/operators/dpf/dpu-cluster-vip-service.yaml.tmpl`:
- Around line 32-42: Replace the manually authored v1 Endpoints resource with a
discovery.k8s.io/v1 EndpointSlice for dpu-cluster-vip-loadbalancer, preserving
the dummy VIP address and port while labeling the slice for the selector-less
Service. Verify and honor the repository’s minimum Kubernetes version before
applying this API change.
🪄 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: cc57ca8a-7ba9-425c-bf15-52437b2649b0
📒 Files selected for processing (23)
.gitignorebook/src/configuration/configurability.mdcrates/dpf/README.mddocs/getting-started/quick-start.mddocs/manuals/dpf.mdhelm-prereqs/.helmignorehelm-prereqs/README.mdhelm-prereqs/clean.shhelm-prereqs/health-check.shhelm-prereqs/helmfile.yamlhelm-prereqs/operators/dpf/cert-manager-policy.yamlhelm-prereqs/operators/dpf/dpfoperatorconfig.yaml.tmplhelm-prereqs/operators/dpf/dpu-cluster-vip-service.yaml.tmplhelm-prereqs/operators/dpf/dpucluster.yaml.tmplhelm-prereqs/operators/values/argo-cd.yamlhelm-prereqs/operators/values/kamaji.yamlhelm-prereqs/operators/values/maintenance-operator.yamlhelm-prereqs/operators/values/node-feature-discovery.yamlhelm-prereqs/preflight.shhelm-prereqs/setup.shhelm-prereqs/values/nico-core.yamlhelm/charts/nico-api/templates/dpf-rbac.yamlhelm/charts/nico-api/values.yaml
Comment on lines
+75
to
+78
| carbide-api's DPF SDK init **hard-requires** the site-wide BMC root credential | ||
| (the shared BMC password DPF uses to reach the DPUs' host BMCs over Redfish). If | ||
| it is not set, carbide-api fails to start with | ||
| `Failed to initialize DPF SDK: ... Site wide BMC root credentials not set`. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate “site-wide”.
Use “site-wide BMC root credentials” in the explanatory text.
As per path instructions, documentation must be grammatically correct.
🧰 Tools
🪛 LanguageTool
[grammar] ~77-~77: Use a hyphen to join words.
Context: ...ot set, carbide-api fails to start with Failed to initialize DPF SDK: ... Site wide BMC root credentials not set. That...
(QB_NEW_EN_HYPHEN)
🤖 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/manuals/dpf.md` around lines 75 - 78, Update the explanatory text in the
DPF SDK initialization documentation to consistently hyphenate “site-wide,”
including the phrase “site-wide BMC root credentials.”
Sources: Path instructions, Linters/SAST tools
Comment on lines
+97
to
+128
| kubectl delete tenantcontrolplane --all -A --timeout=180s 2>/dev/null || true | ||
| # Kamaji `default` DataStore: carries helm.sh/resource-policy:keep (so | ||
| # helmfile destroy won't remove it) and a kamaji finalizer. Delete it here, | ||
| # while the kamaji controller is still alive to finalize it — otherwise its | ||
| # finalizer blocks the datastores CRD deletion and namespace termination. | ||
| kubectl delete datastores.kamaji.clastix.io --all -A \ | ||
| --timeout=120s 2>/dev/null || true | ||
| # Argo CD Applications carry resources-finalizer.argocd.argoproj.io, which | ||
| # cascades a delete onto the (now-gone) DPU cluster and cannot complete on a | ||
| # DPU-less / VIP-unroutable cluster. Delete best-effort; stragglers get their | ||
| # finalizers stripped below before argo-cd itself is destroyed. | ||
| kubectl delete applications.argoproj.io --all -A \ | ||
| --timeout=60s 2>/dev/null || true | ||
|
|
||
| # Best-effort finalizer strip for anything stuck after the bounded waits. | ||
| # Cover every DPF CR kind here — plus the kamaji DataStore and the Argo CD | ||
| # Applications/AppProjects — because `kubectl delete crd` (step 2) blocks | ||
| # until all instances finalize, so any kind left finalizer-stuck would hang | ||
| # the whole teardown. Applications must be stripped before argo-cd is | ||
| # destroyed (below), or nothing can ever clear their cascade finalizer. | ||
| for _kind in dpudeployments dpuservices dpuservicechains dpuserviceinterfaces \ | ||
| dpuservicetemplates dpuserviceconfigurations dpuservicenads \ | ||
| dpus dpudevices dpunodes dpunodemaintenances dpusets \ | ||
| bfbs bluefieldsoftwares dpuflavors \ | ||
| dpfoperatorconfigs dpuclusters tenantcontrolplanes \ | ||
| datastores.kamaji.clastix.io \ | ||
| applications.argoproj.io appprojects.argoproj.io; do | ||
| for _res in $(kubectl get "${_kind}" -A -o name 2>/dev/null); do | ||
| echo "WARNING: force-removing finalizers on stuck ${_res}" | ||
| kubectl patch "${_res}" -n dpf-operator-system --type merge \ | ||
| -p '{"metadata":{"finalizers":[]}}' 2>/dev/null || true | ||
| done |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Scope teardown to resources owned by this installation.
The --all -A operations can delete unrelated Kamaji tenants and Argo CD Applications, including cascading deletion of their workloads. The broad RBAC name match has the same cross-installation risk. Additionally, the finalizer loop discovers resources cluster-wide but patches all of them in dpf-operator-system, so out-of-namespace results are not actually repaired.
Use exact resource names, installation labels, or namespace-aware iteration rather than cluster-wide deletion.
Also applies to: 234-236
🤖 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 `@helm-prereqs/clean.sh` around lines 97 - 128, Scope the teardown commands for
tenantcontrolplane, datastores.kamaji.clastix.io, applications.argoproj.io, and
appprojects.argoproj.io to resources owned by this installation using exact
names or installation labels instead of --all -A. Update the finalizer loop over
_kind and _res to discover only owned resources and patch each resource in its
actual namespace rather than hard-coding dpf-operator-system. Apply the same
ownership restriction to the broad RBAC cleanup.
c31fb7a to
f364b80
Compare
Contributor
Author
|
Thanks @coderabbitai — triaged all 12 comments against the current code. Summary of what was addressed (commit Fixed (9):
Skipped as false positives (3):
Also not changed (optional polish): the |
shayan1995
force-pushed
the
feat/setup-dpf-install
branch
from
f364b80 to
301b826
Compare
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /tmp
gh pr view 3578 --repo NVIDIA/infra-controller --json commits --jq '.commits[-5:] | .[].oid' 2>&1 || true
git log --oneline -10 2>&1 | head -20
git show 301b8261c --stat 2>&1 | head -50Length of output: 523 🏁 Script executed: #!/bin/bash
set -e
gh repo clone NVIDIA/infra-controller /tmp/repo -- --quiet 2>&1 | tail -5
cd /tmp/repo
git fetch origin 301b8261c8e172fc3060c276362d438c47e0b1c3 --depth=2 2>&1 | tail -20
git show 301b8261c8e172fc3060c276362d438c47e0b1c3 --stat 2>&1 | head -60Length of output: 4083 🏁 Script executed: #!/bin/bash
cd /tmp/repo
git checkout 301b8261c8e172fc3060c276362d438c47e0b1c3 -- helm-prereqs/setup.sh helm-prereqs/clean.sh helm-prereqs/helmfile.yaml helm-prereqs/health-check.sh 2>&1
echo "=== secrets off-argv ==="
grep -n "docker-password\|from-literal\|dockerconfigjson\|process substitution\|printf.*|" helm-prereqs/setup.sh | head -40
echo "=== trap / failurePolicy ==="
grep -n "trap\|failurePolicy" helm-prereqs/setup.sh | head -40
echo "=== activeDeadlineSeconds / Job delete wait ==="
grep -n "activeDeadlineSeconds\|kubectl delete job\|wait.*job" helm-prereqs/setup.sh -i | head -40
echo "=== rm -rf NICO_DPF_SRC_DIR ==="
grep -n "NICO_DPF_SRC_DIR" helm-prereqs/setup.sh -i | head -40
echo "=== clean.sh namespace-aware patch ==="
grep -n "kubectl patch\|dpf-operator-system\| -A \|finalize" helm-prereqs/clean.sh | head -60
echo "=== helmfile preSync WARNING ==="
grep -n "preSync\|WARNING\|true$" helm-prereqs/helmfile.yaml | head -40
echo "=== health-check dpf-operator-system ==="
grep -n "dpf-operator-system\|skip-dpf\|fail" helm-prereqs/health-check.sh | head -40Length of output: 12198 🏁 Script executed: #!/bin/bash
cd /tmp/repo
sed -n '900,940p' helm-prereqs/setup.shLength of output: 2329
Everything checks out as described — nice work closing these out cleanly, especially the Kamaji trap and the finalizer-strip namespace bug, which were the two I'd have flagged as highest-risk if left unaddressed. On the three skipped items, your reasoning holds up:
This is a solid, well-scoped remediation pass. Marking the addressed comments as resolved. [resolve] ✏️ Learnings added
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/getting-started/quick-start.md`:
- Around line 92-96: Update the DPF provisioning examples in the quick-start
instructions to use shell-valid quoted placeholder values for
NICO_DPF_DPU_INTERFACE and NICO_DPF_DPU_CLUSTER_VIP, and replace the password
export with a silent read prompt that assigns NICO_DPF_BMC_ROOT_PASSWORD without
exposing it in shell history.
In `@docs/manuals/dpf.md`:
- Around line 373-383: Update both manual dpf-operator Helm commands in the
documentation to remove the unconditional
imagePullSecrets[0].name=dpf-pull-secret setting. Add concise guidance that
imagePullSecrets should be supplied only when using a private registry or
mirror, while keeping the anonymous public-image workflow as the default.
In `@helm-prereqs/setup.sh`:
- Around line 1023-1025: Update the idempotent rerun branches guarded by
_dpf_already_on to apply the supplied NICO_DPF_BMC_ROOT_PASSWORD instead of
skipping BMC credential handling. Ensure both occurrences reconcile the current
password while preserving the existing skip behavior for the DPF-off down-cycle.
🪄 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: 396af73f-a07b-4c78-a3c2-083512530764
📒 Files selected for processing (23)
.gitignorebook/src/configuration/configurability.mdcrates/dpf/README.mddocs/getting-started/quick-start.mddocs/manuals/dpf.mdhelm-prereqs/.helmignorehelm-prereqs/README.mdhelm-prereqs/clean.shhelm-prereqs/health-check.shhelm-prereqs/helmfile.yamlhelm-prereqs/operators/dpf/cert-manager-policy.yamlhelm-prereqs/operators/dpf/dpfoperatorconfig.yaml.tmplhelm-prereqs/operators/dpf/dpu-cluster-vip-service.yaml.tmplhelm-prereqs/operators/dpf/dpucluster.yaml.tmplhelm-prereqs/operators/values/argo-cd.yamlhelm-prereqs/operators/values/kamaji.yamlhelm-prereqs/operators/values/maintenance-operator.yamlhelm-prereqs/operators/values/node-feature-discovery.yamlhelm-prereqs/preflight.shhelm-prereqs/setup.shhelm-prereqs/values/nico-core.yamlhelm/charts/nico-api/templates/dpf-rbac.yamlhelm/charts/nico-api/values.yaml
🚧 Files skipped from review as they are similar to previous changes (17)
- helm-prereqs/.helmignore
- helm-prereqs/operators/values/maintenance-operator.yaml
- helm-prereqs/operators/values/node-feature-discovery.yaml
- helm-prereqs/operators/dpf/dpucluster.yaml.tmpl
- helm-prereqs/operators/dpf/dpfoperatorconfig.yaml.tmpl
- helm-prereqs/values/nico-core.yaml
- .gitignore
- helm/charts/nico-api/values.yaml
- book/src/configuration/configurability.md
- helm-prereqs/operators/dpf/cert-manager-policy.yaml
- helm-prereqs/operators/values/kamaji.yaml
- helm-prereqs/operators/dpf/dpu-cluster-vip-service.yaml.tmpl
- helm-prereqs/health-check.sh
- crates/dpf/README.md
- helm-prereqs/helmfile.yaml
- helm-prereqs/README.md
- helm-prereqs/clean.sh
Comment on lines
+92
to
+96
| # DPF DPU provisioning is installed by default. Set these three, or pass | ||
| # --skip-dpf to setup.sh (sites with no DPUs / still on iPXE): | ||
| export NICO_DPF_DPU_INTERFACE=<control-plane-nic> # NIC facing the DPUs | ||
| export NICO_DPF_DPU_CLUSTER_VIP=<free-routable-ip> # DPU cluster control-plane VIP | ||
| export NICO_DPF_BMC_ROOT_PASSWORD=<bmc-root-password> # site-wide BMC root password |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Use shell-valid, history-safe DPF variable examples.
The angle-bracket placeholders make these Bash assignments invalid when copied, and entering NICO_DPF_BMC_ROOT_PASSWORD directly in an export command records the secret in shell history. Use quoted example values for non-secrets and a silent read prompt for the password.
As per path instructions, Markdown commands/examples must be realistic and safe.
Suggested revision
-export NICO_DPF_DPU_INTERFACE=<control-plane-nic> # NIC facing the DPUs
-export NICO_DPF_DPU_CLUSTER_VIP=<free-routable-ip> # DPU cluster control-plane VIP
-export NICO_DPF_BMC_ROOT_PASSWORD=<bmc-root-password> # site-wide BMC root password
+export NICO_DPF_DPU_INTERFACE="enoX" # replace with the NIC facing the DPUs
+export NICO_DPF_DPU_CLUSTER_VIP="192.0.2.10" # replace with a free DPU-routable VIP
+read -r -s -p "DPF BMC root password: " NICO_DPF_BMC_ROOT_PASSWORD
+printf '\n'
+export NICO_DPF_BMC_ROOT_PASSWORD📝 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
| # DPF DPU provisioning is installed by default. Set these three, or pass | |
| # --skip-dpf to setup.sh (sites with no DPUs / still on iPXE): | |
| export NICO_DPF_DPU_INTERFACE=<control-plane-nic> # NIC facing the DPUs | |
| export NICO_DPF_DPU_CLUSTER_VIP=<free-routable-ip> # DPU cluster control-plane VIP | |
| export NICO_DPF_BMC_ROOT_PASSWORD=<bmc-root-password> # site-wide BMC root password | |
| # DPF DPU provisioning is installed by default. Set these three, or pass | |
| # --skip-dpf to setup.sh (sites with no DPUs / still on iPXE): | |
| export NICO_DPF_DPU_INTERFACE="enoX" # replace with the NIC facing the DPUs | |
| export NICO_DPF_DPU_CLUSTER_VIP="192.0.2.10" # replace with a free DPU-routable VIP | |
| read -r -s -p "DPF BMC root password: " NICO_DPF_BMC_ROOT_PASSWORD | |
| printf '\n' | |
| export NICO_DPF_BMC_ROOT_PASSWORD |
🤖 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/getting-started/quick-start.md` around lines 92 - 96, Update the DPF
provisioning examples in the quick-start instructions to use shell-valid quoted
placeholder values for NICO_DPF_DPU_INTERFACE and NICO_DPF_DPU_CLUSTER_VIP, and
replace the password export with a silent read prompt that assigns
NICO_DPF_BMC_ROOT_PASSWORD without exposing it in shell history.
Source: Path instructions
… setup.sh Automates deploying NVIDIA DOCA Platform Framework (DPF v26.4.0) as part of helm-prereqs/setup.sh so DPF-based DPU provisioning is enable-able this release. DPF installs by default; --skip-dpf opts out. - Prereqs: argo-cd, kamaji, maintenance-operator, node-feature-discovery as pinned helmfile releases (mirrors doca-platform prereqs.yaml), with preSync CRD-apply hooks and static values under operators/values/. - Kamaji cold-start deadlock recovery: breaks the DataStore/webhook chicken-and-egg on first install (out-of-band Issuer/Certificate/DataStore + failurePolicy toggle), plus kamaji controller HA via replicaCount. - DPF operator + CRs: clones doca-platform, installs the operator, applies DPFOperatorConfig / DPUCluster / DPU-cluster VIP Service (envsubst templates). - Two-phase Core: deploys DPF-off, sets the site-wide BMC root via an in-cluster nico-admin-cli Job (Vault-issued mTLS client cert, root token fed via stdin), then re-deploys DPF-on and restarts carbide-api, with idempotent re-run detection so a rerun never down-cycles an already-enabled site. - Build-and-push registry support (custom/self-built DPF image + version) with anonymous NGC pulls as the default; split public/private image handling. - clean.sh DPF teardown (ordered CR deletion + finalizer strip), preflight.sh and health-check.sh DPF coverage, and nico-api dpf-rbac chart template. - Docs: docs/manuals/dpf.md automated-install section, quick-start, README. Validated end-to-end on a dev cluster: clean teardown followed by a fresh reinstall brought all pods healthy with dpucluster Ready and DPF enabled. Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
Contributor
Author
|
Second CodeRabbit pass (on the fix commit) — all 3 addressed:
All scripts pass |
shayan1995
force-pushed
the
feat/setup-dpf-install
branch
from
301b826 to
01a7d54
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/manuals/dpf.md (1)
1029-1058: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the explicit service-to-artifact mapping requested previously.
The separate lists still leave
carbide-otelcol’s two-image mapping ambiguous. Add aService | Chart | Image(s)table covering all four services.Proposed mapping
+| Service | Chart | Image(s) | +| --- | --- | --- | +| `carbide-dpu-agent` | `nico-dpu-agent` | `forge-dpu-agent` | +| `carbide-dhcp-server` | `nico-dhcp-server` | `forge-dhcp-server` | +| `carbide-fmds` | `nico-fmds` | `carbide-fmds` | +| `carbide-otelcol` | `nico-otelcol` | `forge-dpu-otel-agent`, `otelcol-contrib` |As per path instructions, documentation must be technically correct and operationally usable.
🤖 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/manuals/dpf.md` around lines 1029 - 1058, Add a Service | Chart | Image(s) table in the DPF deployment documentation covering all four mandatory services: carbide-api’s carbide-dpu-agent, carbide-dhcp-server, carbide-fmds, and carbide-otelcol. Map each service to its corresponding chart under bluefield/charts and list every image it deploys, explicitly showing carbide-otelcol’s two-image mapping; keep the artifact names consistent with the build commands and configuration blocks.Source: Path instructions
🤖 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 `@helm-prereqs/setup.sh`:
- Around line 654-658: Update the hbn-user-password creation block to avoid
passing the generated password via the kubectl command-line arguments. Reuse the
existing secret-safe off-argv pattern in setup.sh, while preserving the current
namespace, secret name, and generated password behavior.
- Around line 211-217: Update the EXIT cleanup block in setup.sh to terminate
the dpf-set-bmc-root credential Job before deleting dpf-bmc-root-pw and
dpf-admincli-cert. Keep the cleanup scoped to INSTALL_DPF=true, make the Job
termination best-effort so cleanup continues, and preserve deletion of both
Secrets afterward.
---
Duplicate comments:
In `@docs/manuals/dpf.md`:
- Around line 1029-1058: Add a Service | Chart | Image(s) table in the DPF
deployment documentation covering all four mandatory services: carbide-api’s
carbide-dpu-agent, carbide-dhcp-server, carbide-fmds, and carbide-otelcol. Map
each service to its corresponding chart under bluefield/charts and list every
image it deploys, explicitly showing carbide-otelcol’s two-image mapping; keep
the artifact names consistent with the build commands and configuration blocks.
🪄 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: b28f6ead-dae6-4252-bd37-c656c0e45868
📒 Files selected for processing (23)
.gitignorebook/src/configuration/configurability.mdcrates/dpf/README.mddocs/getting-started/quick-start.mddocs/manuals/dpf.mdhelm-prereqs/.helmignorehelm-prereqs/README.mdhelm-prereqs/clean.shhelm-prereqs/health-check.shhelm-prereqs/helmfile.yamlhelm-prereqs/operators/dpf/cert-manager-policy.yamlhelm-prereqs/operators/dpf/dpfoperatorconfig.yaml.tmplhelm-prereqs/operators/dpf/dpu-cluster-vip-service.yaml.tmplhelm-prereqs/operators/dpf/dpucluster.yaml.tmplhelm-prereqs/operators/values/argo-cd.yamlhelm-prereqs/operators/values/kamaji.yamlhelm-prereqs/operators/values/maintenance-operator.yamlhelm-prereqs/operators/values/node-feature-discovery.yamlhelm-prereqs/preflight.shhelm-prereqs/setup.shhelm-prereqs/values/nico-core.yamlhelm/charts/nico-api/templates/dpf-rbac.yamlhelm/charts/nico-api/values.yaml
🚧 Files skipped from review as they are similar to previous changes (16)
- helm-prereqs/operators/dpf/dpucluster.yaml.tmpl
- .gitignore
- helm-prereqs/.helmignore
- helm-prereqs/operators/values/argo-cd.yaml
- helm-prereqs/operators/values/node-feature-discovery.yaml
- helm-prereqs/operators/dpf/dpu-cluster-vip-service.yaml.tmpl
- helm-prereqs/operators/dpf/dpfoperatorconfig.yaml.tmpl
- crates/dpf/README.md
- helm-prereqs/operators/dpf/cert-manager-policy.yaml
- helm-prereqs/operators/values/kamaji.yaml
- helm-prereqs/values/nico-core.yaml
- book/src/configuration/configurability.md
- helm-prereqs/helmfile.yaml
- helm-prereqs/README.md
- docs/getting-started/quick-start.md
- helm-prereqs/clean.sh
Comment on lines
+211
to
+217
| # Drop the ephemeral BMC-root + admin-cert Secrets if a mid-run errexit | ||
| # skipped _dpf_set_bmc_root's own cleanup — the plaintext site-wide BMC | ||
| # password must never linger in the cluster after setup exits. | ||
| if [[ "${INSTALL_DPF:-false}" == "true" ]]; then | ||
| kubectl delete secret dpf-bmc-root-pw dpf-admincli-cert -n nico-system \ | ||
| --ignore-not-found >/dev/null 2>&1 || true | ||
| fi |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Terminate the credential Job from EXIT cleanup before deleting its Secrets.
An interrupt or unrelated errexit during polling leaves dpf-set-bmc-root running with the site-wide BMC password already loaded into its environment.
Proposed fix
if [[ "${INSTALL_DPF:-false}" == "true" ]]; then
+ kubectl delete job dpf-set-bmc-root -n nico-system \
+ --ignore-not-found --wait=true --timeout=30s \
+ >/dev/null 2>&1 || true
kubectl delete secret dpf-bmc-root-pw dpf-admincli-cert -n nico-system \
--ignore-not-found >/dev/null 2>&1 || true
fiAs per path instructions, shell scripts must propagate failures safely and handle secrets securely.
🤖 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 `@helm-prereqs/setup.sh` around lines 211 - 217, Update the EXIT cleanup block
in setup.sh to terminate the dpf-set-bmc-root credential Job before deleting
dpf-bmc-root-pw and dpf-admincli-cert. Keep the cleanup scoped to
INSTALL_DPF=true, make the Job termination best-effort so cleanup continues, and
preserve deletion of both Secrets afterward.
Source: Path instructions
Comment on lines
+654
to
+658
| if ! kubectl get secret hbn-user-password -n dpf-operator-system &>/dev/null; then | ||
| kubectl create secret generic hbn-user-password \ | ||
| --namespace dpf-operator-system \ | ||
| --from-literal=password="$(LC_ALL=C tr -dc 'a-z0-9' < /dev/urandom | head -c 10)" | ||
| fi |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep the generated HBN password out of kubectl arguments.
--from-literal=password="$(...)" exposes this credential through the local process table. Use the existing off-argv pattern.
Proposed fix
kubectl create secret generic hbn-user-password \
--namespace dpf-operator-system \
- --from-literal=password="$(LC_ALL=C tr -dc 'a-z0-9' < /dev/urandom | head -c 10)"
+ --from-file=password=<(LC_ALL=C tr -dc 'a-z0-9' < /dev/urandom | head -c 10)As per path instructions, shell scripts must use secret-safe logging and argument handling.
🤖 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 `@helm-prereqs/setup.sh` around lines 654 - 658, Update the hbn-user-password
creation block to avoid passing the generated password via the kubectl
command-line arguments. Reuse the existing secret-safe off-argv pattern in
setup.sh, while preserving the current namespace, secret name, and generated
password behavior.
Source: Path instructions
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.
Related issues
Closes #2897 — docs: Document standalone NICo control plane setup with DPF provisioning.
Type of Change
Breaking Changes
DPF now installs by default, so existing
setup.shruns will fail preflight unless they either set the DPF env vars (NICO_DPF_DPU_INTERFACE,NICO_DPF_DPU_CLUSTER_VIP,NICO_DPF_BMC_ROOT_PASSWORD) or add--skip-dpfto keep the previous behavior.Testing
Clean teardown + full reinstall on a dev cluster (DPF default-on): all pods healthy,
dpucluster: Ready=True, kamaji HA 2/2,[dpf] enabled = true.Additional Notes
DPF v26.4.0 via setup.sh;
--skip-dpfopts out. Known upstream (kamaji cold-start deadlock, BYO-ArgoCD, clone-unusable operator chart) and NICo-internal (BMC two-phase, SDK doesn't create its CRs, no config-checksum restart) items are worked around here and catalogued for separate issues — none block this PR.