feat(machine-a-tron): simulate WIWYNN GB200 racks - #4422
Open
poroh wants to merge 2 commits into
Open
Conversation
Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
Contributor
Summary by CodeRabbit
WalkthroughChangesRack lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Config
participant MachineATron
participant SimulatorRegistry
participant ControlRouter
participant APIClient
Config->>MachineATron: resolve rack configuration
MachineATron->>SimulatorRegistry: register resolved machines and racks
APIClient->>ControlRouter: request rack status
ControlRouter->>SimulatorRegistry: retrieve rack status
SimulatorRegistry-->>ControlRouter: return member device statuses
ControlRouter-->>APIClient: serialize rack status response
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
crates/api-integration-tests/tests/rack.rs (1)
110-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared
MachineATronConfigdefaults to avoid duplicated boilerplate. Both files build a near-identical ~20-field literal (dhcp,log_file,bmc_mock_port,interface,tui_enabled,use_single_bmc_mock,configure_carbide_bmc_proxy_host,persist_dir,cleanup_on_quit,register_expected_machines,host_bmc_password,dpu_bmc_password,api_refresh_interval,mock_bmc_ssh_server,mock_bmc_ssh_port,enable_ipmi_simulation,hw_mac_address_ranges,mac_address_pool) for every test-harness invocation.
crates/api-integration-tests/tests/rack.rs#L110-L156: replace the inline literal's shared defaults with a common constructor/builder call, keeping only the rack-specificracks/machines/carbide_api_urlfields inline.crates/api-integration-tests/tests/lib.rs#L1371-L1419: same — factor the identical defaults into the shared constructor so both harnesses consume one source of truth for test-harness defaults.As per coding guidelines, "builders mainly for large configurations with optional/defaultable fields" —
MachineATronConfigqualifies given most of its fields already carry#[serde(default = ...)].🤖 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-integration-tests/tests/rack.rs` around lines 110 - 156, Add a shared constructor or builder for MachineATronConfig defaults, then update crates/api-integration-tests/tests/rack.rs:110-156 and crates/api-integration-tests/tests/lib.rs:1371-1419 to use it, keeping only each harness’s racks, machines, and carbide_api_url overrides inline. Ensure both sites consume the same defaults for all listed shared fields.Source: Coding guidelines
crates/machine-a-tron/src/rack.rs (1)
40-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
device_indexis registry-relative.
RackMemberRefis only meaningful against theSimulatorRegistrydevice vector that produced it. A one-line doc comment removes the ambiguity for future callers.📝 Suggested addition
#[derive(Clone, Debug)] pub(crate) struct RackMemberRef { pub position: u8, + /// Index into the owning `SimulatorRegistry` device list; not valid across registries. pub device_index: usize, }🤖 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/machine-a-tron/src/rack.rs` around lines 40 - 52, Add a one-line doc comment to the device_index field in RackMemberRef stating that it is an index relative to the SimulatorRegistry device vector that produced the reference.crates/machine-a-tron/src/config.rs (2)
493-497: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer a fallible error over
expectin a config-resolution path.
resolved_device_configsalready returnseyre::Result, so an unsupported rack hardware type should surface as a configuration error rather than a panic. The current invariant is only enforced by the elevation test incrates/bmc-mock/src/hw/wiwynn_gb200_nvl72_rack.rs; a future rack model could silently introduce a panic here.♻️ Proposed change
- let dpu_per_host_count = unit - .hardware_type - .fixed_number_of_dpu() - .expect("rack hardware must have a fixed number of DPUs") - .into(); + let dpu_per_host_count = u32::from( + unit.hardware_type.fixed_number_of_dpu().ok_or_else(|| { + eyre::eyre!( + "rack {rack_id} unit {} ({}) has no fixed DPU count", + unit.position, + unit.hardware_type + ) + })?, + );🤖 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/machine-a-tron/src/config.rs` around lines 493 - 497, Update the config-resolution logic containing dpu_per_host_count to propagate fixed_number_of_dpu() failure through the existing eyre::Result returned by resolved_device_configs, replacing the panic-producing expect. Preserve the successful conversion to the DPU count while returning a configuration error for unsupported rack hardware types.Source: Coding guidelines
141-211: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRack simulation settings duplicate
MachineConfigfield-for-field.Thirteen fields plus a 28-line manual mapper now mirror
MachineConfig(lines 72-132). Any future simulation knob must be added in three places, and the test helperwiwynn_gb200_rack_from_machineexists purely to bridge the two shapes. Extracting the shared block into one flattened struct keeps the TOML wire format identical while eliminating the mapper.♻️ Sketch
#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)] pub struct SimulationSettings { pub dpu_reboot_delay: u64, pub host_reboot_delay: u64, // ... remaining shared fields with their existing serde attributes } pub struct MachineConfig { #[serde(flatten)] pub simulation: SimulationSettings, // machine-only fields: rack_id, hw_type, host_count, ... } pub struct WiwynnGb200RackConfig { #[serde(flatten)] pub simulation: SimulationSettings, }🤖 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/machine-a-tron/src/config.rs` around lines 141 - 211, Extract the duplicated simulation fields and their existing serde attributes from MachineConfig and WiwynnGb200RackConfig into a shared SimulationSettings struct. Flatten this struct into both configurations to preserve the current TOML wire format, and update component_machine_config and all field accesses to use the shared settings. Remove the manual field-by-field mapper and adjust wiwynn_gb200_rack_from_machine and related construction code to reuse SimulationSettings directly.Source: Coding guidelines
crates/bmc-mock/src/hw/nvidia_gb200.rs (1)
24-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree adjacent
HardwareTypeparameters invite silent transposition.The signature declares
compute_tray, power_shelf, nvlink_switch_traywhile the body populates compute → switch → shelf. Since all three share a type, a swapped call site compiles cleanly and yields a wrong elevation. Consider a small named-field parameter struct so callers cannot mis-order it.♻️ Suggested parameter struct
pub(crate) struct Nvl72Trays { pub compute_tray: HardwareType, pub nvlink_switch_tray: HardwareType, pub power_shelf: HardwareType, } pub(crate) fn nvl72_rack_elevation(trays: Nvl72Trays) -> RackElevation { /* ... */ }🤖 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/hw/nvidia_gb200.rs` around lines 24 - 28, Replace the three positional HardwareType parameters of nvl72_rack_elevation with a named Nvl72Trays struct containing compute_tray, nvlink_switch_tray, and power_shelf fields, then update the function body and all call sites to access or initialize those fields by name while preserving the existing elevation behavior.Source: Coding guidelines
🤖 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/machine-a-tron/config/mat.toml`:
- Around line 85-98: Update the commented standalone rack_id example in the
[machines.config] section to use a rack identifier distinct from the atomic rack
IDs shown in [racks.default], while preserving the existing example structure
and explanatory comments.
---
Nitpick comments:
In `@crates/api-integration-tests/tests/rack.rs`:
- Around line 110-156: Add a shared constructor or builder for
MachineATronConfig defaults, then update
crates/api-integration-tests/tests/rack.rs:110-156 and
crates/api-integration-tests/tests/lib.rs:1371-1419 to use it, keeping only each
harness’s racks, machines, and carbide_api_url overrides inline. Ensure both
sites consume the same defaults for all listed shared fields.
In `@crates/bmc-mock/src/hw/nvidia_gb200.rs`:
- Around line 24-28: Replace the three positional HardwareType parameters of
nvl72_rack_elevation with a named Nvl72Trays struct containing compute_tray,
nvlink_switch_tray, and power_shelf fields, then update the function body and
all call sites to access or initialize those fields by name while preserving the
existing elevation behavior.
In `@crates/machine-a-tron/src/config.rs`:
- Around line 493-497: Update the config-resolution logic containing
dpu_per_host_count to propagate fixed_number_of_dpu() failure through the
existing eyre::Result returned by resolved_device_configs, replacing the
panic-producing expect. Preserve the successful conversion to the DPU count
while returning a configuration error for unsupported rack hardware types.
- Around line 141-211: Extract the duplicated simulation fields and their
existing serde attributes from MachineConfig and WiwynnGb200RackConfig into a
shared SimulationSettings struct. Flatten this struct into both configurations
to preserve the current TOML wire format, and update component_machine_config
and all field accesses to use the shared settings. Remove the manual
field-by-field mapper and adjust wiwynn_gb200_rack_from_machine and related
construction code to reuse SimulationSettings directly.
In `@crates/machine-a-tron/src/rack.rs`:
- Around line 40-52: Add a one-line doc comment to the device_index field in
RackMemberRef stating that it is an index relative to the SimulatorRegistry
device vector that produced the reference.
🪄 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: ed15e692-2fb5-4acd-a113-2793f7de4967
📒 Files selected for processing (17)
crates/api-integration-tests/tests/lib.rscrates/api-integration-tests/tests/rack.rscrates/bmc-mock/src/hw/mod.rscrates/bmc-mock/src/hw/nvidia_gb200.rscrates/bmc-mock/src/hw/rack.rscrates/bmc-mock/src/hw/wiwynn_gb200_nvl72_rack.rscrates/bmc-mock/src/lib.rscrates/bmc-mock/src/rack_info.rscrates/machine-a-tron/config/mat.tomlcrates/machine-a-tron/src/config.rscrates/machine-a-tron/src/control_router.rscrates/machine-a-tron/src/device_handle.rscrates/machine-a-tron/src/host_machine.rscrates/machine-a-tron/src/lib.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-a-tron/src/rack.rscrates/machine-a-tron/src/simulator_registry.rs
Comment on lines
+85
to
98
| # [racks.default] | ||
| # type = "wiwynn_gb200_nvl72" | ||
| # rack_profile_id = "NVL72" | ||
| # ids = ["rack-001", "rack-002"] | ||
| # dpu_reboot_delay = 20 | ||
| # host_reboot_delay = 10 | ||
| # oob_dhcp_relay_address = "192.168.192.1" | ||
| # admin_dhcp_relay_address = "192.168.176.1" | ||
| # host_inband_dhcp_relay_address = "192.168.177.1" | ||
|
|
||
| [machines.config] | ||
| # Standalone devices can share any rack ID. They cannot be added to one of the | ||
| # atomic racks declared above. | ||
| # rack_id = "rack-001" |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The two examples conflict if both are uncommented.
ids = ["rack-001", "rack-002"] and the standalone rack_id = "rack-001" below cannot coexist: validation rejects standalone devices targeting an atomic rack. Using a distinct ID in the standalone example makes the sample config copy-paste safe.
🩹 Suggested tweak
-# rack_id = "rack-001"
+# rack_id = "rack-100"📝 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.
Suggested change
| # [racks.default] | |
| # type = "wiwynn_gb200_nvl72" | |
| # rack_profile_id = "NVL72" | |
| # ids = ["rack-001", "rack-002"] | |
| # dpu_reboot_delay = 20 | |
| # host_reboot_delay = 10 | |
| # oob_dhcp_relay_address = "192.168.192.1" | |
| # admin_dhcp_relay_address = "192.168.176.1" | |
| # host_inband_dhcp_relay_address = "192.168.177.1" | |
| [machines.config] | |
| # Standalone devices can share any rack ID. They cannot be added to one of the | |
| # atomic racks declared above. | |
| # rack_id = "rack-001" | |
| # [racks.default] | |
| # type = "wiwynn_gb200_nvl72" | |
| # rack_profile_id = "NVL72" | |
| # ids = ["rack-001", "rack-002"] | |
| # dpu_reboot_delay = 20 | |
| # host_reboot_delay = 10 | |
| # oob_dhcp_relay_address = "192.168.192.1" | |
| # admin_dhcp_relay_address = "192.168.176.1" | |
| # host_inband_dhcp_relay_address = "192.168.177.1" | |
| [machines.config] | |
| # Standalone devices can share any rack ID. They cannot be added to one of the | |
| # atomic racks declared above. | |
| # rack_id = "rack-100" |
🤖 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/machine-a-tron/config/mat.toml` around lines 85 - 98, Update the
commented standalone rack_id example in the [machines.config] section to use a
rack identifier distinct from the atomic rack IDs shown in [racks.default],
while preserving the existing example structure and explanatory comments.
10 tasks
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds first-class simulation of a WIWYNN GB200 NVL72 rack in machine-a-tron.
Previously, machine-a-tron could simulate the individual hardware components but did not understand a rack as an atomic hardware model. This made it difficult to register an expected rack in Carbide and simulate all devices at their correct rack positions.
The new rack model is defined in bmc-mock and uses the NVIDIA GB200 rack design with WIWYNN compute trays, NVIDIA ND5200 NVLink switch trays, and LiteOn power shelves. Machine-a-tron expands each configured rack into its devices, registers the expected rack using its configured rack_profile_id, and reports the devices through the rack and machine status endpoints.
Rack-specific configuration uses a tagged model enum, keeping WIWYNN fields isolated and allowing future rack types without adding their settings to every RackConfig. Expected-rack registration and device creation also use the same resolved rack representation to avoid configuration drift.
This change intentionally supports only the WIWYNN GB200 NVL72 rack. Standalone machine configuration remains available for custom hardware combinations.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes
The rack profile itself remains owned by Carbide. Machine-a-tron accepts the rack_profile_id to assign when registering each expected rack; it does not redefine or reconfigure that profile.