fix(dell): match DPU HTTP boot option by name prefix, not exact equality#93
fix(dell): match DPU HTTP boot option by name prefix, not exact equality#93kirson-git wants to merge 1 commit into
Conversation
set_boot_order_dpu_first / is_boot_order_setup / machine_setup_status all
compared the expected boot option name "HTTP Device 1: {DeviceDescription}"
for *exact* equality against the boot option display_name. On Dell iDRAC the
real name has a suffix, e.g.:
"HTTP Device 1: NIC in Slot 40 Port 1 Partition 1 - Nvidia Network Adapter - C4:70:BD:2C:3C:0A - IPv4"
so exact ==/!= never matches: set fails with MissingBootOption, and the
verify path (is_boot_order_setup) reports the order unconfigured. Downstream
(NICo) this wedges host provisioning at SetBootOrder/CheckBootOrder.
Match by prefix (starts_with) in all three places. Verified on a live
BlueField-3 + Dell PowerEdge XE9680 (iDRAC): the set fix advanced the state
SetBootOrder -> CheckBootOrder, and the verify fix clears CheckBootOrder.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
c68209a to
b3ce6ac
Compare
| .get_expected_and_actual_first_boot_option(crate::BootInterfaceRef::Mac(mac)) | ||
| .await?; | ||
| if expected.is_none() || expected != actual { | ||
| if !matches!((&expected, &actual), (Some(e), Some(a)) if a.starts_with(e)) { |
There was a problem hiding this comment.
starts_with is too broad. For example, an expected Partition 1 can prefix-match Partition 10. Also this same in-line expression is used 3 times. I would use a matcher function like this:
fn boot_option_name_matches(expected: &str, actual: &str) -> bool {
actual == expected
|| actual
.strip_prefix(expected)
.is_some_and(|suffix| suffix.starts_with(" - "))
}
There was a problem hiding this comment.
I would also add a unit test, something like this:
#[test]
fn boot_option_name_matches_legacy_and_extended_names() {
let expected = "HTTP Device 1: NIC in Slot 4 Port 1 Partition 1";
let cases = [
(expected, true),
(
"HTTP Device 1: NIC in Slot 4 Port 1 Partition 1 - Nvidia Network Adapter - 00:11:22:33:44:55 - IPv4",
true,
),
(
"HTTP Device 1: NIC in Slot 4 Port 1 Partition 10 - Nvidia Network Adapter - 00:11:22:33:44:55 - IPv4",
false,
),
(
"HTTP Device 1: NIC in Slot 4 Port 2 Partition 1 - Nvidia Network Adapter - 00:11:22:33:44:55 - IPv4",
false,
),
(
"HTTP Device 1: NIC in Slot 4 Port 1 Partition 1 unexpected suffix",
false,
),
];
for (actual, should_match) in cases {
assert_eq!(
boot_option_name_matches(expected, actual),
should_match,
"unexpected match result for {actual}"
);
}
}
williampnvidia
left a comment
There was a problem hiding this comment.
Please replace the unrestricted prefix checks with the bounded boot_option_name_matches() helper discussed in the inline review, add the proposed regression test, and use the helper at all three modified call sites: machine_setup_status, set_boot_order_dpu_first, and is_boot_order_setup. This preserves support for both legacy and extended Dell boot-option names while preventing false matches such as Partition 1 matching Partition 10.
## Summary - recognize both legacy exact Dell HTTP boot-option names and newer names with a `" - "`-delimited adapter/MAC/protocol suffix - use one bounded matcher in `machine_setup_status`, `set_boot_order_dpu_first`, and `is_boot_order_setup` - prevent false prefix matches such as `Partition 1` matching `Partition 10` - add table-driven regression coverage for legacy, extended, and non-matching names ## Testing - `cargo fmt --check` - `cargo clippy --locked --all-targets --all-features --workspace -- -D warnings` - `cargo build` - `cargo test --locked -- --test-threads=1` - focused `test_dell` integration test - focused `test_dell_multi_dpu` integration test Fixes #101. Related: #93. This is a draft fallback for the existing fix; only one implementation should be merged. #93 will be closed. Signed-off-by: Josh P <williamp@nvidia.com>
Problem
Dell::set_boot_order_dpu_first(src/dell.rs) builds the expected boot option name as"HTTP Device 1: {DeviceDescription}"and compares it for exact equality against eachboot option's
display_name:On Dell iDRAC the real boot option name has an adapter/MAC/protocol suffix appended, e.g.:
So the exact
==never matches, the loop falls through toErr(RedfishError::MissingBootOption(..)), and the boot order is never set. Downstream(NICo / infra-controller) this wedges the host-provisioning state machine at
SetBootOrderindefinitely.
Fix
Match by prefix — the constructed expected name is already a prefix of the real
display_name:The reorder action itself (
PATCH Boot.BootOrder=[id]) is unchanged.Testing / verification
Reproduced and verified on a live BlueField-3 + Dell PowerEdge XE9680 (iDRAC):
set_boot_order_dpu_firstreturnedMissingBootOption("HTTP Device 1: NIC in Slot 40 Port 1 Partition 1")even though that boot option existed (Boot0000, with the suffix above) and was settable — confirmed by a manualPATCH .../Settings {"Boot":{"BootOrder":["Boot0000"]}}returning HTTP 200.SetBootOrder.🤖 Generated with Claude Code