chore: Select DPF deployment type of Bf4Generic or Bf4Astra based on … - #4871
Open
srinivasadmurthy wants to merge 1 commit into
Open
chore: Select DPF deployment type of Bf4Generic or Bf4Astra based on …#4871srinivasadmurthy wants to merge 1 commit into
srinivasadmurthy wants to merge 1 commit into
Conversation
…whether host machine has Astra NICs or not. Signed-off-by: Srinivasa Murthy <srmurthy@nvidia.com>
Contributor
Summary by CodeRabbit
WalkthroughThe DPF deployment-type API now accepts Astra NIC presence. DPF handlers query DPA interfaces and pass this state through provisioning and state handling. BF4 DPUs resolve to ChangesDPF Astra classification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DPFStateHandler
participant DPAInterfaceDatabase
participant DpfOperations
participant DpfSdkOps
DPFStateHandler->>DPAInterfaceDatabase: Query host DPA interfaces
DPAInterfaceDatabase-->>DPFStateHandler: Return Astra NIC presence
DPFStateHandler->>DpfOperations: deployment_type_for_dpu(dpu, astra_nics)
DpfOperations->>DpfSdkOps: Forward classification input
DpfSdkOps-->>DPFStateHandler: Return deployment type
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 1
🧹 Nitpick comments (1)
crates/machine-controller/src/dpf.rs (1)
93-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new BF4 classification input.
The trait documentation still describes only BF3 versus BF4.
astra_nicsis now part of the classification result:trueselectsBf4Astra, andfalseselectsBf4Generic. Update the contract documentation so callers and implementors cannot miss this rule.Suggested documentation update
- /// Resolve the deployment type of a DPU based on its hardware (BF3 vs BF4). + /// Resolve the deployment type of a DPU based on its hardware and host Astra NICs. + /// + /// For BF4 DPUs, `astra_nics = true` selects `DpuDeploymentType::Bf4Astra`; + /// `false` selects `DpuDeploymentType::Bf4Generic`.🤖 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 `@crates/machine-controller/src/dpf.rs` around lines 93 - 97, Update the trait documentation for deployment_type_for_dpu to document the astra_nics parameter: when true, BF4 classification returns Bf4Astra, and when false, it returns Bf4Generic. Preserve the existing BF3 classification description.
🤖 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 `@crates/machine-controller/src/handler/dpf.rs`:
- Around line 181-182: Update handle_dpf_state to resolve host_has_astra_nics
once and pass the resulting astra_nics value through
create_and_register_dpudevices_and_dpunode and the reprovisioning helpers before
any DPUDevice or DPUNODE resources are registered; remove repeated lookups and
reuse the passed value throughout provisioning.
---
Nitpick comments:
In `@crates/machine-controller/src/dpf.rs`:
- Around line 93-97: Update the trait documentation for deployment_type_for_dpu
to document the astra_nics parameter: when true, BF4 classification returns
Bf4Astra, and when false, it returns Bf4Generic. Preserve the existing BF3
classification description.
🪄 Autofix
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: 51f9b736-f78f-4372-8ab1-7957bc404161
📒 Files selected for processing (7)
crates/api-core/src/tests/dpf/happy_path.rscrates/api-core/src/tests/dpf/reprovisioning.rscrates/api-core/src/tests/dpf/stale_labels.rscrates/api-core/src/tests/dpf/waiting_for_ready.rscrates/api-core/src/tests/machine_admin_force_delete.rscrates/machine-controller/src/dpf.rscrates/machine-controller/src/handler/dpf.rs
Comment on lines
+181
to
182
| ctx: &mut StateHandlerContext<'_, MachineStateHandlerContextObjects>, | ||
| ) -> Result<(), StateHandlerError> { |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 12 \
'dpf_cr_creation_failed|register_dpu_device|register_dpu_node|create_and_register_dpudevices_and_dpunode' \
crates --glob '*.rs'Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="crates/machine-controller/src/handler/dpf.rs"
wc -l "$file"
ast-grep outline "$file" | sed -n '1,220p'
printf '\n--- provisioning and failure paths ---\n'
rg -n -C 20 \
'create_and_register_dpudevices_and_dpunode|dpf_cr_creation_failed|register_dpu_device|register_dpu_node|host_has_astra_nics|handle_dpf_state' \
"$file"
printf '\n--- cleanup and idempotency implementations ---\n'
rg -n -C 18 \
'dpf_cr_creation_failed|delete_dpu_node|delete_dpu_device|register_dpu_device|register_dpu_node' \
crates/dpf/src/sdk.rs crates/machine-controller/src --glob '*.rs' | sed -n '1,260p'Repository: NVIDIA/infra-controller
Length of output: 28812
Resolve Astra NIC state before DPF writes and reuse it.
create_and_register_dpudevices_and_dpunode registers DPUDevice resources before host_has_astra_nics. If the lookup fails, the handler transitions to a failed state without automatic cleanup and can leave partial DPF resources. Pass the already computed astra_nics value from handle_dpf_state through the provisioning and reprovisioning helpers.
🤖 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 `@crates/machine-controller/src/handler/dpf.rs` around lines 181 - 182, Update
handle_dpf_state to resolve host_has_astra_nics once and pass the resulting
astra_nics value through create_and_register_dpudevices_and_dpunode and the
reprovisioning helpers before any DPUDevice or DPUNODE resources are registered;
remove repeated lookups and reuse the passed value throughout provisioning.
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.
…whether
host machine has Astra NICs or not.
When determining DPF deployment type, if the host machine
has Astra NICs, choose Bf4Astra instead of Bf4Generic.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes