refactor(power-control): reuse loaded machines for endpoint resolution#4063
refactor(power-control): reuse loaded machines for endpoint resolution#4063kunzhao-nv wants to merge 3 commits into
Conversation
…esolve Power and firmware compute paths already load machines to classify rack-scale vs standalone; resolve BMC endpoints from that map instead of querying the machine table again. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
Add unit coverage for mixed rack/standalone partitions, unknown ids that do not abort the batch, power-option failures that skip dispatch, and resolve-from-machines BMC gaps. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
WalkthroughCompute-tray machine classification and endpoint resolution now reuse preloaded machine maps. Power-control and firmware handlers pass those maps through dispatch paths, while unresolved machines and credential or BMC gaps are reported per target. Unit tests cover partitioning and endpoint-resolution outcomes. ChangesCompute-tray dispatch reuse
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 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.
🧹 Nitpick comments (2)
crates/api-core/src/handlers/component_manager.rs (2)
1835-1895: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct test for
dispatch_compute_tray_power_control.This function is the key new integration point tying
machines_by_id,resolve_compute_tray_endpoints_from_machines, and theComputeTrayManagerbackend together, but the added tests only exercise its sub-pieces (prepare_dispatch_listsmirrors the classify/gate/partition step, andresolve_compute_tray_endpoints_from_machinesis tested in isolation). Nothing here exercises the full path with a mock backend (success, partial backend failure, and mixed resolved/unresolved input), which is exactly the kind of critical, newly-reused path this PR is meant to harden.A lightweight mock similar to
ReconciliationComputeTrayManager(used incrates/machine-controller/tests/integration/maintenance.rs) would let you assert theip_to_machine_idmapping and result assembly end-to-end.🤖 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/handlers/component_manager.rs` around lines 1835 - 1895, Add direct tests for dispatch_compute_tray_power_control using a lightweight mock ComputeTrayManager, covering successful dispatch, partial backend failures, and mixed resolved/unresolved machines. Assert that backend calls receive the resolved endpoints, results use the resolved ip_to_machine_id mapping, unresolved machines retain error results, and returned IPs match dispatched endpoints.
3708-3743: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest-double duplicates production decision logic — drift risk.
prepare_dispatch_listshand-mirrors the classify → power-option gate → partition sequence fromcomponent_power_control'sMachineIdsarm, including a hardcoded error string ("failed to update power option: precondition"). If that production logic changes (new branch, different ordering, different error text), this double won't automatically reflect it, and the tests could keep passing against stale logic while the real handler diverges.Consider extracting the real classify/gate/partition loop in
component_power_controlinto a standalone function that both production code and these tests call directly, rather than re-implementing it here.🤖 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/handlers/component_manager.rs` around lines 3708 - 3743, Extract the classify, power-option validation, and rack-scale/standalone partition logic from component_power_control’s MachineIds arm into a shared standalone helper. Update both production code and prepare_dispatch_lists to call that helper, preserving unknown-machine results, failed power-option handling, partition outputs, and existing error text without duplicating the decision sequence.
🤖 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/handlers/component_manager.rs`:
- Around line 1835-1895: Add direct tests for
dispatch_compute_tray_power_control using a lightweight mock ComputeTrayManager,
covering successful dispatch, partial backend failures, and mixed
resolved/unresolved machines. Assert that backend calls receive the resolved
endpoints, results use the resolved ip_to_machine_id mapping, unresolved
machines retain error results, and returned IPs match dispatched endpoints.
- Around line 3708-3743: Extract the classify, power-option validation, and
rack-scale/standalone partition logic from component_power_control’s MachineIds
arm into a shared standalone helper. Update both production code and
prepare_dispatch_lists to call that helper, preserving unknown-machine results,
failed power-option handling, partition outputs, and existing error text without
duplicating the decision sequence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f2dbcdcb-43b5-47f8-9877-20b3b8548649
📒 Files selected for processing (1)
crates/api-core/src/handlers/component_manager.rs
2 participants
Description:
Related issues
#3873
Type of Change
Breaking Changes
Testing