Added an additional field PowerState in InventoryStatus #81
Conversation
…l logic to reconcile Machine PowerState changes into Inventory status Signed-off-by: Gaurav Mehta <gaurav.mehta@suse.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a new MachinePowerState field to the Inventory status to track out-of-band power state changes of the underlying host machine. When a host's power status changes outside of Harvester's control (out-of-band via seeder), the Harvester UI can now display the correct power state by reconciling it from the underlying Machine (rufio) status.
Key Changes:
- Added
MachinePowerStatefield to InventoryStatus to store the power state from the underlying Machine resource - Implemented
reconcileMachinePowerStatereconciler to sync Machine power state to Inventory status - Added comprehensive test coverage for power state synchronization with all possible states (On, Off, Unknown)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/api/v1alpha1/inventory_types.go | Added MachinePowerState field of type rufio.PowerState to InventoryStatus struct |
| pkg/controllers/inventory_controller.go | Implemented reconcileMachinePowerState function and added it to the reconciliation chain |
| pkg/controllers/inventory_controller_test.go | Added comprehensive test validating power state synchronization for all power states |
| chart/seeder-crd/templates/metal.harvesterhci.io_inventories.yaml | Updated CRD schema to include machinePowerState field in status section |
| pkg/data/data.go | Updated generated bindata with new CRD content and timestamps |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| b.Status.Power = expectedPowerState | ||
| return k8sClient.Status().Update(ctx, b) | ||
| }).ShouldNot(HaveOccurred()) |
There was a problem hiding this comment.
The Eventually call on line 449 is missing timeout and polling interval parameters. This is inconsistent with the Eventually call on line 465 and other Eventually calls in the test file (e.g., lines 420, 436, 465). Add timeout and polling parameters like ".ShouldNot(HaveOccurred(), "30s", "5s")" for consistency.
| }).ShouldNot(HaveOccurred()) | |
| }, "30s", "5s").ShouldNot(HaveOccurred()) |
| return nil | ||
| } | ||
|
|
||
| // reconcilleMachinePowerState will update the machine power state in inventory status |
There was a problem hiding this comment.
The function name in the comment has a typo. It says "reconcilleMachinePowerState" but should be "reconcileMachinePowerState" (with one 'l' in "reconcile").
| // reconcilleMachinePowerState will update the machine power state in inventory status | |
| // reconcileMachinePowerState will update the machine power state in inventory status |
| } | ||
| b := &rufio.Machine{} | ||
| err := r.Get(ctx, types.NamespacedName{Name: i.Name, Namespace: i.Namespace}, b) | ||
| if err != nil { |
There was a problem hiding this comment.
The error handling when fetching the Machine object should check for IsNotFound errors. If the Machine object doesn't exist (e.g., during deletion or before creation completes), the function will return an error and cause reconciliation to fail. Consider handling the NotFound case gracefully by returning nil, similar to how it's done in handleInventoryDeletion (lines 211-217).
| if err != nil { | |
| if err != nil { | |
| if apierrors.IsNotFound(err) { | |
| return nil | |
| } |
1 participant
Problem:
If an underlying host's power status is changed out of band from seeder, then the Harvester UI still shows node as offline.
Solution:
Add a new field MachinePowerState to the inventory status which is reconcile from the underlying Machine status.
Related Issue(s):
harvester/harvester#5780
Test plan:
Additional documentation or context