Skip to content

fix: (dpf/bf4) - Changes to sync BF4 flavor as used by DPF team#3772

Merged
bcavnvidia merged 1 commit into
NVIDIA:mainfrom
abvarshney-nv:bf4_fixes
Jul 21, 2026
Merged

fix: (dpf/bf4) - Changes to sync BF4 flavor as used by DPF team#3772
bcavnvidia merged 1 commit into
NVIDIA:mainfrom
abvarshney-nv:bf4_fixes

Conversation

@abvarshney-nv

Copy link
Copy Markdown
Contributor

Changes to sync BF4 flavor as used by DPF team

Related issues

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Additional Notes

@abvarshney-nv
abvarshney-nv requested a review from a team as a code owner July 21, 2026 12:19
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Summary by CodeRabbit

  • New Features

    • BF4 networking setup now automatically creates and configures the br-hbn bridge.
    • The required MST service is started during BF4 network initialization.
  • Bug Fixes

    • BF4 systems now use hardware-specific default configuration settings, improving compatibility and ensuring appropriate port and link behavior.

Walkthrough

BF4 flavor generation now adds br-hbn and starts mst in the OVS setup script. The BF4 flavor also uses a dedicated NVconfig parameter set instead of the shared default.

Changes

BF4 configuration

Layer / File(s) Summary
BF4 network setup
crates/dpf/src/flavor.rs
The generated BF4 OVS script creates and configures br-hbn and starts mst.
BF4 NVconfig selection
crates/dpf/src/flavor.rs
flavor_bf4 uses a BF4-specific NVconfig constructor with updated parameter inclusion and values.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is specific and accurately reflects the BF4 flavor sync changes in the diff.
Description check ✅ Passed The description is directly related to the BF4 flavor alignment changes in this pull request.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/dpf/src/flavor.rs (1)

179-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add regression coverage for the BF4 flavor contract.

The selector and helper jointly change the generated NVconfig. Add a focused test asserting the device and exact BF4 parameters, including PF_SF_BAR_SIZE=14, PF_TOTAL_SF=30, and both LINK_TYPE_* = ETH, so a future refactor cannot silently fall back to the shared defaults.

As per path instructions, prioritize missing behavior coverage for this change. As per coding guidelines, use table-driven cases when covering multiple flavor outputs.

Also applies to: 358-381

🤖 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` at line 179, In the flavor tests covering the
selector and get_bf4_default_nvconfig, add table-driven regression coverage for
the BF4 output. Assert the expected device and exact NVconfig values, including
PF_SF_BAR_SIZE=14, PF_TOTAL_SF=30, and both LINK_TYPE_* values set to ETH, while
preserving coverage for the other flavor outputs.

Sources: Coding guidelines, 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.

Nitpick comments:
In `@crates/dpf/src/flavor.rs`:
- Line 179: In the flavor tests covering the selector and
get_bf4_default_nvconfig, add table-driven regression coverage for the BF4
output. Assert the expected device and exact NVconfig values, including
PF_SF_BAR_SIZE=14, PF_TOTAL_SF=30, and both LINK_TYPE_* values set to ETH, while
preserving coverage for the other flavor outputs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: db16eaf2-874b-4c4c-99e4-e16dd9aee194

📥 Commits

Reviewing files that changed from the base of the PR and between b20d9f4 and 4d867e7.

📒 Files selected for processing (1)
  • crates/dpf/src/flavor.rs

@github-actions

Copy link
Copy Markdown

🔍 Container Scan Summary

Service Total Critical High Medium Low Other
boot-artifacts-aarch64 3 0 0 3 0 0
boot-artifacts-x86_64 3 0 0 3 0 0
forge-admin-cli-x86_64 259 16 29 79 7 128
machine-validation-runner 538 41 117 184 15 181
machine_validation 538 41 117 184 15 181
machine_validation-aarch64 538 41 117 184 15 181
TOTAL 1879 139 380 637 52 671

Per-CVE detail lives in the per-service grype-* artifacts (JSON + SARIF). Severity counts only — no CVE IDs published here.

@github-actions

Copy link
Copy Markdown

@bcavnvidia
bcavnvidia enabled auto-merge (squash) July 21, 2026 15:19
@bcavnvidia
bcavnvidia merged commit 4bf329d into NVIDIA:main Jul 21, 2026
61 checks passed
@abvarshney-nv
abvarshney-nv deleted the bf4_fixes branch July 21, 2026 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants