Skip to content

refactor: resolve the boot NIC through one shared step#3194

Merged
chet merged 1 commit into
NVIDIA:mainfrom
chet:gh-issue-3193
Jul 7, 2026
Merged

refactor: resolve the boot NIC through one shared step#3194
chet merged 1 commit into
NVIDIA:mainfrom
chet:gh-issue-3193

Conversation

@chet

@chet chet commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Four Redfish boot steps -- the three boot-order substates (SetBootOrder,
WaitForHttpBootDeviceApplied, CheckBootOrder) and host boot repair -- each
opened with the same fifteen-line block resolving the host's boot NIC, with
only the wait message differing. The resolution step now lives in one helper,
require_boot_interface, and each caller states just the activity it's
blocked on: a zero-DPU host still discovering its boot NIC maps to the
caller's wait outcome, and a host with no resolvable interface stays a hard
error.

  • One source of truth for the resolve-or-wait-or-fail choreography, so the
    boot steps can't drift apart as the state machine grows.
  • Behavior-preserving: every caller keeps its exact wait message (the helper
    templates "before "), and wait-vs-error semantics are unchanged.
  • Call sites shrink from fifteen lines to a handful each.

This supports #3193

Signed-off-by: Chet Nichols III chetn@nvidia.com

Closes #3193

@chet
chet requested a review from a team as a code owner July 7, 2026 05:39
@chet

chet commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full_review, thanks!

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

@chet, you got it — kicking off a full review of all changes now.

(=^・ω・^=)ノ

✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 09c1b6e1-5f61-4282-93a1-9e93fbaff43e

📥 Commits

Reviewing files that changed from the base of the PR and between eb644a2 and ece5760.

📒 Files selected for processing (1)
  • crates/machine-controller/src/handler.rs

Summary by CodeRabbit

  • Bug Fixes
    • Improved host boot-device handling so NIC discovery “wait” behavior is consistent across boot configuration checks and boot-order state transitions.
    • Missing boot interfaces now produce the same clear terminal error across affected flows.
  • Tests
    • Added unit coverage for boot-interface resolution outcomes (Ready, awaiting NIC, and Missing) to prevent regressions.

Walkthrough

Refactors boot-interface resolution in crates/machine-controller/src/handler.rs by introducing RequiredBootInterface and require_boot_interface, consolidating wait/error handling across check_host_boot_config and three set_host_boot_order state-machine arms.

Changes

Boot interface resolution refactor

Layer / File(s) Summary
RequiredBootInterface helper
crates/machine-controller/src/handler.rs
Adds the private RequiredBootInterface<W> enum, the require_boot_interface wrapper, and tests for Ready, AwaitingNic, and Missing mappings.
Boot interface call-site integration
crates/machine-controller/src/handler.rs
check_host_boot_config and the SetBootOrder, WaitForHttpBootDeviceApplied, and CheckBootOrder arms delegate to require_boot_interface, preserving their wait outcomes and treating Missing as terminal.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related issues

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the shared boot NIC resolution refactor across the boot flow.
Description check ✅ Passed The description matches the change set and accurately explains the shared helper refactor.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/machine-controller/src/handler.rs`:
- Around line 4930-4960: Add direct unit coverage for require_boot_interface in
handler.rs, since it wraps resolve_boot_interface and currently lacks focused
tests. Create cases for RequiredBootInterface::Ready,
RequiredBootInterface::Wait from BootInterfaceResolution::AwaitingNic (verify
the wait constructor is invoked with the expected message), and the Missing path
returning StateHandlerError::GenericError. Use the require_boot_interface and
RequiredBootInterface symbols to locate the code and assert the wrapper’s
mapping behavior independently of resolve_boot_interface.
🪄 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: 652ac3c8-8a61-42df-83da-97ce7f747806

📥 Commits

Reviewing files that changed from the base of the PR and between eb644a2 and d216271.

📒 Files selected for processing (1)
  • crates/machine-controller/src/handler.rs

Comment thread crates/machine-controller/src/handler.rs
Four Redfish boot steps -- the three boot-order substates (SetBootOrder,
WaitForHttpBootDeviceApplied, CheckBootOrder) and host boot repair -- each
opened with the same fifteen-line block resolving the host's boot NIC, with
only the wait message differing. The resolution step now lives in one helper,
`require_boot_interface`, and each caller states just the activity it's
blocked on: a zero-DPU host still discovering its boot NIC maps to the
caller's wait outcome, and a host with no resolvable interface stays a hard
error.

- One source of truth for the resolve-or-wait-or-fail choreography, so the
  boot steps can't drift apart as the state machine grows.
- Behavior-preserving: every caller keeps its exact wait message (the helper
  templates "before <activity>"), and wait-vs-error semantics are unchanged.
- Call sites shrink from fifteen lines to a handful each.

This supports NVIDIA#3193

Signed-off-by: Chet Nichols III <chetn@nvidia.com>
@chet

chet commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full_review, thanks!

@chet
chet force-pushed the gh-issue-3193 branch from d216271 to ece5760 Compare July 7, 2026 05:59
@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

@chet, on it — running a full review of all changes again.

✅ Action performed

Full review finished.

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

🔍 Container Scan Summary

Service Total Critical High Medium Low Other
boot-artifacts-aarch64 3 0 0 3 0 0
boot-artifacts-x86_64 3 0 0 3 0 0
forge-admin-cli-x86_64 270 5 25 89 7 144
machine-validation-runner 771 25 207 278 40 221
machine_validation 771 25 207 278 40 221
machine_validation-aarch64 771 25 207 278 40 221
nvmetal-carbide 771 25 207 278 40 221
TOTAL 3360 105 853 1207 167 1028

Per-CVE detail lives in the per-service grype-* artifacts (JSON + SARIF). Severity counts only — no CVE IDs published here.

@chet
chet merged commit 428135d into NVIDIA:main Jul 7, 2026
59 checks passed
@chet
chet deleted the gh-issue-3193 branch July 7, 2026 13:41
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extract a shared boot-NIC resolution step for the boot-order substates

2 participants