fix: support SMC ARS-221GL-FNB-NC24B#3675
Draft
krish-nvidia wants to merge 1 commit into
Draft
Conversation
Signed-off-by: Krish Dandiwala <kdandiwala@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
Summary by CodeRabbit
WalkthroughThe change adds MGX-C2 chassis classification based on NVIDIA hardware identifiers and integrates the result into Supermicro exploration configuration and lockdown status evaluation using the in-band IPMI host-interface state. ChangesMGX-C2 Supermicro lockdown handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant nv_generate_exploration_report
participant ExploredChassisCollection
participant manager_Config
participant lockdown_status
nv_generate_exploration_report->>ExploredChassisCollection: call is_mgx_c2()
ExploredChassisCollection-->>nv_generate_exploration_report: return is_mgx_c2
nv_generate_exploration_report->>manager_Config: set need_oem_supermicro_kcs_interface to !is_mgx_c2
nv_generate_exploration_report->>lockdown_status: pass is_mgx_c2 and system model state
lockdown_status-->>nv_generate_exploration_report: compute Supermicro lockdown status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![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
🤖 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-explorer/src/lib.rs`:
- Around line 668-703: Add table-driven tests for the lockdown status logic
around the MGX-C2 and Supermicro model branches, covering
ipmi_host_interface_enabled values Some(true), Some(false), and None for both
ARS-121L-DNR and a normal Supermicro model. Assert the resulting
InternalLockdownStatus and externally reported message/state, including the
ARS-121L-DNR PXE host-interface exception.
🪄 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: c73e4cbf-599d-4418-8539-ffdec1009a7e
📒 Files selected for processing (2)
crates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/lib.rs
Comment on lines
+668
to
+703
| let ipmi_host_interface_enabled = system | ||
| .raw() | ||
| .ipmi_host_interface | ||
| .as_ref() | ||
| .and_then(|interface| interface.service_enabled); | ||
| let message = format!( | ||
| "SysLockdownEnabled={is_syslockdown}, kcs_privilege={kcs_privilege:#?}, host_interface_enabled={hi_enabled}" | ||
| "SysLockdownEnabled={is_syslockdown}, kcs_privilege={kcs_privilege:#?}, \ | ||
| host_interface_enabled={hi_enabled}, \ | ||
| ipmi_host_interface_enabled={ipmi_host_interface_enabled:?}" | ||
| ); | ||
|
|
||
| let model = system.hardware_id().model.map(|v| v.into_inner()); | ||
| if model == Some("ARS-121L-DNR") { | ||
| // Grace-Grace SMCs (ARS-121L-DNR): | ||
| // 1. Need host_interface enabled even with lockdown | ||
| // 2. Doesn't provide KCSInterface | ||
| match (hi_enabled, is_syslockdown) { | ||
| (true, true) => Ok(InternalLockdownStatus::Enabled), | ||
| (true, false) => Ok(InternalLockdownStatus::Disabled), | ||
| _ => Ok(InternalLockdownStatus::Partial), | ||
| } | ||
| let is_ars_121l_dnr = model == Some("ARS-121L-DNR"); | ||
| let (inband_locked, inband_unlocked) = if is_mgx_c2 { | ||
| ( | ||
| ipmi_host_interface_enabled.is_none_or(|enabled| !enabled), | ||
| ipmi_host_interface_enabled.is_none_or(identity), | ||
| ) | ||
| } else { | ||
| match (hi_enabled, kcs_privilege, is_syslockdown) { | ||
| (false, Some(SupermicroPrivilege::Callback), true) => { | ||
| Ok(InternalLockdownStatus::Enabled) | ||
| } | ||
| (true, Some(SupermicroPrivilege::Administrator), false) => { | ||
| Ok(InternalLockdownStatus::Disabled) | ||
| } | ||
| (true, None, false) => Ok(InternalLockdownStatus::Disabled), | ||
| _ => Ok(InternalLockdownStatus::Partial), | ||
| } | ||
| } | ||
| .map(|status| Some(LockdownStatus { status, message })) | ||
| ( | ||
| kcs_privilege == Some(SupermicroPrivilege::Callback), | ||
| kcs_privilege.is_none() | ||
| || kcs_privilege == Some(SupermicroPrivilege::Administrator), | ||
| ) | ||
| }; | ||
| // ARS-121L-DNR must keep HostInterface enabled to PXE boot. | ||
| let host_interface_locked = hi_enabled == is_ars_121l_dnr; | ||
|
|
||
| let status = if is_syslockdown && inband_locked && host_interface_locked { | ||
| InternalLockdownStatus::Enabled | ||
| } else if !is_syslockdown && inband_unlocked && hi_enabled { | ||
| InternalLockdownStatus::Disabled | ||
| } else { | ||
| InternalLockdownStatus::Partial | ||
| }; | ||
| Ok(Some(LockdownStatus { status, message })) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline crates/bmc-explorer/src/lib.rs --items all --type function
rg -n -C 4 -g '*.rs' \
'lockdown_status|is_mgx_c2|ipmi_host_interface|ARS-121L-DNR' \
crates/bmc-explorerRepository: NVIDIA/infra-controller
Length of output: 15008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the lockdown_status implementation and any tests in the same file.
ast-grep outline crates/bmc-explorer/src/lib.rs --match lockdown_status --view expanded
# Inspect the surrounding test module if present.
rg -n -C 6 'mod tests|lockdown_status|InternalLockdownStatus|ARS-121L-DNR|is_mgx_c2' crates/bmc-explorer/src/lib.rs
# Look for dedicated lockdown-status test cases anywhere in bmc-explorer tests.
rg -n -C 3 'lockdown status|lockdown_status|InternalLockdownStatus::(Enabled|Disabled|Partial)|ARS-121L-DNR|MGX-C2|host_interface' crates/bmc-explorer/tests crates/bmc-explorer/srcRepository: NVIDIA/infra-controller
Length of output: 34347
Add table-driven coverage for the Supermicro lockdown matrix. Cover MGX-C2 with ipmi_host_interface_enabled set to Some(true), Some(false), and None, and include both ARS-121L-DNR and a normal Supermicro model so the PXE exception and externally reported lockdown state stay correct.
🤖 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-explorer/src/lib.rs` around lines 668 - 703, Add table-driven
tests for the lockdown status logic around the MGX-C2 and Supermicro model
branches, covering ipmi_host_interface_enabled values Some(true), Some(false),
and None for both ARS-121L-DNR and a normal Supermicro model. Assert the
resulting InternalLockdownStatus and externally reported message/state,
including the ARS-121L-DNR PXE host-interface exception.
Source: Coding guidelines
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:
PG535/2G535processor module.IPMIHostInterfacewhen available and evaluate Supermicro lockdown consistently with libredfish.ARS-121L-DNRHostInterface PXE exception.The libredfish version needs to be bumped once the following PR is merged: NVIDIA/libredfish#106
Related issues
#3623
Type of Change
Breaking Changes
Testing
Additional Notes