feat(bmc): propagate discovered IPMI port - #3687
Merged
Merged
Conversation
Contributor
|
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 (12)
🚧 Files skipped from review as they are similar to previous changes (11)
Summary by CodeRabbit
WalkthroughThe change adds optional IPMI port extraction to BMC exploration, carries it through manager reports and database lookup, and returns it from the BMC metadata API with handling for missing and invalid values. ChangesIPMI metadata propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BmcExplorer
participant RedfishManager
participant ExplorationReport
participant MetadataApi
participant Database
BmcExplorer->>RedfishManager: Read network protocol data
RedfishManager-->>BmcExplorer: Return IPMI enabled state and port
BmcExplorer->>ExplorationReport: Store validated ipmi_port
MetadataApi->>Database: Look up metadata by endpoint IP
Database-->>MetadataApi: Return vendor and parsed ipmi_port
MetadataApi-->>MetadataApi: Build BMC metadata response
🚥 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: 2
🧹 Nitpick comments (1)
crates/bmc-explorer/src/manager.rs (1)
262-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository table-test helper.
This validator’s input matrix should use
check_casesorscenarios!instead of a hand-rolled case loop.As per coding guidelines, “use table-driven tests using
carbide-test-supportscenarios (scenarios!/value_scenarios!) or explicit cases (check_cases/check_values) for parsers, validators, conversions, and similar input-variant coverage.”🤖 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/manager.rs` around lines 262 - 295, The test extracts its own table-test loop instead of using the repository helper. Update tests::extracts_only_enabled_valid_ipmi_ports to express the existing input matrix with carbide-test-support’s check_cases or scenarios! helper, preserving all case names, inputs, and expected results.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/api-core/tests/integration/machine_bmc_metadata.rs`:
- Around line 177-205: Extend the invalid_port table in the BMC metadata test to
include the JSON value "null". Keep the existing database update, API request,
and assertions unchanged so the lookup path verifies that an explicitly null
IpmiPort returns None.
In `@crates/bmc-explorer/src/manager.rs`:
- Around line 36-44: Update enabled_ipmi_port in
crates/bmc-explorer/src/manager.rs to return None when an enabled reported port
is zero, while preserving validation for other values, and add a matching test
case. In crates/api-db/src/explored_endpoints.rs at the specified site, filter
parsed zero ports before exposing them to API clients.
---
Nitpick comments:
In `@crates/bmc-explorer/src/manager.rs`:
- Around line 262-295: The test extracts its own table-test loop instead of
using the repository helper. Update
tests::extracts_only_enabled_valid_ipmi_ports to express the existing input
matrix with carbide-test-support’s check_cases or scenarios! helper, preserving
all case names, inputs, and expected results.
🪄 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: 7eebc63a-2f98-4f32-bfab-45af1f73004c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlcrates/api-core/src/handlers/bmc_metadata.rscrates/api-core/tests/integration/machine_bmc_metadata.rscrates/api-db/src/explored_endpoints.rscrates/api-model/src/site_explorer/mod.rscrates/api-model/src/test_support/dpu.rscrates/api-model/src/test_support/managed_host.rscrates/bmc-explorer/Cargo.tomlcrates/bmc-explorer/src/manager.rscrates/bmc-explorer/tests/integration/supermicro_gb300_explore.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/redfish.rs
Comment on lines
+177
to
+205
| for invalid_port in [r#""not-a-port""#, "65536", "2147483648"] { | ||
| let mut txn = env.db_txn().await; | ||
| sqlx::query( | ||
| "UPDATE explored_endpoints SET exploration_report = \ | ||
| jsonb_set(exploration_report, '{Managers,0,IpmiPort}', $2::jsonb, true) \ | ||
| WHERE address = $1", | ||
| ) | ||
| .bind(host_bmc_ip) | ||
| .bind(invalid_port) | ||
| .execute(txn.as_mut()) | ||
| .await | ||
| .unwrap(); | ||
| txn.commit().await.unwrap(); | ||
|
|
||
| let metadata = env | ||
| .api() | ||
| .get_bmc_meta_data(tonic::Request::new(rpc::forge::BmcMetaDataGetRequest { | ||
| machine_id: host_machine.id.clone(), | ||
| request_type: rpc::forge::BmcRequestType::Ipmi.into(), | ||
| role: rpc::forge::UserRoles::Administrator.into(), | ||
| bmc_endpoint_request: None, | ||
| })) | ||
| .await | ||
| .unwrap() | ||
| .into_inner(); | ||
|
|
||
| assert_eq!(metadata.ipmi_port, None, "stored port: {invalid_port}"); | ||
| assert!(metadata.vendor.is_some_and(|vendor| !vendor.is_empty())); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover an explicitly stored JSON null.
The default assertion covers an omitted IpmiPort; these cases cover malformed/out-of-range values, but not IpmiPort: null. Add "null" to this case table so the database lookup path is verified for the promised null-safe behavior.
🤖 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-core/tests/integration/machine_bmc_metadata.rs` around lines 177 -
205, Extend the invalid_port table in the BMC metadata test to include the JSON
value "null". Keep the existing database update, API request, and assertions
unchanged so the lookup path verifies that an explicitly null IpmiPort returns
None.
Comment on lines
+36
to
+44
| fn enabled_ipmi_port( | ||
| protocol_enabled: Option<Option<bool>>, | ||
| port: Option<Option<i64>>, | ||
| ) -> Result<Option<u16>, TryFromIntError> { | ||
| if protocol_enabled != Some(Some(true)) { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| port.flatten().map(u16::try_from).transpose() |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject IPMI port zero throughout the metadata flow.
u16 conversion alone accepts zero, but zero is unusable and prevents consumers from applying their port-623 fallback.
crates/bmc-explorer/src/manager.rs#L36-L44: returnNonefor an enabled reported port of zero and add a matching test case.crates/api-db/src/explored_endpoints.rs#L349-L349: filter parsed zero values so malformed or historical JSON cannot reach API clients.
📍 Affects 2 files
crates/bmc-explorer/src/manager.rs#L36-L44(this comment)crates/api-db/src/explored_endpoints.rs#L349-L349
🤖 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/manager.rs` around lines 36 - 44, Update
enabled_ipmi_port in crates/bmc-explorer/src/manager.rs to return None when an
enabled reported port is zero, while preserving validation for other values, and
add a matching test case. In crates/api-db/src/explored_endpoints.rs at the
specified site, filter parsed zero ports before exposing them to API clients.
Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
poroh
force-pushed
the
ipmi-port-plumbing
branch
from
July 17, 2026 20:09
89c84ec to
9df1010
Compare
wminckler
approved these changes
Jul 17, 2026
2 tasks
2 tasks
2 tasks
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
2 participants
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.
Redfish exposes the configured IPMI endpoint through ManagerNetworkProtocol. NICo previously discarded this information, causing consumers such as ssh-console to always fall back to the conventional IPMI port 623.
This change discovers the enabled IPMI port from ManagerNetworkProtocol, stores it in the exploration report, and returns it through the existing BMC metadata API. Consumers can now use the port reported by the BMC while retaining 623 as a fallback when no port is available.
As a secondary benefit, this enables end-to-end IPMI SOL testing with machine-a-tron and bmc-mock, where IPMI simulators use dynamically allocated ports.
Related issues
#3378
Type of Change
Breaking Changes
Testing
Additional Notes