fix: re-assert the HTTP boot device during NIC-mode boot-order setup#2950
Merged
Conversation
Contributor
WalkthroughThe PR re-runs machine_setup before SetBootOrder boot-order reconfiguration, adds Redfish simulator state for HttpDev1 de-enumeration and recovery, extends machine-state tests for zero-DPU hosts, and adds a documented Site Explorer status gauge. ChangesSetBootOrder HttpDev1 re-assert fix
Site Explorer metric documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai PTAL, thanks! |
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
🧹 Nitpick comments (1)
crates/api-core/src/tests/machine_states.rs (1)
3009-3019: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the action order and selected boot NIC.
These tests only prove that
MachineSetupandSetBootOrderDpuFirsthappened at some point. They would still pass ifmachine_setupran after the reorder, or if it was called without the resolved interface, which is the main contract this PR is adding. Please assertMachineSetupprecedesSetBootOrderDpuFirstand that itsboot_interface_macmatches the zero-DPU boot NIC.Also applies to: 3045-3059
🤖 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/src/tests/machine_states.rs` around lines 3009 - 3019, The current assertions in the machine state tests only check that MachineSetup and SetBootOrderDpuFirst occurred, but not their order or the selected boot NIC. Update the relevant assertions in machine_states.rs around the SetBootOrderDpuFirst flow so they verify MachineSetup happens before SetBootOrderDpuFirst and that the MachineSetup payload’s boot_interface_mac matches the resolved zero-DPU boot NIC; use the existing RedfishSimAction matching in the test helpers to inspect both actions and their fields.
🤖 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/redfish/src/libredfish/test_support.rs`:
- Around line 69-99: The new http_dev1_enabled flag is incorrectly stored on the
shared RedfishSimState, causing one host’s de-enumeration or machine_setup to
affect all hosts. Move this state into RedfishSimHostState and update the
relevant reads/writes in RedfishSim and any helper methods to access it via
state.hosts[&self._host]. Keep the default behavior the same per host, but
ensure each simulated host maintains its own HttpDev1 enabled state.
---
Nitpick comments:
In `@crates/api-core/src/tests/machine_states.rs`:
- Around line 3009-3019: The current assertions in the machine state tests only
check that MachineSetup and SetBootOrderDpuFirst occurred, but not their order
or the selected boot NIC. Update the relevant assertions in machine_states.rs
around the SetBootOrderDpuFirst flow so they verify MachineSetup happens before
SetBootOrderDpuFirst and that the MachineSetup payload’s boot_interface_mac
matches the resolved zero-DPU boot NIC; use the existing RedfishSimAction
matching in the test helpers to inspect both actions and their fields.
🪄 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: b8520c86-d673-41ab-a313-332e58ecef87
📒 Files selected for processing (3)
crates/api-core/src/tests/machine_states.rscrates/machine-controller/src/handler.rscrates/redfish/src/libredfish/test_support.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
Author
|
@coderabbitai review Both addressed in
Thanks! |
Contributor
|
(•ᴗ•)🐇 ✅ Action performedReview finished.
|
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/redfish/src/libredfish/test_support.rs`:
- Around line 1264-1269: Keep boot-order readiness isolated per host:
`set_boot_order_setup` is writing a host-specific `http_dev1_enabled` value into
the shared `state.is_boot_order_setup`, but `is_boot_order_setup()` is still
read globally by all clients. Update the test support state so boot-order
readiness is stored and checked per host (using the existing host state around
`http_dev1_enabled` and `is_boot_order_setup()`), and make sure any caller in
`machine-controller` sees the readiness for the current host only. Add or adjust
tests in `test_support.rs` to cover two hosts toggling `HttpDev1` independently.
🪄 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: 33e333b5-0403-4d36-b171-4bc992d2e4ce
📒 Files selected for processing (4)
crates/api-core/src/tests/machine_states.rscrates/machine-controller/src/handler.rscrates/redfish/src/libredfish/test_support.rsdocs/observability/core_metrics.md
✅ Files skipped from review due to trivial changes (1)
- docs/observability/core_metrics.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/api-core/src/tests/machine_states.rs
- crates/machine-controller/src/handler.rs
`SetBootOrder` now re-asserts the HTTP boot device before reordering, so a NIC-mode host's boot setup stays solid even when the BlueField NIC briefly drops off Redfish across a reboot. The BIOS-setup phase already does this on recovery -- this gives `SetBootOrder` the same small touch, rounding out the zero-DPU boot-interface workflow. The motivating case: on a reboot the BlueField can de-enumerate and `HttpDev1` reverts to the onboard default. `set_boot_order_dpu_first` only reorders the boot options it finds, so the "HTTP Device 1" option would be gone and `is_boot_order_setup` could never pass on retry. Re-running `machine_setup` (by interface id) at the top of the SetBootOrder attempt re-enables it -- the existing `RebootHost` restart applies it in the same reboot, so no extra reboot, and it's a no-op when HttpDev1 is already set. While here, regenerates `docs/observability/core_metrics.md` -- the docs check is enforced now and it had drift from a recent site-explorer metric. Tests added! This supports NVIDIA#2947 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
nick-vetro
approved these changes
Jun 28, 2026
nick-vetro
approved these changes
Jun 28, 2026
10 tasks
nv-dmendoza
pushed a commit
that referenced
this pull request
Jul 23, 2026
…ingestion (#4012) > [!IMPORTANT] > This PR cherry-picks the host boot-interface configuration convergence arc into `release/v2.0`, in merge order: > - #3139 (`78aa3766a`) -- skip the SetBootOrder re-apply when the boot config is already set > - #3186 (`064ce21bc`) -- apply SetBootOrder boot config once per reboot > - #3194 (`428135d4f`) -- resolve the boot NIC through one shared step > - #3369 (`a2ab63f2c`) -- ensure a correct boot config during machine validation > - #3454 (`9d80c7fcd`) -- generalized convergence for host boot interface configuration ## Related issues This supports #2821 (nvbug 6430706). ## Type of Change - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [x] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated - [x] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) ## Additional Notes Backports the boot-interface convergence follow-ups that a v2.0 NIC-mode host needs to clear `PollingBiosSetup`/`BiosSetupFailed` (nvbug 6430706 / #2821). The base fix -- #2950, re-assert the HTTP boot device during NIC-mode -- is already in `release/v2.0`; this arc is the refinement set that lands the boot config by interface ID when the Dell NDF MAC goes empty, and converges it across host init, assigned setup, DPU reprovision, and validation. - **One conflict, in #3454** (`crates/machine-controller/Cargo.toml` `[dev-dependencies]`): resolved to `carbide-redfish` + `carbide-test-harness` + `carbide-test-support`, dropping `carbide-instrument` -- that crate does not exist on `release/v2.0` and nothing in the picked code references it. All other commits applied cleanly. - **libredfish:** no bump needed. The interface-ID boot resolution requires libredfish >= v0.44.16; `release/v2.0` already pins **v0.44.21** (`main` is at v0.44.22). - **#2896 intentionally excluded:** it was closed unmerged (a libredfish pin bump + test), superseded by #2950, which is already in v2.0. --------- Signed-off-by: Chet Nichols III <chetn@nvidia.com>
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.
SetBootOrdernow re-asserts the HTTP boot device before reordering, so a NIC-mode host's boot setup stays solid even when the BlueField NIC briefly drops off Redfish across a reboot. The BIOS-setup phase already does this on recovery -- this givesSetBootOrderthe same small touch, rounding out the zero-DPU boot-interface workflow.The motivating case: on a reboot the BlueField can de-enumerate and
HttpDev1reverts to the onboard default.set_boot_order_dpu_firstonly reorders the boot options it finds, so the "HTTP Device 1" option would be gone andis_boot_order_setupcould never pass on retry. Re-runningmachine_setup(by interface id) at the top of the SetBootOrder attempt re-enables it -- the existingRebootHostrestart applies it in the same reboot, so no extra reboot, and it's a no-op when HttpDev1 is already set.Tests added!
This supports #2947 and #2821.