Uh oh!
There was an error while loading. Please reload this page.
Deal with Storage Manager tech debt - #13598
Draft
DaanHoogland wants to merge 1 commit into
Draft
Conversation
DaanHoogland
commented
Jul 13, 2026
ContributorAuthor
@blueorangutan package |
blueorangutan
commented
Jul 13, 2026
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses tech debt in the storage management area (Fixes #12316) by removing unused code paths, tightening type usage, and consolidating configuration access via ConfigKey/ConfigurationManager while standardizing logging.
Changes:
- Refactors
StorageManagerImplto remove unused methods/fields, reduce raw types, and modernize logging and collection checks. - Moves several global configuration keys from
Configenum usage towardConfigurationManager/StorageManagerConfigKeys and updates call sites (including Swift/S3 drivers). - Cleans up API command/service signatures to align with the refactors (generics, removed obsolete checked exceptions, minor code simplifications).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/java/com/cloud/storage/StorageManagerImpl.java | Core refactor: remove unused code, generics cleanup, logging updates, config usage changes. |
| server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java | Exposes newly added ConfigurationManager config keys via getConfigKeys(). |
| server/src/main/java/com/cloud/configuration/Config.java | Removes several config enum entries migrated to ConfigKey locations. |
| plugins/storage/image/swift/src/main/java/org/apache/cloudstack/storage/datastore/driver/SwiftImageStoreDriverImpl.java | Uses ConfigurationManagerConfigKey for extract URL expiry; logging/generics cleanup. |
| plugins/storage/image/s3/src/main/java/org/apache/cloudstack/storage/datastore/driver/S3ImageStoreDriverImpl.java | Uses ConfigurationManagerConfigKey for extract URL expiry; logging cleanup. |
| engine/storage/cache/src/main/java/org/apache/cloudstack/storage/cache/manager/StorageCacheManagerImpl.java | Switches expunge worker count to ConfigurationManager.ExpungeWorkers. |
| engine/components-api/src/main/java/com/cloud/storage/StorageManager.java | Removes unused API surface; adds disk throttling ConfigKeys and minor boolean cleanup. |
| engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java | Introduces expunge/extract URL related ConfigKeys. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/swift/AddSwiftCmd.java | Simplifies execute flow after service signature change. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateStoragePoolCmd.java | Adjusts details map typing to reduce raw/unchecked usage. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/storage/CreateStoragePoolCmd.java | Formatting cleanup and removes obsolete exception handling. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddImageStoreS3CMD.java | Simplifies execute flow and generics; API command annotation adjusted. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddImageStoreCmd.java | Simplifies details extraction loop and execute flow. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/host/AddSecondaryStorageCmd.java | Simplifies execute flow after service signature change. |
| api/src/main/java/com/cloud/storage/StorageService.java | Tightens generics and removes obsolete checked exceptions from API surface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+57
to
+58
| @APICommand(name = "addImageStoreS3", description = "Adds S3 Image Store", responseObject = ImageStoreResponse.class, | ||
| since = "4.7.0", responseHasSensitiveInfo = false) |
Comment on lines
4477
to
4481
| if (logger.isDebugEnabled()) { | ||
| logger.debug("Failed to add object store: " + e.getMessage(), e); | ||
| 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); | ||
| } |
Comment on lines
4387
to
+4390
| DataStoreDownloadFollowRedirects, | ||
| AllowVolumeReSizeBeyondAllocation, | ||
| StoragePoolHostConnectWorkers, | ||
| ObjectStorageCapacityThreshold, | ||
| COPY_TEMPLATES_FROM_OTHER_SECONDARY_STORAGES | ||
| ObjectStorageCapacityThreshold |
Comment on lines
+244
to
+248
| ConfigKey<Integer> VmDiskThrottlingIopsReadRate = new ConfigKey<>( | ||
| Integer.class, | ||
| "vm.disk.throttling.iops_read_rate", | ||
| "Storage", | ||
| "0", |
Comment on lines
+253
to
+257
| ConfigKey<Integer> VmDiskThrottlingIopsWriteRate = new ConfigKey<>( | ||
| Integer.class, | ||
| "vm.disk.throttling.iops_write_rate", | ||
| "Storage", | ||
| "0", |
Comment on lines
+262
to
+266
| ConfigKey<Integer> VmDiskThrottlingBytesReadRate = new ConfigKey<>( | ||
| Integer.class, | ||
| "vm.disk.throttling.bytes_read_rate", | ||
| "Storage", | ||
| "0", |
| ConfigKey.CATEGORY_ADVANCED, | ||
| Integer.class, | ||
| "expunge.workers", | ||
| "10", |
| } | ||
| HypervisorType hypervisorType = HypervisorType.KVM; | ||
| HypervisorType hypervisorType; // defaults to HypervisorType.KVM |
| 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## main #13598 +/- ##
============================================
+ Coverage 19.47% 19.50% +0.02% - Complexity 19404 19440 +36
============================================
Files 6303 6303 Lines 569237 569111 -126 Branches 69781 69755 -26 ============================================
+ Hits 110870 111010 +140 + Misses 446234 445957 -277 - Partials 12133 12144 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
blueorangutan
commented
Jul 13, 2026
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18550 |
|
🔴 Test Coverage Grade: |
| Metric | Value |
|---|---|
| Line coverage | 24.31% |
| Branch coverage | 18.45% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
| throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage()); | ||
| ImageStore result = _storageService.discoverImageStore(null, getUrl(), "NFS", getZoneId(), getDetails()); | ||
| ImageStoreResponse storeResponse = null; | ||
| if (result != null ) { |
Collaborator
There was a problem hiding this comment.
@DaanHoogland can we invert the logic to reduce identation?
ContributorAuthor
There was a problem hiding this comment.
not my style, but I’ll have to revisit anyway because of conflicts :/ later..
| throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage()); | ||
| ImageStore result = _storageService.discoverImageStore(getName(), getUrl(), getProviderName(), getZoneId(), getDetails()); | ||
| ImageStoreResponse storeResponse; | ||
| if (result != null) { |
| throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage()); | ||
| ImageStore result = _storageService.discoverImageStore(null, null, "S3", null, dm); | ||
| ImageStoreResponse storeResponse; | ||
| if (result != null) { |
| throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage()); | ||
| ImageStore result = _storageService.discoverImageStore(null, getUrl(), "Swift", null, dm); | ||
| ImageStoreResponse storeResponse; | ||
| if (result != null) { |
DaanHoogland
marked this pull request as draft
July 20, 2026 11:27
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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?