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
1 participant
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