feat(bmc-mock): add libvirt-backed virtual media#3912
Conversation
Signed-off-by: Brennen Murray <brennenm@nvidia.com>
Summary by CodeRabbit
WalkthroughThe BMC mock gains typed boot-source overrides, Redfish virtual media support, libvirt and simulated callback backends, configurable host/DPU generation, and a multi-stage container image with corresponding usage documentation. ChangesBMC mock integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant RedfishClient
participant BmcRouter
participant VirtualMediaState
participant LibvirtCallbacks
participant virsh
RedfishClient->>BmcRouter: InsertMedia or EjectMedia
BmcRouter->>VirtualMediaState: validate and update device state
BmcRouter->>LibvirtCallbacks: persist media operation
LibvirtCallbacks->>virsh: attach or detach domain media
virsh-->>LibvirtCallbacks: command result
LibvirtCallbacks-->>BmcRouter: VirtualMediaResult
BmcRouter-->>RedfishClient: HTTP response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)crates/bmc-mock/DockerfileTraceback (most recent call last): 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/bmc-mock/src/redfish/computer_system.rs (1)
584-629: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRejected
BootOrderPATCH still mutates boot-source-override state and fires the callback.When
boot_order_modeisViaSettings, theBootOrderMode::ViaSettingsarm only stores a 400 response inresponse— it does notreturnearly like the equivalent branch inpatch_settingsdoes. Execution falls through to theapply_boot_source_override(...)/callbacks.set_boot_source_override(...)block regardless, mutatingsystem_state's internal override and invoking a real side-effecting callback (e.g. the libvirt backend rewriting the domain's boot XML) even though the client is ultimately told the request was rejected with 400.patch_settingsavoids this via an earlyreturnin the analogous branch;patch_systemshould do the same.🐛 Proposed fix: reject before mutating state
let boot = patch_system.get("Boot"); + if let Some(new_boot_order) = boot + .and_then(|obj| obj.get("BootOrder")) + .and_then(serde_json::Value::as_array) + .filter(|_| matches!(system_state.config.boot_order_mode, BootOrderMode::ViaSettings)) + { + let _ = new_boot_order; + return json!("Boot order setup must use Settings resource") + .into_response(StatusCode::BAD_REQUEST); + } let response = if let Some(new_boot_order) = boot .and_then(|obj| obj.get("BootOrder")) .and_then(serde_json::Value::as_array) .map(|arr| { arr.iter() .filter_map(serde_json::Value::as_str) .map(ToString::to_string) .collect() }) { match system_state.config.boot_order_mode { BootOrderMode::OrderedCollection => { ... } - BootOrderMode::ViaSettings => json!("Boot order setup must use Settings resource") - .into_response(StatusCode::BAD_REQUEST), + BootOrderMode::ViaSettings => unreachable!("handled above"), BootOrderMode::Generic => { ... } } } else { json!({}).into_ok_response() };(Alternatively, simply
returnthe 400 directly from theViaSettingsmatch arm instead of storing it inresponse, mirroringpatch_settings.)🤖 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/bmc-mock/src/redfish/computer_system.rs` around lines 584 - 629, Update patch_system so the BootOrderMode::ViaSettings branch returns the BAD_REQUEST response immediately instead of assigning it to response. This must prevent execution from reaching apply_boot_source_override and callbacks.set_boot_source_override after a rejected BootOrder PATCH; preserve the existing behavior of the OrderedCollection and Generic branches.
🧹 Nitpick comments (3)
crates/bmc-mock/src/libvirt.rs (1)
321-326: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
is_empty()here.!url.username().is_empty()reads more clearly than comparing against"".🤖 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/bmc-mock/src/libvirt.rs` around lines 321 - 326, Update the URL validation condition in the “http” | “https” branch to use url.username().is_empty() rather than comparing url.username() against an empty string, while preserving the existing credential and query validation behavior.Source: Coding guidelines
crates/bmc-mock/src/redfish/virtual_media.rs (1)
280-539: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing negative-path test coverage for
insert_media/eject_media.Tests cover the happy-path insert/eject cycle and system scoping, but the validation branches in
insert_media(emptyImage→ 400,Inserted: false→ 400) and the 404-on-unknown-device_idpath for both handlers are untested.🤖 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/bmc-mock/src/redfish/virtual_media.rs` around lines 280 - 539, Extend the test module around the existing request helper and virtual-media tests to cover negative paths for insert_media and eject_media. Add assertions that an empty Image and Inserted: false return BAD_REQUEST, and that both handlers return NOT_FOUND for an unknown device_id; use the existing test_router setup and action endpoints without changing production behavior.Source: Path instructions
crates/bmc-mock/src/redfish/computer_system.rs (1)
195-204: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoff
Othercatch-all discards the original Redfish target string on round-trip.
BootSourceOverrideTargetonly modelsCd,Hdd,None,Pxe,UefiHttp; every other legitimate Redfish value (e.g.Floppy,Usb,BiosSetup,Utilities,Diags,UefiShell,UefiTarget,SDCard,RemoteDrive,UefiBootNext) deserializes intoOthervia#[serde(other)]. Because#[serde(other)]only affectsDeserialize, re-serializingOtheringet_system/SystemBuilder::boot_source_overrideemits the literal string"Other"instead of the value the client originally PATCHed — a silent round-trip corruption for any target outside this small subset.🐛 Sketch of a fix preserving the original value
-#[derive(Clone, Copy, Debug, Deserialize, Eq, PartialEq, Serialize)] +#[derive(Clone, Debug, Eq, PartialEq)] pub enum BootSourceOverrideTarget { Cd, Hdd, None, Pxe, UefiHttp, - #[serde(other)] - Other, + Other(String), } + +// Manual Deserialize/Serialize impls that fall back to `Other(raw_string)` +// instead of discarding the original value, keeping `Copy` off the type +// (or wrapping `Other`'s payload in an `Arc<str>` to keep it cheap to clone).🤖 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/bmc-mock/src/redfish/computer_system.rs` around lines 195 - 204, Update BootSourceOverrideTarget’s serialization model so unknown Redfish target strings are retained rather than collapsed into the Other unit variant. Implement custom deserialization/serialization or an equivalent owned-string variant that preserves the original value while keeping known target mappings unchanged. Ensure get_system and SystemBuilder::boot_source_override re-emit the exact unknown target received from the client instead of "Other", and adjust affected derives/usages as needed.
🤖 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/bmc-mock/Dockerfile`:
- Around line 102-115: Create a dedicated unprivileged runtime user in the final
Docker image, grant it ownership or read access to /usr/local/bin/bmc-mock and
the TLS files under /opt/carbide, then switch to that user before ENTRYPOINT.
Document the required libvirt socket group mapping so the service can access the
mounted socket without root.
- Around line 109-111: Remove the tls.key COPY instruction from the Dockerfile
so the image no longer contains a private key. Keep certificate provisioning
deployment-specific through a secret, bind mount, or generated per-deployment
credential, while retaining only non-secret tls.crt handling if required.
In `@crates/bmc-mock/README.md`:
- Around line 111-115: Update the “Legacy generated mode” documentation to list
all conditions that preserve legacy behavior: omit role, backend, profile,
libvirt, dpu-count, and dpu-index flags, and use --instance-index 0. Clarify
that any other instance index, such as 1, enters explicit-mode validation
instead.
---
Outside diff comments:
In `@crates/bmc-mock/src/redfish/computer_system.rs`:
- Around line 584-629: Update patch_system so the BootOrderMode::ViaSettings
branch returns the BAD_REQUEST response immediately instead of assigning it to
response. This must prevent execution from reaching apply_boot_source_override
and callbacks.set_boot_source_override after a rejected BootOrder PATCH;
preserve the existing behavior of the OrderedCollection and Generic branches.
---
Nitpick comments:
In `@crates/bmc-mock/src/libvirt.rs`:
- Around line 321-326: Update the URL validation condition in the “http” |
“https” branch to use url.username().is_empty() rather than comparing
url.username() against an empty string, while preserving the existing credential
and query validation behavior.
In `@crates/bmc-mock/src/redfish/computer_system.rs`:
- Around line 195-204: Update BootSourceOverrideTarget’s serialization model so
unknown Redfish target strings are retained rather than collapsed into the Other
unit variant. Implement custom deserialization/serialization or an equivalent
owned-string variant that preserves the original value while keeping known
target mappings unchanged. Ensure get_system and
SystemBuilder::boot_source_override re-emit the exact unknown target received
from the client instead of "Other", and adjust affected derives/usages as
needed.
In `@crates/bmc-mock/src/redfish/virtual_media.rs`:
- Around line 280-539: Extend the test module around the existing request helper
and virtual-media tests to cover negative paths for insert_media and
eject_media. Add assertions that an empty Image and Inserted: false return
BAD_REQUEST, and that both handlers return NOT_FOUND for an unknown device_id;
use the existing test_router setup and action endpoints without changing
production behavior.
🪄 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: fec0edac-0313-40b6-bbef-f834097dc0c7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
crates/bmc-mock/Cargo.tomlcrates/bmc-mock/Dockerfilecrates/bmc-mock/README.mdcrates/bmc-mock/src/bmc_state.rscrates/bmc-mock/src/command_line.rscrates/bmc-mock/src/lib.rscrates/bmc-mock/src/libvirt.rscrates/bmc-mock/src/main.rscrates/bmc-mock/src/mock_machine_router.rscrates/bmc-mock/src/redfish/computer_system.rscrates/bmc-mock/src/redfish/mod.rscrates/bmc-mock/src/redfish/virtual_media.rscrates/bmc-mock/src/simulated.rscrates/bmc-mock/src/test_support/mod.rs
| RUN apt-get update && apt-get install -y --no-install-recommends \ | ||
| ca-certificates \ | ||
| libvirt-clients \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| RUN mkdir -p /opt/carbide | ||
|
|
||
| COPY --from=builder /artifacts/bmc-mock /usr/local/bin/bmc-mock | ||
| COPY crates/bmc-mock/tls.crt /opt/carbide/tls.crt | ||
| COPY crates/bmc-mock/tls.key /opt/carbide/tls.key | ||
|
|
||
| EXPOSE 1266 | ||
|
|
||
| ENTRYPOINT ["/usr/local/bin/bmc-mock"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Run the service as an unprivileged user.
The final image runs bmc-mock as root while exposing an HTTPS service and mounting a write-capable libvirt socket. Create a dedicated runtime user, assign ownership/read access to the binary and TLS material, add USER, and document any required socket-group mapping.
As per path instructions, Dockerfiles must use correct user and permission handling.
🤖 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/bmc-mock/Dockerfile` around lines 102 - 115, Create a dedicated
unprivileged runtime user in the final Docker image, grant it ownership or read
access to /usr/local/bin/bmc-mock and the TLS files under /opt/carbide, then
switch to that user before ENTRYPOINT. Document the required libvirt socket
group mapping so the service can access the mounted socket without root.
Sources: Path instructions, Linters/SAST tools
| COPY --from=builder /artifacts/bmc-mock /usr/local/bin/bmc-mock | ||
| COPY crates/bmc-mock/tls.crt /opt/carbide/tls.crt | ||
| COPY crates/bmc-mock/tls.key /opt/carbide/tls.key |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not bake a TLS private key into the image.
tls.key is copied into every published image, so any image recipient can extract and reuse the same private key. Supply certificate material at deployment time through a secret or bind mount, or generate unique development credentials per deployment.
As per path instructions, Dockerfiles must avoid embedded secrets.
🤖 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/bmc-mock/Dockerfile` around lines 109 - 111, Remove the tls.key COPY
instruction from the Dockerfile so the image no longer contains a private key.
Keep certificate provisioning deployment-specific through a secret, bind mount,
or generated per-deployment credential, while retaining only non-secret tls.crt
handling if required.
Source: Path instructions
| ## Legacy generated mode | ||
|
|
||
| Invoking `bmc-mock` without any role, backend, profile, or libvirt flags retains | ||
| the original pre-libvirt behavior: a generated Wiwynn GB200 host using the | ||
| existing channel callbacks. Archive routers and library callers are unchanged. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document every flag that disables legacy mode.
Legacy mode also requires no --dpu-count or --dpu-index and --instance-index 0. For example, bmc-mock --instance-index 1 enters explicit-mode validation and fails, rather than retaining legacy behavior. List these constraints here.
Based on supplied crates/bmc-mock/src/main.rs:231-332, legacy mode has additional flag constraints.
🤖 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/bmc-mock/README.md` around lines 111 - 115, Update the “Legacy
generated mode” documentation to list all conditions that preserve legacy
behavior: omit role, backend, profile, libvirt, dpu-count, and dpu-index flags,
and use --instance-index 0. Clarify that any other instance index, such as 1,
enters explicit-mode validation instead.
Source: Path instructions
|
|
||
| fn state_refresh_indication(&self); | ||
|
|
||
| fn set_boot_source_override( |
There was a problem hiding this comment.
We don't add more callbacks for each individual actions to the callbacks. state_refresh_indication is called after each modification request and you should check bmc-mock state on it to identify what has changed.
Otherwise it will be infinite number of callbacks. The same is for virtual media callbacks below.
| #[derive(Clone, Copy, Debug, Default, Deserialize, Eq, PartialEq, Serialize)] | ||
| pub struct BootSourceOverride { | ||
| mode: Option<String>, | ||
| enabled: Option<String>, | ||
| target: Option<String>, | ||
| #[serde(rename = "BootSourceOverrideMode")] | ||
| pub mode: Option<BootSourceOverrideMode>, | ||
| #[serde(rename = "BootSourceOverrideEnabled")] | ||
| pub enabled: Option<BootSourceOverrideEnabled>, | ||
| #[serde(rename = "BootSourceOverrideTarget")] | ||
| pub target: Option<BootSourceOverrideTarget>, | ||
| } | ||
|
|
||
| #[derive(Clone, Copy, Debug, Deserialize, Eq, PartialEq, Serialize)] | ||
| pub enum BootSourceOverrideMode { | ||
| #[serde(rename = "UEFI")] | ||
| Uefi, | ||
| Legacy, | ||
| #[serde(other)] | ||
| Other, | ||
| } | ||
|
|
||
| #[derive(Clone, Copy, Debug, Deserialize, Eq, PartialEq, Serialize)] | ||
| pub enum BootSourceOverrideEnabled { | ||
| Once, | ||
| Continuous, | ||
| Disabled, | ||
| #[serde(other)] | ||
| Other, | ||
| } | ||
|
|
||
| #[derive(Clone, Copy, Debug, Deserialize, Eq, PartialEq, Serialize)] | ||
| pub enum BootSourceOverrideTarget { | ||
| Cd, | ||
| Hdd, | ||
| None, | ||
| Pxe, | ||
| UefiHttp, | ||
| #[serde(other)] | ||
| Other, |
There was a problem hiding this comment.
We don't introduce typed model in bmc-mock. It is intentionally as flexible as possible. So, please don't introduce it here as well, it will not follow common pattern.
| image: String, | ||
| inserted: Option<bool>, | ||
| write_protected: Option<bool>, | ||
| } |
There was a problem hiding this comment.
The same comment as above, please, don't introduce model here. Just use real json field names to extract needed data.
| #[derive(Default, Debug, Clone, Copy, Serialize, Deserialize, Eq, PartialEq, ValueEnum)] | ||
| pub enum HostHardwareType { | ||
| #[serde(rename = "dell_poweredge_r750")] | ||
| #[value(name = "dell-poweredge-r750")] |
There was a problem hiding this comment.
I don't think that we need another string representation of it. It will be confusing. I understand that it may look better to have kebab-style option in command line interface but lets avoid multiple ways to name thing.
| pub update_service_state: Arc<UpdateServiceState>, | ||
| pub account_service_state: Arc<AccountServiceState>, | ||
| pub session_service_state: Arc<SessionServiceState>, | ||
| pub virtual_media_state: Option<Arc<VirtualMediaState>>, |
There was a problem hiding this comment.
It should be part of computer system state, not full bmc mock state. In Redfish model Manager can manage more than one ComputerSystem, each can has its own Virtual media.
2 participants
Adds a consumable libvirt-backed mode to
bmc-mockfor VM-based development and test environments. This allows Redfish clients to exercise host power, boot override, and virtual-media workflows against a simulated BMC that controls a configured libvirt domain.Previously, the standalone generated mock provided Redfish resources but did not provide a complete per-VM backend for these workflows.
This change:
Related issues
None.
Type of Change
Breaking Changes
The existing standalone invocation and archive-backed behavior remain supported.
Testing
Validated with:
cargo test -p bmc-mock --lockedcargo fmt --all -- --checkcargo clippy -p bmc-mock --all-targets --all-features -- -D warningsAdditional Notes