Support for creating DPF flavor, deployment and services for BF4 Astra#4127
Support for creating DPF flavor, deployment and services for BF4 Astra#4127aadvani-nvidia wants to merge 3 commits into
Conversation
Signed-off-by: aadvani <aadvani@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughAdds BF4 Astra as a deployment type with dedicated configuration, DOCA Weave/Xplane services, Astra-specific DPU flavor generation, and SDK initialization and deployment wiring. ChangesBF4 Astra deployment support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DpfConfig
participant DpfServices
participant DpfSdk
participant DpfFlavor
DpfConfig->>DpfConfig: Resolve BF4 Astra configuration
DpfConfig->>DpfServices: Provide base and extra services
DpfServices->>DpfSdk: Build mandatory service definitions
DpfSdk->>DpfFlavor: Select Astra flavor
DpfSdk->>DpfSdk: Enable astra_enabled and create deployment
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
crates/api-core/src/cfg/file.rs (2)
1409-1421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer an exhaustive match over the
_catch-all for deployment types.
DpuDeploymentTypeis a small closed enum; the wildcard arm means the next variant added silently resolves to zero extra services instead of failing to compile. ListingBf3andBf4Genericexplicitly makes the omission deliberate.♻️ Suggested change
let extra = match deployment_type { DpuDeploymentType::Bf4Astra => HashMap::from([ ( DOCA_WEAVE_SERVICE_NAME.to_string(), crate::dpf_services::default_doca_weave_service(), ), ( DOCA_XPLANE_SERVICE_NAME.to_string(), crate::dpf_services::default_doca_xplane_service(), ), ]), - _ => HashMap::new(), + DpuDeploymentType::Bf3 | DpuDeploymentType::Bf4Generic => HashMap::new(), };As per path instructions, Rust changes should favour "designs that are hard to misuse".
🤖 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/cfg/file.rs` around lines 1409 - 1421, Update the match constructing extra services in the deployment configuration flow to replace the `_` catch-all with explicit `DpuDeploymentType::Bf3` and `DpuDeploymentType::Bf4Generic` arms, both returning `HashMap::new()`, while preserving the existing `Bf4Astra` services.Source: Path instructions
5926-5926: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe new
bf4_astrabranches are untested.Every updated literal simply sets
bf4_astra: None, so neither the newvalidate_provisioning_sourcesbranch forbf4_astranorresolved_services_for(_, Bf4Astra)returning the Weave/Xplane extras is exercised. Two additional rows in the existing tables would close the gap: onebf4_astrawithbfb_urland nobluefield_software(expected error), and one asserting the resolvedextrakeys forBf4Astra.As per coding guidelines: "Use table-driven tests for functions mapping inputs to outputs or errors."
🤖 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/cfg/file.rs` at line 5926, Extend the existing table-driven tests for validate_provisioning_sources with a bf4_astra case containing bfb_url but no bluefield_software, expecting the validation error. Add a separate resolved_services_for(_, Bf4Astra) table row asserting the returned Weave/Xplane extra keys, so both new branches are exercised.Source: Coding guidelines
crates/dpf/src/sdk.rs (2)
781-791: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
suffixanddeployment_typeare now redundant parameters that can disagree.At the only production call site the suffix is
deployment_cr_suffix(deployment_type)(line 1234), sobuild_deploymentreceives the same information twice and nothing prevents a caller from pairingBf4Astrawith the BF3 suffix — which would produce a deployment whose service CR references point at another deployment's resources. Deriving the suffix internally removes the hazard and shortens an already nine-parameter signature.♻️ Suggested refactor
deployment_node_labels: BTreeMap<String, String>, - suffix: &str, deployment_type: DpuDeploymentType, ) -> DPUDeployment { + let suffix = deployment_cr_suffix(deployment_type);As per path instructions, prefer "designs that are hard to misuse".
🤖 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 781 - 791, Update build_deployment to remove the redundant suffix parameter and derive the suffix internally from deployment_type using deployment_cr_suffix(deployment_type). Adjust the existing production call site and any affected callers to pass only deployment_type, preserving the current generated deployment and service CR references.Source: Path instructions
2196-2206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth call sites were mechanically updated, but the new behaviour is untested.
The two existing tests only satisfy the new signature. Nothing asserts
deployment_cr_suffix(DpuDeploymentType::Bf4Astra) == "bf4astra", nor thatspec.dpus.astra_enabledisSome(true)forBf4AstraandNonefor the other two variants — the latter is the entire point of the change and is a field the DPF operator acts on. A three-rowvalue_scenarios!table overDpuDeploymentTypewould cover both.As per coding guidelines: "Use table-driven tests for functions mapping inputs to outputs or errors."
Also applies to: 2258-2268
🤖 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 2196 - 2206, The existing deployment tests only compile against the updated signature and do not verify the new Bf4Astra behavior. Extend the tests around build_deployment and deployment_cr_suffix with a three-row value_scenarios! table for Bf3, Bf4, and Bf4Astra, asserting suffix values and that spec.dpus.astra_enabled is Some(true) only for Bf4Astra and None for the other variants.Source: Coding guidelines
crates/api-core/src/setup.rs (1)
646-664: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe BF4 Astra block is a copy of the BF4 generic block above it.
Lines 646-664 differ from 623-643 only in the deployment field, the
DpuDeploymentType, and the deployment name embedded in the two error strings. A small loop keeps the two BF4 paths from drifting and makes the third one free.♻️ Suggested refactor
- if let Some(bf4) = &carbide_config.dpf.deployments.bf4_generic { - // Validation guarantees `bluefield_software` is set with exactly one PSID - // entry for a BF4 deployment. - let bfs = bf4.bluefield_software.as_ref().ok_or_else(|| { - eyre::eyre!("bf4_generic DPF deployment is missing bluefield_software") - })?; - ... - } + // Validation guarantees `bluefield_software` is set with exactly one PSID + // entry for every BF4 deployment. + let bf4_deployments = [ + ( + "bf4_generic", + carbide_config.dpf.deployments.bf4_generic.as_ref(), + DpuDeploymentType::Bf4Generic, + ), + ( + "bf4_astra", + carbide_config.dpf.deployments.bf4_astra.as_ref(), + DpuDeploymentType::Bf4Astra, + ), + ]; + for (name, deployment, deployment_type) in bf4_deployments { + let Some(deployment) = deployment else { + continue; + }; + let bfs = deployment.bluefield_software.as_ref().ok_or_else(|| { + eyre::eyre!("{name} DPF deployment is missing bluefield_software") + })?; + let pldm_url = bfs + .pldm_fw_bundle + .values() + .next() + .ok_or_else(|| eyre::eyre!("{name} DPF deployment has an empty pldm_fw_bundle"))?; + let params = carbide_dpf::BlueFieldSoftwareParams { + os_iso: bfs.os_iso.clone(), + pldm_fw_bundle: Some(pldm_url.clone()), + }; + sdk.create_initialization_objects(&make_init_config( + deployment, + deployment_type, + Some(params), + )) + .await + .map_err(|err| eyre::eyre!("failed to initialize {name} DPF deployment: {err}"))?; + }🤖 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/setup.rs` around lines 646 - 664, Refactor the duplicated BF4 deployment initialization logic around sdk.create_initialization_objects into a small loop or shared helper that handles both generic BF4 and bf4_astra deployments. Parameterize the deployment reference, DpuDeploymentType, and error-message deployment name, while preserving the existing bluefield_software validation and PLDM bundle parameter construction; structure it so adding another BF4 variant requires only another entry.crates/dpf/src/flavor.rs (2)
1495-1692: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid coverage; two assertions are load-bearing on incidental formatting.
The invariant table is thorough. However, the row at lines 1590-1599 pins the exact blank-line placement and two-space helper indentation of the OVS script, and 1601-1618 pins YAML indentation — a purely cosmetic reflow of those literals will fail these tests without any behavioural change. Asserting on the parsed structure (or on the presence of the script invocations alone) would be more durable.
Separately,
!names.contains(&"USER_PROGAMMABLE_CC")at line 1658 asserts the absence of a misspelling that no longer appears in the source; it can only ever pass.🤖 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 1495 - 1692, Make the assertions in bf4_astra_flavor_spec_invariants resilient to formatting-only changes: replace exact OVS whitespace checks with checks for the required script invocations or parsed behavior, and validate the Spectrum-X YAML through its parsed structure rather than indentation. Remove the always-passing USER_PROGAMMABLE_CC absence check while retaining the positive USER_PROGRAMMABLE_CC validation.
632-1143: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider moving these ~500 lines of embedded YAML and shell into asset files.
get_bf4_astra_config_filesis dominated byconcat!string literals where every line carries"and\nnoise.include_str!("assets/bf4_astra/spectrum-x-RA2.2-runtime.yaml")and friends would keep the payloads editable (and diffable, and syntax-highlighted) while leaving this function as a thin assembler ofDpuFlavorConfigFiles. The escaping in the shell scripts in particular is where a silent mistake is most likely to hide.As per path instructions, Rust changes should "prefer simple explicit code".
🤖 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 632 - 1143, Refactor get_bf4_astra_config_files to move the large embedded YAML and shell payloads out of concat! string literals into appropriately named asset files. Load each asset with include_str! and keep the function focused on assembling DpuFlavorConfigFiles, preserving the existing file paths, permissions, and exact payload contents while avoiding unnecessary abstraction.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 `@crates/api-core/src/cfg/file.rs`:
- Around line 1488-1493: Replace the String-keyed HashMap in
DpfResolvedMandatoryServicesConfig (crates/api-core/src/cfg/file.rs:1488-1493)
with an enum-keyed BTreeMap, or explicit optional Weave/Xplane fields, and
promote its leading comments to doc comments; update the consumer in
crates/api-core/src/dpf_services.rs:548-556 to use an exhaustive enum match
without a silent-drop arm, preserving deterministic service ordering and
compile-time coverage of new extras.
In `@crates/api-core/src/dpf_services.rs`:
- Around line 548-556: Update the service construction flow around the loop over
resolved.extra to reject or explicitly report unknown keys instead of silently
ignoring the wildcard match, and make the resulting service_vec deterministic by
sorting extra entries or using a defined service order before appending them.
Preserve the existing handlers for DOCA_WEAVE_SERVICE_NAME and
DOCA_XPLANE_SERVICE_NAME.
- Around line 512-532: Update doca_weave_service and doca_xplane_service to
project cfg.docker_repo_url, cfg.docker_image_tag, and
cfg.docker_image_pull_secret into the returned ServiceDefinition helm_values,
matching the established pattern used by other mandatory services and preserving
the values populated by default_doca_*_service.
In `@crates/api-core/src/setup.rs`:
- Line 665: Update the error message in the bf4_astra DPF deployment
initialization map_err to start with lowercase “failed” instead of “Failed”,
while preserving the existing message content and formatting.
- Line 553: Update the startup log in initialize_dpf_sdk to use a
deployment-independent stable message instead of claiming BF4 Astra; if
deployment configuration should remain visible, add it as a structured tracing
field rather than interpolating it into the message.
In `@crates/dpf/src/flavor.rs`:
- Around line 1052-1072: Update the node-selection logic surrounding the NODES
array and its exit paths so an unlisted DPU emits a clear diagnostic containing
the requested serial and addressing context before exiting. Preserve successful
netplan application for listed nodes, and avoid leaving the failure silent; if
the existing deployment configuration provides node data, prefer sourcing the
mapping from it rather than requiring another compiled serial-number entry.
- Around line 1145-1178: The containerd proxy drop-in generation is duplicated
across flavor config-file builders. Extract the block into a shared
containerd_proxy_config_file function accepting &DpfProxyDetails and returning
Result<DpuFlavorConfigFiles, DpfError>; move validation, trimming, filtering,
sorting, deduplication, and Environment= formatting there, then have each
builder reuse the helper.
---
Nitpick comments:
In `@crates/api-core/src/cfg/file.rs`:
- Around line 1409-1421: Update the match constructing extra services in the
deployment configuration flow to replace the `_` catch-all with explicit
`DpuDeploymentType::Bf3` and `DpuDeploymentType::Bf4Generic` arms, both
returning `HashMap::new()`, while preserving the existing `Bf4Astra` services.
- Line 5926: Extend the existing table-driven tests for
validate_provisioning_sources with a bf4_astra case containing bfb_url but no
bluefield_software, expecting the validation error. Add a separate
resolved_services_for(_, Bf4Astra) table row asserting the returned Weave/Xplane
extra keys, so both new branches are exercised.
In `@crates/api-core/src/setup.rs`:
- Around line 646-664: Refactor the duplicated BF4 deployment initialization
logic around sdk.create_initialization_objects into a small loop or shared
helper that handles both generic BF4 and bf4_astra deployments. Parameterize the
deployment reference, DpuDeploymentType, and error-message deployment name,
while preserving the existing bluefield_software validation and PLDM bundle
parameter construction; structure it so adding another BF4 variant requires only
another entry.
In `@crates/dpf/src/flavor.rs`:
- Around line 1495-1692: Make the assertions in bf4_astra_flavor_spec_invariants
resilient to formatting-only changes: replace exact OVS whitespace checks with
checks for the required script invocations or parsed behavior, and validate the
Spectrum-X YAML through its parsed structure rather than indentation. Remove the
always-passing USER_PROGAMMABLE_CC absence check while retaining the positive
USER_PROGRAMMABLE_CC validation.
- Around line 632-1143: Refactor get_bf4_astra_config_files to move the large
embedded YAML and shell payloads out of concat! string literals into
appropriately named asset files. Load each asset with include_str! and keep the
function focused on assembling DpuFlavorConfigFiles, preserving the existing
file paths, permissions, and exact payload contents while avoiding unnecessary
abstraction.
In `@crates/dpf/src/sdk.rs`:
- Around line 781-791: Update build_deployment to remove the redundant suffix
parameter and derive the suffix internally from deployment_type using
deployment_cr_suffix(deployment_type). Adjust the existing production call site
and any affected callers to pass only deployment_type, preserving the current
generated deployment and service CR references.
- Around line 2196-2206: The existing deployment tests only compile against the
updated signature and do not verify the new Bf4Astra behavior. Extend the tests
around build_deployment and deployment_cr_suffix with a three-row
value_scenarios! table for Bf3, Bf4, and Bf4Astra, asserting suffix values and
that spec.dpus.astra_enabled is Some(true) only for Bf4Astra and None for the
other variants.
🪄 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: 83df5701-e5db-48de-9d21-938b185e63de
📒 Files selected for processing (6)
crates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/setup.rscrates/dpf/src/flavor.rscrates/dpf/src/sdk.rscrates/dpf/src/types.rs
Signed-off-by: aadvani <aadvani@nvidia.com>
Signed-off-by: aadvani <aadvani@nvidia.com>
4874782 to
7374649
Compare
1 participant
This PR adds support to create DPUFlavor and other yamls for BF4 Astra
Related issues
#3205
Type of Change
Breaking Changes
Testing
Addition
Sample generated dpu flavor
dpuflavor.yaml