refactor storepool automation #11789
Conversation
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #11789 +/- ##
=========================================
Coverage 16.17% 16.17%
Complexity 13296 13296
=========================================
Files 5656 5656
Lines 498219 498239 +20
Branches 60451 60446 -5
=========================================
+ Hits 80579 80584 +5
- Misses 408672 408684 +12
- Partials 8968 8971 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15281 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14519)
|
DaanHoogland
changed the title
|
[SF] Trillian test result (tid-14533)
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the StoragePoolAutomationImpl class by breaking down its two large methods (maintain and cancelMaintain) into smaller, more focused helper methods and consolidating duplicated code.
Key changes:
- Extracted multiple helper methods from the large
maintainandcancelMaintainmethods - Consolidated VM handling logic into reusable methods
- Removed unused imports and injected dependencies
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (!(storagePool.getParent().equals(pool.getParent()) || !pool.getParent().equals(storagePool.getId())) && | ||
| (storagePool.getStatus() == StoragePoolStatus.PrepareForMaintenance)) { | ||
| throw new CloudRuntimeException(String.format("Only one storage pool in a cluster can be in PrepareForMaintenance mode, %s is already in PrepareForMaintenance mode ", storagePool)); |
There was a problem hiding this comment.
The boolean logic in this condition is complex and hard to understand. Consider breaking it down into named boolean variables or simplifying the logic for better readability.
| if (!(storagePool.getParent().equals(pool.getParent()) || !pool.getParent().equals(storagePool.getId())) && | |
| (storagePool.getStatus() == StoragePoolStatus.PrepareForMaintenance)) { | |
| throw new CloudRuntimeException(String.format("Only one storage pool in a cluster can be in PrepareForMaintenance mode, %s is already in PrepareForMaintenance mode ", storagePool)); | |
| boolean sameParent = storagePool.getParent().equals(pool.getParent()); | |
| boolean poolIsParentOfStoragePool = pool.getParent().equals(storagePool.getId()); | |
| boolean isOtherPoolInMaintenance = storagePool.getStatus() == StoragePoolStatus.PrepareForMaintenance; | |
| // Only one storage pool in a cluster can be in PrepareForMaintenance mode | |
| if (!(sameParent || !poolIsParentOfStoragePool) && isOtherPoolInMaintenance) { | |
| throw new CloudRuntimeException(String.format("Only one storage pool in a cluster can be in PrepareForMaintenance mode, %s is already in PrepareForMaintenance mode ", storagePool)); |
Copilot uses AI. Check for mistakes.
| * // check if the vm has a root volume. If not, remove the item from the queue, the vm should be | ||
| * // started only when it has at least one root volume attached to it | ||
| * // don't allow to start vm that doesn't have a root volume |
There was a problem hiding this comment.
The JavaDoc comment is malformed. It should use proper JavaDoc format instead of starting each line with * //. These appear to be regular comments that should either be removed or converted to proper JavaDoc.
| * // check if the vm has a root volume. If not, remove the item from the queue, the vm should be | |
| * // started only when it has at least one root volume attached to it | |
| * // don't allow to start vm that doesn't have a root volume | |
| * Checks if the VM has a root volume. If not, removes the item from the queue. | |
| * The VM should be started only when it has at least one root volume attached to it. | |
| * Do not allow starting a VM that does not have a root volume. |
Copilot uses AI. Check for mistakes.
Description
This PR cleans up the StoragePoolAutomationImpl class to reduce its two big methods and reuse some code.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?