Skip to content

refactor(bmc-mock): avoid cloning machine info for BMC mock setup#2779

Merged
poroh merged 1 commit into
NVIDIA:mainfrom
poroh:refactor-bmc-mock-machine-info-clones
Jun 23, 2026
Merged

refactor(bmc-mock): avoid cloning machine info for BMC mock setup#2779
poroh merged 1 commit into
NVIDIA:mainfrom
poroh:refactor-bmc-mock-machine-info-clones

Conversation

@poroh

@poroh poroh commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Change BMC mock router construction to borrow MachineInfo and store only the derived SSH prompt behavior in BmcMockWrapper.

Related issues

This is preliminary refactor before
#2617

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

@poroh poroh requested a review from a team as a code owner June 23, 2026 00:35
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b0927b53-5008-4f30-b03f-095cc4b556f3

📥 Commits

Reviewing files that changed from the base of the PR and between 8a16333 and 144cca1.

📒 Files selected for processing (7)
  • crates/bmc-mock/src/machine_info.rs
  • crates/bmc-mock/src/main.rs
  • crates/bmc-mock/src/mock_machine_router.rs
  • crates/bmc-mock/src/redfish/expander_router.rs
  • crates/bmc-mock/src/test_support/mod.rs
  • crates/machine-a-tron/src/bmc_mock_wrapper.rs
  • crates/machine-a-tron/src/machine_state_machine.rs
🚧 Files skipped from review as they are similar to previous changes (6)
  • crates/machine-a-tron/src/machine_state_machine.rs
  • crates/bmc-mock/src/redfish/expander_router.rs
  • crates/bmc-mock/src/main.rs
  • crates/machine-a-tron/src/bmc_mock_wrapper.rs
  • crates/bmc-mock/src/mock_machine_router.rs
  • crates/bmc-mock/src/test_support/mod.rs

Summary by CodeRabbit

  • Refactor
    • Internal code optimization to improve memory efficiency. No user-facing functionality or feature changes.

Walkthrough

The PR removes Serialize/Deserialize derives from HostMachineInfo and DpuMachineInfo, changes machine_router to accept &MachineInfo instead of owned values, updates all callers accordingly, and refactors BmcMockWrapper to accept borrowed references, precompute ssh_prompt_behavior at construction, and eliminate unnecessary cloning in run_bmc_mock.

Changes

MachineInfo Borrow Refactor and Serialization Removal

Layer / File(s) Summary
Remove Serde derives from MachineInfo structs
crates/bmc-mock/src/machine_info.rs
HostMachineInfo and DpuMachineInfo no longer derive Serialize/Deserialize; Serde field attributes (#[serde(default)], #[serde(flatten)]) are removed.
machine_router signature: owned → &MachineInfo
crates/bmc-mock/src/mock_machine_router.rs
machine_info parameter changes from MachineInfo to &MachineInfo; the internal match is adjusted from &machine_info to machine_info.
Call site updates across bmc-mock
crates/bmc-mock/src/main.rs, crates/bmc-mock/src/redfish/expander_router.rs, crates/bmc-mock/src/test_support/mod.rs
All callers of machine_router — including default_host_mock, test_host_mock, and ten test helper constructors — add & before their MachineInfo::... arguments.
BmcMockWrapper refactor: borrow and precompute
crates/machine-a-tron/src/bmc_mock_wrapper.rs, crates/machine-a-tron/src/machine_state_machine.rs
BmcMockWrapper gains a stored ssh_prompt_behavior: PromptBehavior field; new accepts &MachineInfo and precomputes the prompt behavior during construction; start passes the stored value to mock_ssh_server::spawn; run_bmc_mock passes &self.machine_info instead of cloning.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 17.65% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and accurately captures the main refactoring objective: avoiding cloning of MachineInfo during BMC mock setup.
Description check ✅ Passed The description is clearly related to the changeset, explaining the refactoring approach of borrowing MachineInfo and storing derived SSH prompt behavior instead of cloning.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Change BMC mock router construction to borrow `MachineInfo` and store
only the derived SSH prompt behavior in `BmcMockWrapper`.

Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
@poroh poroh force-pushed the refactor-bmc-mock-machine-info-clones branch from 8a16333 to 144cca1 Compare June 23, 2026 00:40
@poroh poroh enabled auto-merge (squash) June 23, 2026 00:55
@github-actions

github-actions Bot commented Jun 23, 2026

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 264 6 23 99 6 130
machine-validation-runner 714 34 183 266 35 196
machine_validation 714 34 183 266 35 196
nvmetal-carbide 714 34 183 266 35 196
TOTAL 2412 108 572 903 111 718

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

@poroh poroh merged commit 2effb2a into NVIDIA:main Jun 23, 2026
55 checks passed
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