feat(bmc-mock): add optional IPMI and SOL simulation - #3542
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (9)
Summary by CodeRabbit
WalkthroughAdds optional IPMI/SOL simulation to BMC mocks, synchronizes Redfish administrator passwords with ChangesIPMI simulation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MachineATron
participant BmcMockWrapper
participant ipmi_sim
participant BmcState
participant Redfish
MachineATron->>BmcMockWrapper: start IPMI simulation
BmcMockWrapper->>ipmi_sim: start(IpmiSimConfig)
ipmi_sim->>BmcState: configure password updater and endpoint
Redfish->>BmcState: patch administrator password
BmcState->>ipmi_sim: invoke ipmitool password update
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
f436a0d to
ea41c8b
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: 3
🧹 Nitpick comments (1)
crates/bmc-mock/src/main.rs (1)
97-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
.expect()safety is an implicit, unenforced cross-branch invariant.The
generated_state.as_ref().expect(...)at Line 111-113 is only safe because the guard at Line 74-78 preventsenable_ipmi_simulationfrom coexisting withtargz. That's correct today, but it's a runtime invariant spanning two independent branches with no compiler enforcement — a future refactor of either check could silently turn this into a panic instead of a clean error return.Consider returning a
Result/graceful error instead of.expect()here, so a broken invariant degrades to an error message rather than a panic.♻️ Proposed fix to fail gracefully instead of panicking
let _ipmi_sim_handle = if args.enable_ipmi_simulation { - let state = generated_state - .as_ref() - .expect("archive-backed routers were rejected above"); + let Some(state) = generated_state.as_ref() else { + return Err("--enable-ipmi-simulation requires the default BMC mock state".into()); + }; Some( bmc_mock::ipmi_sim::start( state,🤖 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/main.rs` around lines 97 - 127, Replace the expect-based unwrap in the IPMI simulation setup with graceful error propagation when generated_state is absent. Update the branch using generated_state.as_ref() so it returns a descriptive error through main’s existing Result flow, while preserving normal startup for default_host_mock state and the disabled-simulation path.
🤖 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/src/ipmi_sim.rs`:
- Around line 422-435: Update serve_console to remove the unbounded input Vec
and detect newline termination directly from each read slice, while preserving
the existing echo and prompt behavior. Use the current buffer segment and length
for the newline/carriage-return check, without accumulating data across reads.
- Around line 65-69: The simulator currently requests child termination without
awaiting cleanup, and console session tasks are detached. In
crates/bmc-mock/src/ipmi_sim.rs lines 65-69, replace Drop-only process cleanup
with an explicit async shutdown path that terminates and awaits/reaps the child;
in lines 405-412, track spawned console sessions in a JoinSet and have shutdown
cancel and join all active tasks.
In `@crates/bmc-mock/src/redfish/account_service.rs`:
- Around line 126-145: Restrict the password updater invocation in
update_password to accounts whose typed role is Administrator, leaving
non-administrator Redfish password updates unaffected. Use the account role enum
rather than comparing a string, and add a test confirming non-administrator
updates do not invoke synchronization or alter IPMI credentials.
---
Nitpick comments:
In `@crates/bmc-mock/src/main.rs`:
- Around line 97-127: Replace the expect-based unwrap in the IPMI simulation
setup with graceful error propagation when generated_state is absent. Update the
branch using generated_state.as_ref() so it returns a descriptive error through
main’s existing Result flow, while preserving normal startup for
default_host_mock state and the disabled-simulation path.
🪄 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: 42bdfe9f-48d8-476d-ad55-ae73e428d961
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
crates/api-integration-tests/tests/lib.rscrates/bmc-mock/Cargo.tomlcrates/bmc-mock/src/command_line.rscrates/bmc-mock/src/ipmi_sim.rscrates/bmc-mock/src/lib.rscrates/bmc-mock/src/machine_info.rscrates/bmc-mock/src/main.rscrates/bmc-mock/src/redfish/account_service.rscrates/bmc-mock/src/redfish/manager.rscrates/bmc-mock/src/redfish/manager_network_protocol.rscrates/machine-a-tron/config/mac.tomlcrates/machine-a-tron/config/mat.tomlcrates/machine-a-tron/src/bmc_mock_wrapper.rscrates/machine-a-tron/src/config.rscrates/machine-a-tron/src/machine_state_machine.rscrates/ssh-console/tests/util/ipmi_sim.rscrates/ssh-console/tests/util/mod.rs
| impl Drop for IpmiSimHandle { | ||
| fn drop(&mut self) { | ||
| self.child.start_kill().ok(); | ||
| self.manager.set_ipmi_endpoint(None); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/bmc-mock/src/ipmi_sim.rs"
echo "== outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== targeted lines around Drop and console accept loop =="
sed -n '1,130p' "$FILE" | cat -n
echo
sed -n '360,470p' "$FILE" | cat -n
echo
echo "== searches for shutdown/join/task ownership =="
rg -n "JoinSet|start_kill|wait\(|kill\(|set_ipmi_endpoint|CancellationToken|spawn\(|accept\(\)" "$FILE"Repository: NVIDIA/infra-controller
Length of output: 13058
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/bmc-mock/src/ipmi_sim.rs"
python3 - <<'PY'
from pathlib import Path
p = Path("crates/bmc-mock/src/ipmi_sim.rs")
text = p.read_text()
for needle in ["struct IpmiSimHandle", "impl Drop for IpmiSimHandle", "tokio::spawn(async move", "JoinSet", "kill_on_drop", "wait().await", "start_kill"]:
idx = text.find(needle)
print(f"\n### {needle} ###")
if idx == -1:
print("not found")
continue
start = max(0, text.rfind("\n", 0, idx-300))
end = min(len(text), text.find("\n", idx+500))
if end == -1:
end = min(len(text), idx+1000)
print(text[start:end])
PYRepository: NVIDIA/infra-controller
Length of output: 4585
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/bmc-mock/src/ipmi_sim.rs"
echo "== start() cleanup branch around child shutdown =="
sed -n '150,210p' "$FILE" | cat -n
echo
echo "== MockConsole tests and shutdown behavior =="
sed -n '470,560p' "$FILE" | cat -nRepository: NVIDIA/infra-controller
Length of output: 3645
🌐 Web query:
tokio::process::Child kill_on_drop drop reaped wait semantics
💡 Result:
In the tokio::process module, the kill_on_drop(true) configuration and the drop semantics of a Child handle are governed by the following key principles: 1. No Automatic Wait on Drop: When a Child handle is dropped, Tokio does not perform a blocking wait on the child process, even if kill_on_drop(true) is enabled [1][2]. Blocking the current task (or the executor thread) in a destructor is avoided to prevent stalling the runtime [1][2]. 2. kill_on_drop Semantics: When kill_on_drop is set to true, dropping the Child handle triggers a signal (SIGKILL on Unix) to the child process [3][1]. However, because this happens in the destructor, the process is not immediately reaped (i.e., waitpid is not called) [1][4]. This creates a "zombie" process state until the operating system or the Tokio runtime's background reaper processes it [5][1]. 3. Best-Effort Reaping: Tokio implements a best-effort mechanism to reap these orphaned processes [5][2]. This reaping process typically occurs as a side effect when other child processes are spawned or polled on the event loop [2][4]. If no further processes are spawned, there is no strict guarantee on when, or if, the zombie process will be fully reaped, which may lead to an accumulation of zombie processes in extreme cases [5][1][2]. 4. Recommended Best Practice: Because of these limitations, relying solely on drop for cleanup is discouraged for applications requiring strict resource management [5][1]. The authoritative way to ensure a child process is terminated and fully reaped is to manually await its completion using child.wait.await or child.kill.await [5][6][1]. These methods ensure that the process is explicitly signaled and, crucially, that the exit status is collected, preventing the process from lingering as a zombie [5][1][7]. Top Results: [5], [1], [2], [4]
Citations:
- 1: https://github.com/tokio-rs/tokio/blob/master/tokio/src/process/mod.rs
- 2: process: add ProcessDriver to handle orphan reaping tokio-rs/tokio#2907
- 3: https://docs.rs/tokio/latest/%20tokio/process/struct.Child.html
- 4: tokio::process::Command leaves zombies when child future is dropped tokio-rs/tokio#2685
- 5: https://docs.rs/tokio/latest/tokio/process/
- 6: https://docs.rs/tokio/latest/src/tokio/process/mod.rs.html
- 7: https://docs.rs/tokio/1.16.1/tokio/process/struct.Child.html
Own simulator shutdown and join all background work
crates/bmc-mock/src/ipmi_sim.rs#L65-L69:Droponly requests termination; move process cleanup into an explicit async shutdown path so the child is actually awaited and reaped.crates/bmc-mock/src/ipmi_sim.rs#L405-L412: accepted console sessions are spawned-and-forgotten; track them in aJoinSetso teardown cancels and joins active connections.
📍 Affects 1 file
crates/bmc-mock/src/ipmi_sim.rs#L65-L69(this comment)crates/bmc-mock/src/ipmi_sim.rs#L405-L412
🤖 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/ipmi_sim.rs` around lines 65 - 69, The simulator
currently requests child termination without awaiting cleanup, and console
session tasks are detached. In crates/bmc-mock/src/ipmi_sim.rs lines 65-69,
replace Drop-only process cleanup with an explicit async shutdown path that
terminates and awaits/reaps the child; in lines 405-412, track spawned console
sessions in a JoinSet and have shutdown cancel and join all active tasks.
Sources: Coding guidelines, Path instructions
| async fn serve_console(mut stream: TcpStream, prompt: &str) -> Result<(), std::io::Error> { | ||
| let mut input = Vec::new(); | ||
| let mut buffer = [0_u8; 32]; | ||
| loop { | ||
| let length = stream.read(&mut buffer).await?; | ||
| if length == 0 { | ||
| return Ok(()); | ||
| } | ||
| input.extend_from_slice(&buffer[..length]); | ||
| stream.write_all(&buffer[..length]).await?; | ||
| if input.ends_with(b"\n") || input.ends_with(b"\r") { | ||
| input.clear(); | ||
| stream.write_all(format!("\r\n{prompt}").as_bytes()).await?; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the unbounded console input buffer.
input grows until a newline arrives, allowing a stalled or malformed SOL client to consume memory indefinitely. The contents are otherwise unused, so test the current read slice directly.
Proposed fix
async fn serve_console(mut stream: TcpStream, prompt: &str) -> Result<(), std::io::Error> {
- let mut input = Vec::new();
let mut buffer = [0_u8; 32];
loop {
let length = stream.read(&mut buffer).await?;
if length == 0 {
return Ok(());
}
- input.extend_from_slice(&buffer[..length]);
stream.write_all(&buffer[..length]).await?;
- if input.ends_with(b"\n") || input.ends_with(b"\r") {
- input.clear();
+ if buffer[..length].ends_with(b"\n") || buffer[..length].ends_with(b"\r") {
stream.write_all(format!("\r\n{prompt}").as_bytes()).await?;
}
}
}As per path instructions, prioritize behavior and resource-lifetime defects in crates/**/*.rs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn serve_console(mut stream: TcpStream, prompt: &str) -> Result<(), std::io::Error> { | |
| let mut input = Vec::new(); | |
| let mut buffer = [0_u8; 32]; | |
| loop { | |
| let length = stream.read(&mut buffer).await?; | |
| if length == 0 { | |
| return Ok(()); | |
| } | |
| input.extend_from_slice(&buffer[..length]); | |
| stream.write_all(&buffer[..length]).await?; | |
| if input.ends_with(b"\n") || input.ends_with(b"\r") { | |
| input.clear(); | |
| stream.write_all(format!("\r\n{prompt}").as_bytes()).await?; | |
| } | |
| async fn serve_console(mut stream: TcpStream, prompt: &str) -> Result<(), std::io::Error> { | |
| let mut buffer = [0_u8; 32]; | |
| loop { | |
| let length = stream.read(&mut buffer).await?; | |
| if length == 0 { | |
| return Ok(()); | |
| } | |
| stream.write_all(&buffer[..length]).await?; | |
| if buffer[..length].ends_with(b"\n") || buffer[..length].ends_with(b"\r") { | |
| stream.write_all(format!("\r\n{prompt}").as_bytes()).await?; | |
| } | |
| } | |
| } |
🤖 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/ipmi_sim.rs` around lines 422 - 435, Update serve_console
to remove the unbounded input Vec and detect newline termination directly from
each read slice, while preserving the existing echo and prompt behavior. Use the
current buffer segment and length for the newline/carriage-return check, without
accumulating data across reads.
Source: Path instructions
| pub async fn update_password( | ||
| &self, | ||
| account_id: &str, | ||
| password: impl Into<String>, | ||
| ) -> Result<bool, String> { | ||
| let password = password.into(); | ||
| let account = self.find(account_id); | ||
| let Some(account) = account else { | ||
| return Ok(false); | ||
| }; | ||
| account.password = password.into(); | ||
| true | ||
| let updater = self | ||
| .password_updater | ||
| .lock() | ||
| .expect("mutex poisoned") | ||
| .as_ref() | ||
| .and_then(Weak::upgrade); | ||
| if let Some(updater) = updater { | ||
| updater | ||
| .update_password(&account.username, &account.password, &password) | ||
| .await?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Synchronize only the administrator account.
The updater is invoked for every Redfish account, but IpmiPasswordUpdater always changes IPMI user ID 3—the configured administrator. A non-administrator PATCH can therefore fail unexpectedly or alter unrelated IPMI credentials. Gate synchronization using a typed administrator role and add a non-administrator test.
As per coding guidelines, known finite Rust values must use enums rather than bare strings.
As per path instructions, BMC-facing code requires careful credential 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/src/redfish/account_service.rs` around lines 126 - 145,
Restrict the password updater invocation in update_password to accounts whose
typed role is Administrator, leaving non-administrator Redfish password updates
unaffected. Use the account role enum rather than comparing a string, and add a
test confirming non-administrator updates do not invoke synchronization or alter
IPMI credentials.
Sources: Coding guidelines, Path instructions
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Integrate ipmi_sim with bmc-mock and machine-a-tron so simulated BMCs can expose dynamically allocated IPMI LAN and serial-over-LAN endpoints. Keep Redfish and IPMI credentials synchronized, advertise the IPMI endpoint through Redfish, and support both per-machine and shared BMC-mock deployments. Make the feature opt-in because ipmi_sim and ipmitool are external runtime dependencies. Add IPMI-backed SSH console integration coverage, credential validation, dynamic port allocation, and cleanup of simulator processes and listener tasks. Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
ea41c8b to
02db974
Compare
3 participants
Machine-a-tron currently simulates BMC management through Redfish and optional SSH, but it cannot exercise workflows that depend on IPMI or Serial over LAN. This leaves the SSH console integration path dependent on separately managed simulator infrastructure and prevents local environments from representing an IPMI-capable BMC as one coherent mock.
This PR is the first step toward full IPMI support in machine-a-tron. It establishes the simulator lifecycle and integration foundation; broader IPMI command and behavior coverage will follow separately.
What changed
Related issues
Part of #3378
Type of Change
Breaking Changes
Testing
Additional Notes