feat: add force-restart verification support for GB200s - #4376
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Summary by CodeRabbit
WalkthroughThe workspace updates ChangesGB200 restart verification and boot-option mock updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RedfishClient
participant BMCMockRoute
participant SingleSystemState
RedfishClient->>BMCMockRoute: PATCH boot option settings
BMCMockRoute->>SingleSystemState: Merge boot-option override
SingleSystemState-->>BMCMockRoute: Return success or 404
RedfishClient->>BMCMockRoute: GET boot option
BMCMockRoute->>SingleSystemState: Render base option with overrides
SingleSystemState-->>BMCMockRoute: Return overridden boot-option JSON
Possibly related PRs
🚥 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/machine-controller/src/handler.rs (1)
2299-2299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid a redundant exact-match entry.
The existing generic matcher already recognizes this message because
ForceWarmRebootcontainsrebootafter lowercasing. Remove this entry, or change the matching strategy and add a regression test if exact GB200 matching is intentionally required.🤖 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-controller/src/handler.rs` at line 2299, Remove the redundant exact-match message entry near the existing generic reboot matcher in the relevant handler configuration. Preserve the generic case-insensitive matching behavior for messages containing “reboot”; only retain a GB200-specific entry if you also change the matching strategy and add regression coverage for exact matching.
🤖 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/machine-controller/src/handler.rs`:
- Line 2299: Remove the redundant exact-match message entry near the existing
generic reboot matcher in the relevant handler configuration. Preserve the
generic case-insensitive matching behavior for messages containing “reboot”;
only retain a GB200-specific entry if you also change the matching strategy and
add regression coverage for exact matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e3a58ccb-373a-47b2-a60c-274466be6c59
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.tomlcrates/machine-controller/src/handler.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/bmc-mock/src/redfish/computer_system.rs (1)
308-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the redundant clone when merging boot-option overrides.
current.clone().patch(patch_request)clones the full stored override JSON right before overwriting it. Sincecurrentis a&mut serde_json::ValuefromEntry::or_insert_with, useserde_json::Value::take()to move the value out (leavingNullin its place) instead of cloning it.♻️ Proposed fix
let mut overrides = self.boot_option_overrides.lock().expect("mutex poisoned"); let current = overrides .entry(option_id.to_string()) .or_insert_with(|| json!({})); - *current = current.clone().patch(patch_request); + *current = current.take().patch(patch_request); trueAs per coding guidelines, "Avoid needless
.clone()calls; prefer borrowing, moving withinto_iter, ordering struct fields to enable moves, or usingCowwhere values may be borrowed or owned."🤖 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 308 - 332, Update patch_boot_option to replace current.clone().patch(patch_request) with a take-based move from the mutable entry, leaving Null in the entry before assigning the patched value. Keep the existing entry initialization and boolean return behavior unchanged.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.
Nitpick comments:
In `@crates/bmc-mock/src/redfish/computer_system.rs`:
- Around line 308-332: Update patch_boot_option to replace
current.clone().patch(patch_request) with a take-based move from the mutable
entry, leaving Null in the entry before assigning the patched value. Keep the
existing entry initialization and boolean return behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ec3c9a34-76ef-4a57-b3c2-fa71efbb09f7
📒 Files selected for processing (1)
crates/bmc-mock/src/redfish/computer_system.rs
672e19b to
5af969e
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4376.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/bmc-mock/src/redfish/computer_system.rs (3)
887-891: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAccept partial HPE settings payloads.
Json<HpeBootSettingsPatch>requiresPersistentBootConfigOrder. If a client stages any other HPE boot property, axum rejects the request with422before the handler runs. The mock then diverges from the permissive behavior used elsewhere incrates/bmc-mock. Make the field optional and ignore the request when it is absent.♻️ Proposed refactor
struct HpeBootSettingsPatch { - persistent_boot_config_order: Vec<String>, + persistent_boot_config_order: Option<Vec<String>>, }- system_state.set_hpe_boot_order(request.persistent_boot_config_order); + if let Some(boot_order) = request.persistent_boot_config_order { + system_state.set_hpe_boot_order(boot_order); + } json!({}).into_ok_response()Based on learnings: in the Redfish mock server used for test support, prefer permissive behavior over strict input validation for malformed or partial payloads.
🤖 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 887 - 891, Update HpeBootSettingsPatch and patch_hpe_boot_settings so PersistentBootConfigOrder is optional during JSON deserialization, allowing partial HPE boot settings payloads to reach the handler. When the field is absent, ignore the request while preserving existing behavior when it is present.Source: Learnings
1237-1303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the boot-option override round trip.
The test module covers only the HPE OEM ordering path. The primary change of this layer,
patch_boot_option_settingsplus the override-awareget_boot_optionresponse, has no test. A short test that PATCHes a boot-option settings resource, then asserts the merged GET body, and asserts404for an unknown boot-option ID would lock in the new contract consumed bylog_host_configincrates/machine-controller/src/handler.rs.I can generate that test if you want.
🤖 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 1237 - 1303, The tests module only covers HPE ordering; add coverage for the boot-option override flow implemented by patch_boot_option_settings and get_boot_option. Create a test that PATCHes a boot-option settings resource, verifies the subsequent GET contains the merged override values, and verifies GET for an unknown boot-option ID returns 404, preserving the contract used by log_host_config.Source: Coding guidelines
358-369: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the needless clone of the stored override.
current.clone()copies the whole persisted JSON object on every PATCH.serde_json::ValueimplementsDefault, sostd::mem::takemoves the value out in place and keeps the merge semantics identical.Also note the lock style: the two new call sites use
.expect("mutex poisoned"), while the adjacent methods in this file use.unwrap(). Align them for consistency.♻️ Proposed refactor
- let mut overrides = self.boot_option_overrides.lock().expect("mutex poisoned"); + let mut overrides = self.boot_option_overrides.lock().unwrap(); let current = overrides .entry(option_id.to_string()) .or_insert_with(|| json!({})); - *current = current.clone().patch(patch_request); + *current = std::mem::take(current).patch(patch_request); trueAs per coding guidelines: "Avoid needless
.clone()calls; prefer borrowing, moving withinto_iter, ordering struct fields to enable moves".🤖 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 358 - 369, Update patch_boot_option to replace current.clone() with std::mem::take(current) before applying patch, preserving the existing merge behavior without cloning the stored JSON value. Change the boot_option_overrides lock handling in this method to use .unwrap() consistently with adjacent methods.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.
Nitpick comments:
In `@crates/bmc-mock/src/redfish/computer_system.rs`:
- Around line 887-891: Update HpeBootSettingsPatch and patch_hpe_boot_settings
so PersistentBootConfigOrder is optional during JSON deserialization, allowing
partial HPE boot settings payloads to reach the handler. When the field is
absent, ignore the request while preserving existing behavior when it is
present.
- Around line 1237-1303: The tests module only covers HPE ordering; add coverage
for the boot-option override flow implemented by patch_boot_option_settings and
get_boot_option. Create a test that PATCHes a boot-option settings resource,
verifies the subsequent GET contains the merged override values, and verifies
GET for an unknown boot-option ID returns 404, preserving the contract used by
log_host_config.
- Around line 358-369: Update patch_boot_option to replace current.clone() with
std::mem::take(current) before applying patch, preserving the existing merge
behavior without cloning the stored JSON value. Change the boot_option_overrides
lock handling in this method to use .unwrap() consistently with adjacent
methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0d7439f8-32cc-443f-8c81-e7ec5c1a0e86
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlcrates/bmc-mock/src/redfish/computer_system.rscrates/machine-controller/src/handler.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- crates/machine-controller/src/handler.rs
5af969e to
7894254
Compare
Signed-off-by: Alex Ball <aball@nvidia.com>
2 participants
This PR adds force-restart verification support for GB200s by looking for the following BMC log entry immediately after a force-restart is issued (manually verified):
It also bumps libredfish to
v0.46.1which brings in the following changes:Related issues
NVBug 6520998
Type of Change
Breaking Changes
Testing
Additional Notes