fix(api-core,dpf): Add BF3 and generic BF4 intercept bridging parity for DPF - #4570
fix(api-core,dpf): Add BF3 and generic BF4 intercept bridging parity for DPF#4570bcavnvidia wants to merge 1 commit into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Summary by CodeRabbit
WalkthroughThe change adds typed DPF intercept-bridging topology, effective interface and SF-capacity calculation, deployment-scoped ServiceInterfaces, topology-aware BF3 and BF4 provisioning, defensive VF ID conversion, and instance VF admission validation. ChangesDPF-aware VMaaS bridging
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant InstanceAPI
participant CarbideConfig
participant DpfTopology
participant DpfSdk
participant KubernetesRepository
InstanceAPI->>CarbideConfig: receive instance VF request
CarbideConfig->>DpfTopology: validate selected VF identities
DpfTopology-->>InstanceAPI: accept or reject VF set
DpfSdk->>DpfTopology: resolve effective interfaces and SF capacity
DpfSdk->>KubernetesRepository: create scoped DPF resources
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
7ba5145 to
7437528
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/dpf/src/flavor.rs (1)
1488-1506: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd deployment-specific validation for BF3 PF IDs.
DpfInterceptBridging::newacceptspf_id: 3, and BF3 renders it aspf3hpf. BF3 nvconfig setsNUM_OF_PF=1, so reject unsupported PF identities before generating the BF3 bootstrap. Keep this check BF3-specific because generic BF4 usespf_id: 3. Otherwise|| truehides the OVS attach failure. Add a BF3 out-of-range validation test.🤖 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/dpf/src/flavor.rs` around lines 1488 - 1506, Add BF3-specific PF ID validation in DpfInterceptBridging::new before BF3 bootstrap generation, rejecting pf_id values unsupported by the BF3 nvconfig NUM_OF_PF=1 configuration while preserving pf_id 3 support for generic BF4. Add a test covering the BF3 out-of-range PF ID rejection.crates/rpc/src/model/instance/config/network.rs (1)
244-261: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale overflow comment; it now contradicts the code.
The change itself is the right fix. Previously
virtual_function_id = 256truncated to0and was silently accepted as VF0, aliasing a distinct virtual function. The conversion now rejects it. The comment on lines 244-247 still documents the removed behaviour, stating that overflow is acceptable and that the intent is only to avoid a crash. A reader who trusts that comment will conclude truncation is still tolerated on this path.🧹 Proposed comment correction
let function_id = match iface_type { InterfaceFunctionType::Physical => InterfaceFunctionId::Physical {}, InterfaceFunctionType::Virtual => { - // Note that this might overflow if the RPC call delivers more than - // 256 VFs. However that's ok - the `InstanceNetworkConfig.validate()` - // call will declare those configs as invalid later on anyway. - // We mainly don't want to crash here. + // Carbide-allocated IDs are assigned locally and saturate; cloud-supplied + // IDs are range-checked, so a wide wire value can no longer alias a VF. InterfaceFunctionId::Virtual {🤖 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/rpc/src/model/instance/config/network.rs` around lines 244 - 261, Remove the stale overflow comment above the InterfaceFunctionId::Virtual conversion, including the statements that overflow is acceptable and that avoiding a crash is the goal. Keep the current checked conversion behavior in convert_wire_virtual_function_id unchanged.crates/dpf/src/sdk.rs (1)
1380-1413: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the legacy
Pf/Vfinterface spec on the unscoped path.The previous implementation emitted
nic_selector: None. The shared builder now emitscontroller_number: Some(1)for everyPfandVf, includingbuild_service_interfaceand unscoped initialization. This changes existing BF3/BF4 resources and can trigger reconciliation on upgrade. Emit the selector only for scoped interfaces.🤖 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/dpf/src/sdk.rs` around lines 1380 - 1413, Update the Pf and Vf branches in the shared interface-spec builder used by build_service_interface and unscoped initialization so unscoped interfaces retain nic_selector: None. Only populate the DPU nic selector with controller_number Some(1) when the interface is scoped, while preserving the existing Pf/Vf fields and parent-interface behavior.
🧹 Nitpick comments (5)
crates/dpf/src/test/sdk_initialization.rs (2)
510-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
assert_no_initialization_crshere.The test asserts the no-partial-write property, but it checks only three of the eight CR stores.
service_templates,service_configs,nads,service_interfaces, andbluefield_softwaresremain unverified, so a regression that writes a ServiceInterface before the capacity preflight would still pass. The helper defined at line 144 covers all of them.♻️ Proposed change
assert!(matches!(result, Err(DpfError::ConfigError(_)))); assert!(mock.secrets.is_empty()); - assert!(mock.bfbs.is_empty()); - assert!(mock.flavors.is_empty()); - assert!(mock.deployments.is_empty()); + assert_no_initialization_crs(&mock); }🤖 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/dpf/src/test/sdk_initialization.rs` around lines 510 - 513, Replace the individual store-emptiness assertions in the affected test with the existing assert_no_initialization_crs helper, ensuring all CR stores are validated for the no-partial-write property.
1027-1034: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLabel the assertions with the active case.
The loop runs two cases, and both assertions panic with the same message. On failure the reader cannot tell which case regressed without reading the source. Add
existing_nameto the messages.♻️ Proposed change
let existing_after = DpuServiceInterfaceRepository::get(&mock, existing_name, TEST_NS) .await .unwrap() - .expect("pre-existing ServiceInterface must remain"); + .unwrap_or_else(|| panic!("pre-existing ServiceInterface {existing_name} must remain")); assert_eq!( serde_json::to_value(existing_after).unwrap(), - existing_snapshot + existing_snapshot, + "initialization must not mutate {existing_name}" );🤖 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/dpf/src/test/sdk_initialization.rs` around lines 1027 - 1034, Add existing_name to the failure messages for the pre-existing ServiceInterface assertions in the loop, including the expect and assert_eq! around existing_after, so failures identify the active case while preserving the current validation.crates/dpf/src/flavor.rs (2)
331-339: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the legacy default VF count into a named constant.
The literal
16now appears indefault_flavor_for(line 335),flavor_bf4(line 390), anddefault_flavor(line 646), and it must stay equal toInitDpfResourcesConfig::default().num_of_vfs. A shared constant next toDEFAULT_PF_TOTAL_SF_RESERVEDdocuments that this value is hash-stable legacy behaviour and prevents the three sites from drifting apart.♻️ Proposed change
+// in crates/dpf/src/types.rs, beside DEFAULT_PF_TOTAL_SF_RESERVED +/// Legacy VF population assumed by inventory-free flavor construction. +pub const DEFAULT_NUM_OF_VFS: u32 = 16;default_flavor_for_with_topology( namespace, proxy, deployment_type, - 16, + DEFAULT_NUM_OF_VFS, DEFAULT_PF_TOTAL_SF_RESERVED, None, None, )🤖 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/dpf/src/flavor.rs` around lines 331 - 339, Define a shared named constant beside DEFAULT_PF_TOTAL_SF_RESERVED for the legacy default VF count, set to the value of InitDpfResourcesConfig::default().num_of_vfs, and replace the literal 16 in default_flavor_for, flavor_bf4, and default_flavor with that constant.
190-211: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDiscarded
writeln!results across the new script renderers. Both new rendering functions build their shell scripts withlet _ = writeln!(...). The style guide forbids discarding aResultthrough an underscore binding and requires.ok()for an intentional discard. Writing into aStringis infallible, so the change is mechanical and does not alter behaviour.
crates/dpf/src/flavor.rs#L190-L211: replace the fivelet _ = writeln!(script, ...)statements inappend_peer_bridge_bootstrapwithwriteln!(script, ...).ok();.crates/dpf/src/flavor.rs#L252-L270: replace the twolet _ = writeln!(script, ...)statements inrender_bf4_pf_preflightwithwriteln!(script, ...).ok();.As per coding guidelines: "Do not use
let _ =or underscore bindings to discard errors; when intentionally discarding aResult, use.ok()and ensure operational failures remain observable where appropriate".🤖 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/dpf/src/flavor.rs` around lines 190 - 211, In crates/dpf/src/flavor.rs lines 190-211, update all five writeln! calls in append_peer_bridge_bootstrap to discard results with .ok() instead of let _ bindings. Apply the same change to the two writeln! calls in render_bf4_pf_preflight at lines 252-270; no other behavior changes are needed.Source: Coding guidelines
crates/dpf/README.md (1)
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReconcile the DPUSet/DPUDeployment terminology and document the new initialization inputs.
Two points on this step.
First, the CRD list earlier in this document presents DPUSet as the object that references BFB and DPUFlavor. This step now presents DPUDeployment as the referencing object. Both are true at different layers, but the document never states the relationship. Add a short clause that DPUDeployment owns the DPUSet, or align the terminology.
Second,
create_initialization_objectsgained a materially larger contract in this change:intercept_bridging,interfaces,num_of_vfs,pf_total_sf_reserved, anddeployment_scoped_service_interfaces. The last one is documented in the type as requiring manual cleanup and DPU re-ingestion on transition, andpf_total_sf_reservedhas a documented default of 30. None of that appears here. Record the accepted values, defaults, and the migration constraint on the scoping flag.As per coding guidelines: "Document interface contracts completely, including spelling, requiredness, defaults, accepted values, units, bounds, interactions, ordering, fallback behavior, outputs, side effects, errors, and unsupported paths" and "When changing a documented fact or behavior, search all relevant documentation surfaces, reconcile conflicting occurrences or establish and link to one canonical explanation".
🤖 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/dpf/README.md` around lines 19 - 21, Update the README step describing create_initialization_objects to state that DPUDeployment owns the DPUSet, then document the inputs intercept_bridging, interfaces, num_of_vfs, pf_total_sf_reserved, and deployment_scoped_service_interfaces with their accepted values, defaults, and relevant constraints, including the default of 30 for pf_total_sf_reserved and the manual cleanup plus DPU re-ingestion required when the scoping flag changes.Source: Coding guidelines
🤖 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/dpf/src/flavor.rs`:
- Around line 883-895: Update ovn_encap_ip_commands so the ovs-vsctl write
explicitly terminates with a nonzero status when it fails, ensuring
rawConfigScript and systemd oneshot paths share fail-fast behavior. Preserve the
existing successful command flow and IPv4 validation; extend coverage with a
failing ovs-vsctl stub if appropriate.
- Around line 1552-1569: The custom interfaces handling around the
Patch-interface iteration must validate that a non-empty inventory contains at
least one Patch entry chained to DOCA_HBN_SERVICE_NAME when intercept_bridging
is enabled. Reject the custom inventory during initialization, or otherwise
route it through the effective inventory projection, instead of allowing
dhcp_acl_rules to render without required HBN endpoints; preserve append_rule
behavior for valid entries.
In `@crates/dpf/src/types.rs`:
- Around line 378-392: Add a test scenario to
dpf_intercept_bridging_validates_identity_and_ovs_contracts using a valid-range
vf_id such as 8 and num_of_vfs set below it, such as 4. Assert that validation
returns the existing ConfigError for selecting a VF outside num_of_vfs, ensuring
this reaches the second gate rather than MAX_INSTANCE_VF_ID.
In `@crates/machine-controller/src/dpf.rs`:
- Around line 48-50: Update the documentation comment for HOST_BMC_IP_LABEL to
state that the label is populated with the host BMC address on both DPUDevice
and DPUNode resources, reflecting its usages in the surrounding registration
logic.
---
Outside diff comments:
In `@crates/dpf/src/flavor.rs`:
- Around line 1488-1506: Add BF3-specific PF ID validation in
DpfInterceptBridging::new before BF3 bootstrap generation, rejecting pf_id
values unsupported by the BF3 nvconfig NUM_OF_PF=1 configuration while
preserving pf_id 3 support for generic BF4. Add a test covering the BF3
out-of-range PF ID rejection.
In `@crates/dpf/src/sdk.rs`:
- Around line 1380-1413: Update the Pf and Vf branches in the shared
interface-spec builder used by build_service_interface and unscoped
initialization so unscoped interfaces retain nic_selector: None. Only populate
the DPU nic selector with controller_number Some(1) when the interface is
scoped, while preserving the existing Pf/Vf fields and parent-interface
behavior.
In `@crates/rpc/src/model/instance/config/network.rs`:
- Around line 244-261: Remove the stale overflow comment above the
InterfaceFunctionId::Virtual conversion, including the statements that overflow
is acceptable and that avoiding a crash is the goal. Keep the current checked
conversion behavior in convert_wire_virtual_function_id unchanged.
---
Nitpick comments:
In `@crates/dpf/README.md`:
- Around line 19-21: Update the README step describing
create_initialization_objects to state that DPUDeployment owns the DPUSet, then
document the inputs intercept_bridging, interfaces, num_of_vfs,
pf_total_sf_reserved, and deployment_scoped_service_interfaces with their
accepted values, defaults, and relevant constraints, including the default of 30
for pf_total_sf_reserved and the manual cleanup plus DPU re-ingestion required
when the scoping flag changes.
In `@crates/dpf/src/flavor.rs`:
- Around line 331-339: Define a shared named constant beside
DEFAULT_PF_TOTAL_SF_RESERVED for the legacy default VF count, set to the value
of InitDpfResourcesConfig::default().num_of_vfs, and replace the literal 16 in
default_flavor_for, flavor_bf4, and default_flavor with that constant.
- Around line 190-211: In crates/dpf/src/flavor.rs lines 190-211, update all
five writeln! calls in append_peer_bridge_bootstrap to discard results with
.ok() instead of let _ bindings. Apply the same change to the two writeln! calls
in render_bf4_pf_preflight at lines 252-270; no other behavior changes are
needed.
In `@crates/dpf/src/test/sdk_initialization.rs`:
- Around line 510-513: Replace the individual store-emptiness assertions in the
affected test with the existing assert_no_initialization_crs helper, ensuring
all CR stores are validated for the no-partial-write property.
- Around line 1027-1034: Add existing_name to the failure messages for the
pre-existing ServiceInterface assertions in the loop, including the expect and
assert_eq! around existing_after, so failures identify the active case while
preserving the current validation.
🪄 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: 9eecfaf6-79d1-4bd9-92c9-70073d46512c
📒 Files selected for processing (17)
crates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/instance/mod.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/instance.rscrates/dpf/README.mdcrates/dpf/src/flavor.rscrates/dpf/src/lib.rscrates/dpf/src/sdk.rscrates/dpf/src/test/sdk_initialization.rscrates/dpf/src/types.rscrates/machine-controller/src/dpf.rscrates/rpc/src/errors.rscrates/rpc/src/model/instance/config/network.rs
7437528 to
2b305c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
crates/dpf/src/flavor.rs (2)
1801-1815: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering a topology that spans two distinct PFs.
Every BF4 preflight test uses a single PF identity (
c2pf3). The duplicate-netdev cross-check emitted at Lines 261-272 and the multi-PF resolution ordering are therefore never exercised. Add one case with two controller/PF pairs to confirm bothresolve_dpf_pfcalls precede the first OVS mutation and that the emitted comparison is well formed.🤖 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/dpf/src/flavor.rs` around lines 1801 - 1815, Extend the BF4 preflight tests around bf4_intercept_bridging_preflight_precedes_all_ovs_mutation to use a topology containing two distinct controller/PF pairs instead of a single c2pf3 identity. Assert both corresponding resolve_dpf_pf calls occur before the first ovs-vsctl mutation, and verify the emitted duplicate-netdev cross-check comparison is well formed.
2429-2465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the two OVN encapsulation paths to the base-path assertions.
config_file_count_depends_on_proxynow expects nine base files, butbase_config_file_paths_are_presentstill lists only the seven original paths. A future change that renamesOVN_ENCAP_SCRIPT_PATHor the unit path keeps the count at nine and passes both tests. Add the two paths so the contract is pinned.💚 Proposed addition
"mlnx-sf.conf" { "/etc/mellanox/mlnx-sf.conf" => true, } + + "OVN encapsulation script" { + OVN_ENCAP_SCRIPT_PATH => true, + } + + "OVN encapsulation unit" { + "/etc/systemd/system/nico-ovn-encap-ip.service" => true, + } );🤖 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/dpf/src/flavor.rs` around lines 2429 - 2465, Add assertions for both OVN encapsulation paths in the base_config_file_paths_are_present value_scenarios list, using the existing OVN_ENCAP_SCRIPT_PATH and unit-path symbols. Keep the current seven path assertions unchanged so the test explicitly covers all nine base files.crates/dpf/src/sdk.rs (1)
2928-2942: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a preflight test for the Astra scoping guard.
resolve_initialization_inventoryrejectsBf4Astrawhendeployment_scoped_service_interfacesis false (line 1310). No test in this module exercises that guard.The neighbouring test states the reason this coverage matters: direct SDK callers do not pass through api-core configuration. The api-core test
dpf_service_interface_scoping_gates_astracoversvalidate_service_interface_scoping, which is a different function in a different crate, so it does not protect this path.Without the guard, Astra's static inventory is written to unsuffixed, globally selected ServiceInterfaces and collides with BF3 and generic BF4 resources. Please pin it alongside the platform-limit test.
💚 Proposed test for the Astra scoping guard
+ /// Verifies Astra cannot initialize into the legacy global ServiceInterface namespace. + #[test] + fn initialization_rejects_unscoped_astra_deployment() { + // Astra's static inventory would otherwise overwrite BF3 and generic-BF4 resources. + let config = InitDpfResourcesConfig { + deployment_type: DpuDeploymentType::Bf4Astra, + deployment_scoped_service_interfaces: false, + ..Default::default() + }; + + // Pure preflight runs before the SDK writes its shared BMC Secret. + assert!(matches!( + resolve_initialization_inventory(&config), + Err(DpfError::ConfigError(message)) + if message.contains("BF4 Astra requires deployment_scoped_service_interfaces=true") + )); + } +🤖 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/dpf/src/sdk.rs` around lines 2928 - 2942, Add a neighbouring unit test for resolve_initialization_inventory that configures Bf4Astra with deployment_scoped_service_interfaces set to false and asserts it returns DpfError::ConfigError with the expected scoping-related message. Keep the test focused on the SDK preflight guard and preserve the existing platform-limit test.
🤖 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/dpf/src/flavor.rs`:
- Around line 174-180: Update append_ovn_encap_ip_bootstrap to execute the
shared ovn_encap_ip_commands body in a subshell, preserving its exit 1 behavior
for the systemd oneshot while preventing rawConfigScript from failing. Adjust
ovs_bootstrap_ends_with_ovn_encap_ip_configuration to expect the
subshell-wrapped shared text, keeping the bootstrap path best-effort.
- Around line 188-211: Replace each `let _ = writeln!(...)` in the topology
rendering loop with `writeln!(...).ok()`, preserving the existing script output.
Apply the same change to all corresponding `writeln!` calls in
`render_bf4_pf_preflight`, including the ranges around the referenced lines, so
no underscore bindings discard their Results.
In `@crates/dpf/src/sdk.rs`:
- Around line 1339-1345: Update the projection-mismatch error in the config
validation block to identify the first differing interface name, while retaining
the received and expected counts. Compare config.interfaces with projected in
order, capture the first mismatching entry, and include its interface name in
the DpfError::ConfigError message; preserve the existing validation behavior.
---
Nitpick comments:
In `@crates/dpf/src/flavor.rs`:
- Around line 1801-1815: Extend the BF4 preflight tests around
bf4_intercept_bridging_preflight_precedes_all_ovs_mutation to use a topology
containing two distinct controller/PF pairs instead of a single c2pf3 identity.
Assert both corresponding resolve_dpf_pf calls occur before the first ovs-vsctl
mutation, and verify the emitted duplicate-netdev cross-check comparison is well
formed.
- Around line 2429-2465: Add assertions for both OVN encapsulation paths in the
base_config_file_paths_are_present value_scenarios list, using the existing
OVN_ENCAP_SCRIPT_PATH and unit-path symbols. Keep the current seven path
assertions unchanged so the test explicitly covers all nine base files.
In `@crates/dpf/src/sdk.rs`:
- Around line 2928-2942: Add a neighbouring unit test for
resolve_initialization_inventory that configures Bf4Astra with
deployment_scoped_service_interfaces set to false and asserts it returns
DpfError::ConfigError with the expected scoping-related message. Keep the test
focused on the SDK preflight guard and preserve the existing platform-limit
test.
🪄 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: df2888e5-83ac-4e3b-9703-c6d799171ad9
📒 Files selected for processing (7)
crates/api-core/src/cfg/file.rscrates/dpf/src/flavor.rscrates/dpf/src/lib.rscrates/dpf/src/sdk.rscrates/dpf/src/types.rscrates/machine-controller/src/dpf.rscrates/rpc/src/model/instance/config/network.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/dpf/src/lib.rs
- crates/machine-controller/src/dpf.rs
- crates/rpc/src/model/instance/config/network.rs
- crates/api-core/src/cfg/file.rs
2b305c5 to
7de8618
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/api-core/src/dpf_services.rs (1)
357-371: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging when an operator overlay replaces
resourceswith a non-object value.Line 364 detects that
resourcesis not an object and replaces it with an empty object. The operator-supplied value is then discarded without any signal. The SF-count contract is preserved, which is correct, but the discarded overlay is invisible to the operator.Emit a
tracing::warn!in that branch so the discarded overlay remains observable.The two
expectcalls are acceptable here. Line 361 holds because the generated Helm values are an object literal andapply_helm_valuesmerges into that map. Line 369 holds because the preceding normalization guarantees an object.♻️ Proposed change
if !resources.is_object() { + tracing::warn!( + service = "doca_hbn", + "replacing non-object HBN resources overlay with generated values" + ); *resources = serde_json::json!({}); }🤖 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/api-core/src/dpf_services.rs` around lines 357 - 371, Add a tracing::warn! call in set_hbn_sf_count when resources is non-object before replacing it with an empty object, indicating that the operator-supplied resources overlay was discarded. Preserve the existing normalization and SF-count insertion behavior.crates/dpf/src/flavor.rs (1)
1656-1681: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the synthetic sysfs fixture even when
bashexecution fails.
run_bf4_preflightdeletes the fixture directory only afterCommand::output()returns successfully. If theexpecton line 1678 panics, the directory stays in the system temp directory. Each run creates a new UUID-named directory, so failures accumulate.Move the cleanup so it runs on both outcomes.
♻️ Proposed change
let script = format!("{prefix}\n{}\n{suffix}", render_bf4_pf_preflight(topology)); let output = Command::new("bash") .arg("-c") .arg(script) .env("NICO_SYS_CLASS_NET", &fixture) - .output() - .expect("bash must execute synthetic BF4 preflight"); - fs::remove_dir_all(&fixture).expect("synthetic BF4 sysfs fixture must be removed"); - output + .output(); + fs::remove_dir_all(&fixture).expect("synthetic BF4 sysfs fixture must be removed"); + output.expect("bash must execute synthetic BF4 preflight")🤖 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/dpf/src/flavor.rs` around lines 1656 - 1681, Update run_bf4_preflight so the temporary fixture cleanup runs regardless of whether Command::output succeeds or panics, while preserving the existing expect behavior and returned Output on success. Ensure fs::remove_dir_all is guarded by cleanup logic that executes on both outcomes.
🤖 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.
Nitpick comments:
In `@crates/api-core/src/dpf_services.rs`:
- Around line 357-371: Add a tracing::warn! call in set_hbn_sf_count when
resources is non-object before replacing it with an empty object, indicating
that the operator-supplied resources overlay was discarded. Preserve the
existing normalization and SF-count insertion behavior.
In `@crates/dpf/src/flavor.rs`:
- Around line 1656-1681: Update run_bf4_preflight so the temporary fixture
cleanup runs regardless of whether Command::output succeeds or panics, while
preserving the existing expect behavior and returned Output on success. Ensure
fs::remove_dir_all is guarded by cleanup logic that executes on both outcomes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: de4f786b-9084-4d23-afd3-1494656ac332
📒 Files selected for processing (3)
crates/api-core/src/dpf_services.rscrates/dpf/src/flavor.rscrates/dpf/src/sdk.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dpf/src/sdk.rs
7de8618 to
7cec830
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4570.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/dpf/src/flavor.rs (2)
246-264: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the
NICO_SYS_CLASS_NEToverride from the rendered production script.Line 247 renders
sys_class_net=${NICO_SYS_CLASS_NET:-/sys/class/net}into the DPU-siderawConfigScript. The override exists to letrun_bf4_preflightpoint discovery at a synthetic sysfs tree. It also ships to every provisioned generic BF4 DPU. Any environment that setsNICO_SYS_CLASS_NETin the provisioning context redirects PF resolution to an arbitrary directory, and the resolved netdev name is then used verbatim in laterovs-vsctloperations.The blast radius is small because DPF controls the execution environment. The test seam is still visible in production output and contributes to the flavor hash.
Two options: hard-code
/sys/class/netand have the test rewrite that single literal before executing, or keep the override and add a short comment stating that it exists only for host-side rendering tests.🤖 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/dpf/src/flavor.rs` around lines 246 - 264, Remove the NICO_SYS_CLASS_NET environment override from the rendered production script in the rawConfigScript construction, and use /sys/class/net directly for PF discovery. Update run_bf4_preflight or its test setup to rewrite that literal when a synthetic sysfs tree is needed, preserving production resolution against the real DPU sysfs path.
1680-1706: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up the synthetic sysfs fixture even when
bashfails.
fs::remove_dir_allruns at Line 1704. Line 1703 panics if thebashinvocation fails. The fixture directory then remains in the temp directory for every failing run. A guard type, ortempfile::TempDir, removes the directory on unwind.The static analysis hints on Lines 1697-1700 and 1727-1730 report shell command injection. Both scripts are built from test-controlled literals and from rendering functions in this file, so the finding does not apply here.
🤖 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/dpf/src/flavor.rs` around lines 1680 - 1706, Update run_bf4_preflight so the synthetic fixture is owned by an RAII cleanup guard, such as tempfile::TempDir, ensuring it is removed when Command::output panics or returns normally. Preserve the existing fixture layout, NICO_SYS_CLASS_NET environment setup, and returned Output; do not alter the shell construction, since the injection findings are out of scope.Source: Linters/SAST 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.
Nitpick comments:
In `@crates/dpf/src/flavor.rs`:
- Around line 246-264: Remove the NICO_SYS_CLASS_NET environment override from
the rendered production script in the rawConfigScript construction, and use
/sys/class/net directly for PF discovery. Update run_bf4_preflight or its test
setup to rewrite that literal when a synthetic sysfs tree is needed, preserving
production resolution against the real DPU sysfs path.
- Around line 1680-1706: Update run_bf4_preflight so the synthetic fixture is
owned by an RAII cleanup guard, such as tempfile::TempDir, ensuring it is
removed when Command::output panics or returns normally. Preserve the existing
fixture layout, NICO_SYS_CLASS_NET environment setup, and returned Output; do
not alter the shell construction, since the injection findings are out of scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f0aa2eb7-d490-486e-b012-714a19836bca
📒 Files selected for processing (17)
crates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/instance/mod.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/instance.rscrates/dpf/README.mdcrates/dpf/src/flavor.rscrates/dpf/src/lib.rscrates/dpf/src/sdk.rscrates/dpf/src/test/sdk_initialization.rscrates/dpf/src/types.rscrates/machine-controller/src/dpf.rscrates/rpc/src/errors.rscrates/rpc/src/model/instance/config/network.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- crates/rpc/src/errors.rs
- crates/dpf/README.md
- crates/api-core/src/handlers/instance.rs
- crates/dpf/src/lib.rs
- crates/api-core/src/test_support/default_config.rs
- crates/rpc/src/model/instance/config/network.rs
- crates/api-core/src/tests/instance.rs
- crates/machine-controller/src/dpf.rs
- crates/api-core/src/instance/mod.rs
- crates/dpf/src/test/sdk_initialization.rs
- crates/api-core/src/setup.rs
- crates/api-core/src/cfg/README.md
- crates/api-core/src/dpf_services.rs
- crates/api-core/src/cfg/file.rs
- crates/dpf/src/sdk.rs
- crates/dpf/src/types.rs
2 participants
DPF support lacks the intercept-bridging topology supported by legacy BlueField provisioning and rejects configurations that enable both features.
This PR...
ovn-encap-ip.Related issues
#1034
Type of Change
Breaking Changes
Testing
Additional Notes
Closes #1034
pf_total_sf_reservedis the completePF_TOTAL_SF, defaults to 30, and supports overrides.Deeper Dive
This PR branch makes representor interception a first-class DPF topology for BF3 and generic BF4.
The resulting data path is:
host PF/VF representor
│
▼
operator-configured intermediate OVS bridge
│
▼
DPF-owned Patch interface pair
│
▼
br-sfc
│
├── HBN
├── DHCP
└── FMDS (selected PF only)
Configuration and inventory behavior
Each host_representor_intercept_bridging entry can now include a typed dpf_interface identifying its controller_id, pf_id, and optional vf_id.
When DPF and intercept topology are configured:
The same effective inventory is then used for flavors, ServiceInterfaces, service chains, service configuration, DHCP ACLs, SF sizing, and instance admission. This is the main consistency promise of the change.
The full configuration contract is documented in crates/api-core/src/cfg/README.md:508.
How DPF primitives implement it
DPUFlavorDPUServiceInterface(Patch)br-sfc.DPUDeployment.spec.serviceChainsDPUServiceTemplateDPUServiceConfigurationDPUServiceNADDPUDeploymentFor example, a selected VF might be realized as:
BF4 c2pf3vf4
→ configured bridge br-vf4
→ DPF Patch pair
→ br-sfc
→ HBN interface pf0vf4_if
→ DHCP interface d_pf0vf4_if
The hardware identity remains visible in DPF resource names, but services see the selected PF as logical PF0. That preserves existing agent and service naming:
Every selected PF/VF receives HBN and DHCP. Only the selected PF receives FMDS. FMDS remains a single-PF, single-interface service.
Flavor and provisioning behavior
For BF3, raw representor names are derived directly as pfN....
For generic BF4, the flavor:
Representor attachment remains tolerant because a VF representor may not exist yet during provisioning, matching the earlier (pre-DPF) intercept creation behavior.
BF3 and generic BF4 flavors also configure Open_vSwitch.external_ids:ovn-encap-ip from exactly one global IPv4 address on oob_net0:
The flavor implementation is centered in crates/dpf/src/flavor.rs:139.
Capacity behavior
dpu_config.num_of_vfs now has an enforced maximum of 126 and is passed into BF3/generic-BF4 provisioning. Its default remains 16.
With topology configured:
Since P=1, 16 selected VFs require 37 managed endpoints.
dpf.pf_total_sf_reserved, defaulting to 30, is added as headroom:
PF_TOTAL_SF = managed endpoints + reserved
Thus one PF plus 16 VFs produces 37 + 30 = 67.
Additional guarantees:
Instance admission behavior
allow_instance_vf remains the global switch:
This exact membership check applies to both:
A sparse topology containing only VF7 therefore rejects a request for VF0 instead of accepting it and later attempting to configure a nonexistent pf0vf0_if.
The protobuf conversion path also now rejects wire VF IDs that cannot fit in u8, preventing malformed values such as 263 from truncating and aliasing VF7.
This admission logic is in crates/api-core/src/instance/mod.rs:143.
Deployment-scoped ServiceInterfaces
A new dpf.deployment_scoped_service_interfaces flag controls isolation.
When disabled—the default:
When enabled:
svc.dpu.nvidia.com/owned-by-dpudeployment=_<deployment_name>
Astra configuration requires scoping. Astra otherwise retains its own static inventory and fixed flavor behavior; it does not consume the intercept topology, num_of_vfs, or the configurable SF reserve.
Importantly, this makes Astra resources safely isolatable but does not implement production BF4+CX9 classification. The current production machine-controller still classifies every BF4 as generic BF4, so this branch does not promise end-to-end Astra deployment selection.
Compatibility promises
Pre-DPF intercept behavior remains unchanged:
DPF without intercept topology also intentionally preserves the existing static behavior:
That legacy path is deliberately not reconciled by this branch because doing so would alter existing ServiceInterfaces and immutable flavors.
Operational promises and limits
This is provisioning-time desired state, not live topology migration:
The new OVN bootstrap changes BF3/generic-BF4 flavor contents even without intercept topology. Existing DPUs receive that behavior through the normal immutable-flavor/reprovisioning lifecycle, not through an in-place dataplane mutation.
Finally, validation covers the known generated namespace, but it cannot prove that operator-selected bridge and patch names do not collide with every live Linux or OVS object on every DPU. Runtime namespace hygiene and hardware acceptance remain operator/qualification
responsibilities.