-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Deal with Storage Manager tech debt #12321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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 @@
## main #12321 +/- ##
==========================================
Coverage 17.51% 17.51%
Complexity 15585 15585
==========================================
Files 5914 5914
Lines 529867 529687 -180
Branches 64722 64681 -41
==========================================
- Hits 92782 92766 -16
+ Misses 426635 426474 -161
+ Partials 10450 10447 -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 16130 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
5844dcc to
7d7e5df
Compare
|
|
[SF] Trillian test result (tid-15045)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses technical debt in the Storage Manager by refactoring code, modernizing patterns, and removing unused components. The changes focus on improving maintainability without altering functional behavior.
Key Changes:
- Migrated configuration values from Config enum to ConfigKey pattern for better type safety and consistency
- Removed unused imports, fields, methods, and deprecated exception handling
- Modernized logging statements from String.format to SLF4J parameterized format
- Updated from commons-collections to commons-collections4 for improved API safety
- Simplified code with modern Java patterns (enhanced for loops, try-with-resources, etc.)
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/java/com/cloud/storage/StorageManagerImpl.java | Main cleanup: removed unused code, modernized logging, migrated to ConfigKey pattern, simplified collections usage |
| server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java | Added new ConfigKey registrations for expunge and extract URL configurations |
| server/src/main/java/com/cloud/configuration/Config.java | Removed deprecated enum entries now handled by ConfigKey |
| plugins/storage/image/swift/src/main/java/org/apache/cloudstack/storage/datastore/driver/SwiftImageStoreDriverImpl.java | Migrated from ConfigurationDao to ConfigurationManager.ConfigKey, simplified code |
| plugins/storage/image/s3/src/main/java/org/apache/cloudstack/storage/datastore/driver/S3ImageStoreDriverImpl.java | Migrated to ConfigKey pattern, modernized logging |
| engine/storage/cache/src/main/java/org/apache/cloudstack/storage/cache/manager/StorageCacheManagerImpl.java | Updated to use ConfigurationManager.ExpungeWorkers ConfigKey |
| engine/components-api/src/main/java/com/cloud/storage/StorageManager.java | Added ConfigKey definitions for VM disk throttling, removed deprecated methods, updated API signatures |
| engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java | Added ConfigKey definitions for expunge and URL extraction configurations |
| api/src/main/java/org/apache/cloudstack/api/command/admin/swift/AddSwiftCmd.java | Removed unnecessary DiscoveryException handling |
| api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateStoragePoolCmd.java | Changed getDetails() return type for better type safety |
| api/src/main/java/org/apache/cloudstack/api/command/admin/storage/CreateStoragePoolCmd.java | Cleaned up exception handling, improved parameter formatting |
| api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddImageStoreS3CMD.java | Simplified exception handling, removed unnecessary required=false |
| api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddImageStoreCmd.java | Modernized iteration pattern, removed DiscoveryException |
| api/src/main/java/org/apache/cloudstack/api/command/admin/host/AddSecondaryStorageCmd.java | Simplified exception handling |
| api/src/main/java/com/cloud/storage/StorageService.java | Updated method signatures to remove unnecessary exception declarations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (sPool.getPoolType() == StoragePoolType.DatastoreCluster) { | ||
| // FR41 yet to handle on failure of deletion of any of the child storage pool | ||
| // FR41 yet to handle on failure of deletion any child storage pool |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The grammar error "deletion any child storage pool" should be corrected to "deletion of any child storage pool".
| // FR41 yet to handle on failure of deletion any child storage pool | |
| // FR41 yet to handle on failure of deletion of any child storage pool |
| if (CollectionUtils.isNotEmpty(srcStorageAccessGroups) && CollectionUtils.isNotEmpty(destStorageAccessGroups)) { | ||
| List<String> intersection = new ArrayList<>(srcStorageAccessGroups); | ||
| intersection.retainAll(destStorageAccessGroups); | ||
|
|
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removed lines (intersection calculation) were computing the common storage access groups between source and destination pools. This logic appears to be missing in the updated code. The code now checks various conditions without computing the intersection, which was likely important for determining if migration is possible. This could be a logic error where necessary validation was removed.
| logger.debug("Failed to add object store: {}", e.getMessage(), e); | ||
| } | ||
| throw new CloudRuntimeException("Failed to add object store: " + e.getMessage(), e); | ||
| throw new CloudRuntimeException("Failed to add object store: {}" + e.getMessage(), e); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message format string contains an extra pair of curly braces. The message should be "Failed to add object store: {}" with the exception message as the second argument, but currently it has "Failed to add object store: {}" + e.getMessage(). This will result in string concatenation instead of proper formatting. Remove the string concatenation and use proper SLF4J formatting.
| throw new CloudRuntimeException("Failed to add object store: {}" + e.getMessage(), e); | |
| throw new CloudRuntimeException("Failed to add object store: " + e.getMessage(), e); |
|
|
||
| throw new CloudRuntimeException("Unable to create extraction URL"); | ||
| logger.debug("Swift temp-url: {}", swiftUrl); | ||
| return swiftUrl.toString(); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of the null check for swiftUrl could potentially cause a NullPointerException. The previous code checked if swiftUrl was not null before returning it. Now the code directly returns swiftUrl.toString(), which will throw NPE if SwiftUtil.generateTempUrl() returns null. Consider adding back the null check or ensuring that generateTempUrl() never returns null.
| return swiftUrl.toString(); | |
| return swiftUrl != null ? swiftUrl.toString() : null; |
| } | ||
|
|
||
| HypervisorType hypervisorType = HypervisorType.KVM; | ||
| HypervisorType hypervisorType; // defaults to HypervisorType.KVM |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on line 858 is misleading. The variable is now declared without initialization, so it doesn't "default" to HypervisorType.KVM. It will be assigned a value later in the conditional blocks starting at line 859. Consider removing this comment or updating it to accurately reflect that the hypervisorType will be assigned based on the scope.
| HypervisorType hypervisorType; // defaults to HypervisorType.KVM | |
| HypervisorType hypervisorType; // will be set based on the provided scope/hypervisor |
| } | ||
|
|
||
| public Map<String,String> getDetails() { | ||
| public Map<String,Object> getDetails() { |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type of getDetails() has changed from Map<String,String> to Map<String,Object>. This is a breaking API change that could affect callers expecting Map<String,String>. Verify that all callers of this method have been updated to handle the new return type, particularly the extractApiParamAsMap method in StorageManagerImpl which now correctly expects Map<String,Object>.
| ConfigKey<Integer> VmDiskThrottlingBytesWriteRate = new ConfigKey<>( | ||
| Integer.class, | ||
| "vm.disk.throttling.bytes_write_rate", | ||
| "Advanced", |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The category for this ConfigKey should be "Storage" to be consistent with the other VM disk throttling ConfigKeys defined above (lines 235-262). Currently, it's set to "Advanced" while the others use "Storage".
| "Advanced", | |
| "Storage", |
| details.put(PrimaryDataStoreLifeCycle.CAPACITY_BYTES, String.valueOf(updatedCapacityBytes)); | ||
| _storagePoolDao.updateCapacityBytes(id, updatedCapacityBytes); | ||
| } | ||
| if (updatedCapacityIops != null) { | ||
| details.put(PrimaryDataStoreLifeCycle.CAPACITY_IOPS, updatedCapacityIops != null ? String.valueOf(updatedCapacityIops) : null); | ||
| details.put(PrimaryDataStoreLifeCycle.CAPACITY_IOPS, String.valueOf(updatedCapacityIops)); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of null checks before String.valueOf() on lines 1134 and 1138 could result in the string "null" being stored if updatedCapacityBytes or updatedCapacityIops is null. The previous code explicitly checked for null before conversion. While the updated code only executes if these values are not null (due to the if checks on lines 1133 and 1137), the redundant null ternary operators were defensive programming. This change is acceptable but less defensive.
| * XenServer and ESXi that put virtual disks in clustered file systems), then the storage system will need to determine if it already has a copy of | ||
| * the template or if it will need to create one first before cloning the template to a new volume to be used for the new root disk (assuming the root | ||
| * disk is being deployed from a template). If the template doesn't already exists on the storage system, then you need to take into consideration space | ||
| * disk is being deployed from a template). If the template doesn't already exist on the storage system, then you need to take into consideration space |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The grammar error "doesn't already exists" should be corrected to "doesn't already exist" (singular form of the verb to match the singular subject "template").
| @APICommand(name = "addImageStoreS3", description = "Adds S3 Image Store", responseObject = ImageStoreResponse.class, since = "4.7.0", | ||
| requestHasSensitiveInfo = true, responseHasSensitiveInfo = false) | ||
| @APICommand(name = "addImageStoreS3", description = "Adds S3 Image Store", responseObject = ImageStoreResponse.class, | ||
| since = "4.7.0", responseHasSensitiveInfo = false) |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removed attribute "requestHasSensitiveInfo = true" from the APICommand annotation indicates that this command was previously marked as containing sensitive information (access keys, secret keys). Removing this flag could have security implications if there's any downstream processing that relies on this metadata to handle sensitive data appropriately. Verify that this removal is intentional and that sensitive parameter handling is maintained through other means.
| since = "4.7.0", responseHasSensitiveInfo = false) | |
| since = "4.7.0", requestHasSensitiveInfo = true, responseHasSensitiveInfo = false) |


Description
This PR...
Fixes: #12316
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?