fix(machine-a-tron): invalidate stale boot actions - #4430
Conversation
|
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 selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughPower-change cleanup now removes stale machine-side actions from the queue while retaining BMC setup, BMC DHCP, and power-off cleanup actions. Machine DHCP retry state resets only when machine DHCP work is removed. Tests cover these rules. ChangesPower-change cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/machine-a-tron/src/machine_state_machine.rs (1)
1626-1627: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the power-change integration path.
This test calls the cleanup helper directly. It does not verify that
fsm_eventinvokes the helper for bothEvent::PowerCycleandEvent::PowerOff, or that the pending deadlines are cleared.Add focused table cases through the power-change entry point and assert the queue, retry state, and deadlines.
🤖 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-a-tron/src/machine_state_machine.rs` around lines 1626 - 1627, Add focused table-driven tests through the fsm_event power-change entry point rather than calling abandon_machine_actions_on_power_change directly. Cover both Event::PowerCycle and Event::PowerOff, asserting that actions are cleared, retry state is updated, and pending deadlines are cleared.
🤖 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/machine-a-tron/src/machine_state_machine.rs`:
- Around line 1626-1627: Add focused table-driven tests through the fsm_event
power-change entry point rather than calling
abandon_machine_actions_on_power_change directly. Cover both Event::PowerCycle
and Event::PowerOff, asserting that actions are cleared, retry state is updated,
and pending deadlines are cleared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1e4441b3-45d4-4ea2-9617-f99b43b754b5
📒 Files selected for processing (1)
crates/machine-a-tron/src/machine_state_machine.rs
Power changes could leave a retrying discovery action at the queue head, preventing cleanup and reboot timers from running. Discard actions tied to the previous boot, clear pending deadlines, and preserve BMC-scoped work. Exhaustive classification forces new action variants to declare their power-change behavior. Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
444e00b to
30d832b
Compare
A retrying Machine-a-Tron boot action can remain at the front of the action queue after a power change. This prevents the queued power-off cleanup and power-cycle timer from running. In the observed failure, a stale `InitialDiscoveryRequest` kept retrying while the BIOS job remained scheduled until the test timed out. This change invalidates work tied to the previous boot when the machine powers off or cycles. BMC-scoped setup, BMC DHCP, and existing power-off cleanup remain queued. Pending machine and agent deadlines are cleared. Action classification uses an exhaustive match so future `FsmAction`, `Timer`, or `BmcEvent` variants require an explicit power-change policy. ## Related issues Resolves NVIDIA#4429 ## 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 - [ ] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) Signed-off-by: Jay Zhu <jayzhu@nvidia.com> Signed-off-by: Alex Ball <aball@nvidia.com>
#4454) After #4430 fixed the stale Machine-a-Tron action queue, the rack integration test exposed a second ordering problem: the controller could enter the `On` phase before the BMC had actually applied `ForceOff`. So, this gives the observed power-off its own persisted `WaitingForPlatformPowerOff` phase. `ForceOff` is issued once, stale power readings stay parked there, and the existing idempotent `On` phase remains restart-safe if the process exits between the Redfish request and state commit. Tests added! ## Related issues - Closes #4446 - Docs follow-up: #4452 ## 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** No configuration or API changes are required. ## Testing - [x] Unit tests added/updated - [x] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) - `cargo test -p carbide-machine-controller --test integration dpu_init_power_cycle_waits_for_observed_host_power_off -- --nocapture` - `cargo test -p carbide-api-core tests::machine_history::test_machine_state_history --lib -- --nocapture` - `cargo make check-format-nightly` - `cargo make clippy` - `cargo make carbide-lints` ## Additional Notes - `WaitingForPlatformPowerOff` remains part of the existing `waitingforplatformpowercycle` metric so dashboards and label cardinality do not change. - The controller still issues one `ForceOff`, matching the existing behavior. A separate retry policy is outside this fix. - The architecture diagram update stays in the docs-only follow-up so tech-writer review does not block this controller change. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
A retrying Machine-a-Tron boot action can remain at the front of the action queue after a power change. This prevents the queued power-off cleanup and power-cycle timer from running. In the observed failure, a stale
InitialDiscoveryRequestkept retrying while the BIOS job remained scheduled until the test timed out.This change invalidates work tied to the previous boot when the machine powers off or cycles. BMC-scoped setup, BMC DHCP, and existing power-off cleanup remain queued. Pending machine and agent deadlines are cleared.
Action classification uses an exhaustive match so future
FsmAction,Timer, orBmcEventvariants require an explicit power-change policy.Related issues
Resolves #4429
Type of Change
Breaking Changes
Testing