refactor: generalized convergence for host boot interface configuration#3454
Conversation
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe change centralizes BIOS and boot-order convergence, integrates it across lifecycle and validation state machines, adds persisted validation substates and metrics, and expands coverage for drift repair, migration, retry exhaustion, and recovery. ChangesHost boot configuration convergence
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant HostBootConfig
participant Redfish
participant BIOSJob
participant State
Controller->>HostBootConfig: inspect_host_boot_config
HostBootConfig->>Redfish: inspect BIOS and boot order
HostBootConfig-->>Controller: ConfigureBios, SetBootOrder, or Complete
Controller->>HostBootConfig: run_host_boot_config_stage
HostBootConfig->>BIOSJob: configure or advance BIOS job
HostBootConfig->>Redfish: poll BIOS and set boot order
HostBootConfig-->>Controller: return next outcome
Controller->>State: persist lifecycle or validation state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/api-core/src/tests/machine_states.rs (1)
3482-3523: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert validation-id preservation on the order-only repair path.
This test plants the validation state with a discarded
MachineValidationId::new()and then matches the resumed step with a wildcardRebootHost { .. }. The siblingtest_machine_validation_repairs_reverted_boot_configalready bindsvalidation_idand asserts*current_validation_id == validation_id, directly guarding the "preserve validation run identity" objective. As written, this order-only path would not catch a regression that loses or rewrites the id during boot-order repair. Capturing the id and threading the same equality check through the resume match closes that gap with minimal effort.As per path instructions: "Prefer findings about behavior, concurrency, resource lifetimes, and missing tests over style-only comments."
♻️ Proposed strengthening of the id assertion
+ let validation_id = MachineValidationId::new(); set_host_controller_state_stuck_in( &env, host_id, &ManagedHostState::Validation { validation_state: ValidationState::MachineValidation { machine_validation: MachineValidatingState::MachineValidating { context: "Discovery".to_string(), - id: MachineValidationId::new(), + id: validation_id, completed: 1, total: 1, is_enabled: true, }, }, }, 0, ) .await; @@ if matches!( host.current_state(), ManagedHostState::Validation { validation_state: ValidationState::MachineValidation { - machine_validation: MachineValidatingState::RebootHost { .. }, + machine_validation: MachineValidatingState::RebootHost { + validation_id: current_validation_id, + }, }, - } + } if *current_validation_id == validation_id ) {🤖 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 3482 - 3523, Strengthen the order-only repair test around the initial MachineValidation state by binding the generated MachineValidationId to a local variable, then update the resumed RebootHost match to capture its validation id and assert it equals the original value. Follow the existing assertion pattern in test_machine_validation_repairs_reverted_boot_config while preserving the current unlocked and resumed checks.Source: Path instructions
🤖 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/host_boot_config.rs`:
- Around line 82-83: Correct the documentation for the
HostBootConfigDecision::Complete variant by replacing “NICo-managed” with
“NIC-managed.” Do not alter the enum behavior or surrounding lifecycle logic.
---
Nitpick comments:
In `@crates/api-core/src/tests/machine_states.rs`:
- Around line 3482-3523: Strengthen the order-only repair test around the
initial MachineValidation state by binding the generated MachineValidationId to
a local variable, then update the resumed RebootHost match to capture its
validation id and assert it equals the original value. Follow the existing
assertion pattern in test_machine_validation_repairs_reverted_boot_config while
preserving the current unlocked and resumed checks.
🪄 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: 7a0ce5b7-ccfe-4c8f-bbb6-736c27a75d51
📒 Files selected for processing (8)
crates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/machine_states.rscrates/api-model/src/machine/mod.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/host_boot_config.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/io.rs
| /// BIOS and every NICo-managed boot-order setting are already correct. | ||
| Complete, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Documentation typo in shared-module comment. NICo-managed should read NIC-managed. As this comment documents the semantics of HostBootConfigDecision::Complete for a module consumed by four lifecycles, the correct term avoids ambiguity for future maintainers.
✏️ Proposed correction
- /// BIOS and every NICo-managed boot-order setting are already correct.
+ /// BIOS and every NIC-managed boot-order setting are already correct.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// BIOS and every NICo-managed boot-order setting are already correct. | |
| Complete, | |
| /// BIOS and every NIC-managed boot-order setting are already 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/machine-controller/src/handler/host_boot_config.rs` around lines 82 -
83, Correct the documentation for the HostBootConfigDecision::Complete variant
by replacing “NICo-managed” with “NIC-managed.” Do not alter the enum behavior
or surrounding lifecycle logic.
There was a problem hiding this comment.
NICo is the product/controller name here (NVIDIA Infra Controller), so “NICo-managed” means the boot-order settings managed by NICo. “NIC-managed” would instead imply that the network interface manages them, which changes the meaning. I’m leaving the comment as written, thanks!
There was a problem hiding this comment.
@chet, thank you for the clarification — that makes sense. "NICo" refers to the NVIDIA Infra Controller product itself, not the network interface card, so the original comment as written is accurate. I'll withdraw my suggestion.
✏️ Learnings added
Learnt from: chet
Repo: NVIDIA/infra-controller PR: 3454
File: crates/machine-controller/src/handler/host_boot_config.rs:82-83
Timestamp: 2026-07-13T22:35:06.977Z
Learning: In this repository (NVIDIA/infra-controller), "NICo" is a product/controller name abbreviation for "NVIDIA Infra Controller" and is used in code comments (e.g., crates/machine-controller/src/handler/host_boot_config.rs) to refer to boot-order settings managed by the controller itself. Do not flag "NICo" as a typo for "NIC" in code reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
chet
changed the title
chet
changed the title
chet
changed the title
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
|
@coderabbitai just checking that the last full_review follow-up surfaced nothing? thank you! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="NVIDIA/infra-controller"
pr=3454
echo "=== Reviews ==="
gh api "repos/$repo/pulls/$pr/reviews" --paginate | jq -r '.[] | select(.user.login | test("coderabbit"; "i")) | "\(.id) \(.submitted_at) \(.state) \(.body | .[0:80])"'
echo "=== Review comments (inline) ==="
gh api "repos/$repo/pulls/$pr/comments" --paginate | jq -r '.[] | select(.user.login | test("coderabbit"; "i")) | "\(.id) \(.created_at) \(.path):\(.line // .original_line)"'Length of output: 1018 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="NVIDIA/infra-controller"
pr=3454
echo "=== Second review full body ==="
gh api "repos/$repo/pulls/$pr/reviews/4689339886" | jq -r '.body'
echo "=== Inline comment body (second review) ==="
gh api "repos/$repo/pulls/$pr/comments/3574667172" | jq -r '.body'
echo "=== Inline comment body (first review) ==="
gh api "repos/$repo/pulls/$pr/comments/3574540490" | jq -r '.body'Length of output: 826 🏁 Script executed: #!/bin/bash
repo="NVIDIA/infra-controller"
pr=3454
echo "=== Second review full body ==="
gh api "repos/$repo/pulls/$pr/reviews/4689339886" | jq -r '.body // "EMPTY"'
echo "=== Second review comments list ==="
gh api "repos/$repo/pulls/$pr/comments" --paginate | jq -r '.[] | select(.pull_request_review_id==4689339886) | "\(.id) \(.path):\(.line) \n\(.body)\n---"'
echo "=== First review comments list ==="
gh api "repos/$repo/pulls/$pr/comments" --paginate | jq -r '.[] | select(.pull_request_review_id==4689193550) | "\(.id) \(.path):\(.line) \n\(.body)\n---"'Length of output: 3907
So the second pass is clean — nothing outstanding on my end. (=^･ω･^=) |
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/machine-controller/src/handler.rs (1)
11578-11583: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmit the retry details as structured tracing fields.
Interpolating these values makes logfmt filtering and aggregation harder.
Proposed change
tracing::info!( - "Boot order check timed out for {} after {} minutes, retrying SetBootOrder (retry {} of {})", - mh_snapshot.host_snapshot.id, - time_since_state_change.num_minutes(), - retry_count + 1, - max_retries + machine_id = %mh_snapshot.host_snapshot.id, + timeout_minutes = time_since_state_change.num_minutes(), + retry = retry_count + 1, + max_retries, + "Boot order check timed out; retrying SetBootOrder" );As per coding guidelines, tracing messages should pass common values as structured fields rather than interpolating them into strings. As per path instructions, use structured tracing fields.
🤖 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/machine-controller/src/handler.rs` around lines 11578 - 11583, Update the tracing::info! call in the Boot order timeout retry path to emit mh_snapshot.host_snapshot.id, time_since_state_change.num_minutes(), retry_count + 1, and max_retries as structured tracing fields rather than interpolating them into the message. Keep a concise static message describing the timeout and retry action.Sources: Coding guidelines, Path instructions
crates/api-core/src/tests/machine_states.rs (1)
3544-3561: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the persisted Dell BIOS job ID.
The simulator returns
JID_VALIDATION_BOOT_REPAIR, but this match verifies onlyvalidation_id. A regression that drops or replaces the BIOS job ID would pass. Destructure the persisted job field and compare it with the configured ID.As per path instructions, prioritize state-machine persistence behavior and missing test assertions.
🤖 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 3544 - 3561, Update the assertion matching ManagedHostState::Validation in the persisted host check to destructure the WaitingForBootBiosJob BIOS job ID field, and compare it against the configured JID_VALIDATION_BOOT_REPAIR value alongside validation_id. Preserve the existing validation-state and validation-ID checks.Source: Path instructions
crates/api-core/src/tests/dpu_reprovisioning.rs (1)
631-643: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the persisted Viking recovery transition instead of replacing it.
After verifying
ForceOff, the test overwritesHandleJobFailurewithCheckBootOrder. This bypasses the transition under test, so a regression leaving Viking hosts permanently parked would still pass. Drive controller iterations fromparked_recoveryto the safe terminal state before testing BIOS drift.As per path instructions, prioritize safe recovery from partial failures and missing behavioral 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/api-core/src/tests/dpu_reprovisioning.rs` around lines 631 - 643, Update the test setup around the persisted state transition so it exercises Viking recovery from parked_recovery through the controller iterations to the safe terminal state, rather than directly overwriting HandleJobFailure with CheckBootOrder. Preserve the existing ForceOff verification, then invoke the recovery path and persist its resulting state before testing BIOS drift, ensuring the transition under test is actually covered.Source: Path instructions
🤖 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.
Nitpick comments:
In `@crates/api-core/src/tests/dpu_reprovisioning.rs`:
- Around line 631-643: Update the test setup around the persisted state
transition so it exercises Viking recovery from parked_recovery through the
controller iterations to the safe terminal state, rather than directly
overwriting HandleJobFailure with CheckBootOrder. Preserve the existing ForceOff
verification, then invoke the recovery path and persist its resulting state
before testing BIOS drift, ensuring the transition under test is actually
covered.
In `@crates/api-core/src/tests/machine_states.rs`:
- Around line 3544-3561: Update the assertion matching
ManagedHostState::Validation in the persisted host check to destructure the
WaitingForBootBiosJob BIOS job ID field, and compare it against the configured
JID_VALIDATION_BOOT_REPAIR value alongside validation_id. Preserve the existing
validation-state and validation-ID checks.
In `@crates/machine-controller/src/handler.rs`:
- Around line 11578-11583: Update the tracing::info! call in the Boot order
timeout retry path to emit mh_snapshot.host_snapshot.id,
time_since_state_change.num_minutes(), retry_count + 1, and max_retries as
structured tracing fields rather than interpolating them into the message. Keep
a concise static message describing the timeout and retry action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2627172a-cfcd-4bb1-b39a-2e407b7cf0d7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/api-core/src/cfg/README.mdcrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/machine_states.rscrates/api-model/src/machine/mod.rscrates/machine-controller/Cargo.tomlcrates/machine-controller/src/config/controller.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/bios_config.rscrates/machine-controller/src/handler/host_boot_config.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/io.rs
Host boot configuration now runs through one controller path across host initialization, assigned platform setup, DPU reprovision, and validation. Each lifecycle keeps its own persisted state while sharing BIOS setup, vendor jobs, polling, boot order, and final verification. Primary callouts are: - `host_boot_config` selects the smallest repair and advances the shared flow without introducing a new serialized wrapper. - Order-only drift skips `machine_setup`, while late BIOS drift returns to the job-aware path and carries a bounded convergence budget. - Dell job IDs stay persisted through reboot and recovery; validation keeps its run identity and closes the run on terminal repair failure. - Viking and legacy persisted states keep their safe recovery behavior. Tests updated! This supports NVIDIA#3391 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
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/machine-controller/src/handler/host_boot_config.rs`:
- Around line 393-398: Update next_boot_config_retry_count to use then_some
instead of then with a closure, preserving the existing retry_count <
max_retries condition and retry_count + 1 result.
🪄 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: dc08acd7-effb-4023-a6e9-a1447f8d956d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/api-core/src/cfg/README.mdcrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/machine_states.rscrates/api-model/src/machine/mod.rscrates/machine-controller/Cargo.tomlcrates/machine-controller/src/config/controller.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/bios_config.rscrates/machine-controller/src/handler/host_boot_config.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/io.rs
2 participants
Host boot configuration now runs through one controller path across host initialization, assigned platform setup, DPU reprovision, and validation. Each lifecycle keeps its own persisted state while sharing BIOS setup, vendor jobs, polling, boot order, and final verification.
Primary callouts are:
host_boot_configselects the smallest repair and advances the shared flow without introducing a new serialized wrapper.machine_setup, while late BIOS drift returns to the job-aware path and carries a bounded convergence budget.Tests updated!
This supports #3391
Closes #3391