Repository navigation
StorageManager tech debt: remove unused methods, fields and return values - #13598
Open
DaanHoogland wants to merge 1 commit into
Open
DaanHoogland wants to merge 1 commit into
DaanHoogland wants to merge 1 commit into
Conversation
boring-cyborg
Bot
added
component:api
component:secondary-storage
component:storage
labels
Contributor
Author
|
@blueorangutan package |
|
@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 ConfigurationManager ConfigKey for extract URL expiry; logging/generics cleanup. |
| plugins/storage/image/s3/src/main/java/org/apache/cloudstack/storage/datastore/driver/S3ImageStoreDriverImpl.java | Uses ConfigurationManager ConfigKey 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
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); | ||
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13598 +/- ##
============================================
- Coverage 20.78% 20.78% -0.01%
+ Complexity 21931 21927 -4
============================================
Files 6496 6496
Lines 591657 591531 -126
Branches 72592 72568 -24
============================================
- Hits 122966 122925 -41
+ Misses 455346 455269 -77
+ Partials 13345 13337 -8
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:
|
|
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. |
GaOrtiga
reviewed
Jul 14, 2026
DaanHoogland
marked this pull request as draft
July 20, 2026 11:27
DaanHoogland
moved this from Backlog
to conflict/waiting for author
in CloudStack Testing
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 25.89% |
| Branch coverage | 20.22% |
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. |
DaanHoogland
force-pushed
the
ghi12316-storageManagerTechDebt
branch
from
November 2, 2026 13:20
50ddab2 to
ea758d7
Compare
DaanHoogland
force-pushed
the
ghi12316-storageManagerTechDebt
branch
from
November 2, 2026 14:30
ea758d7 to
58eae14
Compare
DaanHoogland
changed the title
1 task done
This was referenced Nov 2, 2026
DaanHoogland
marked this pull request as ready for review
November 2, 2026 14:31
7 tasks done
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
7 open issues
updateSecondaryStoragefrom returningHosttovoidis a binary-incompatible change… · Newexpunge.workersdefault changed from 1 (previous Config enum default) to 10 here. That increases…COPY_TEMPLATES_FROM_OTHER_SECONDARY_STORAGESis still a StorageManager ConfigKey and is used…
3 resolved since last review
Review effort: Lite
1 task done
DaanHoogland
changed the title
Part of #12316 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DaanHoogland
force-pushed
the
ghi12316-storageManagerTechDebt
branch
from
November 6, 2026 11:36
58eae14 to
07a100a
Compare
|
DaanHoogland
changed the title
This branch has not been deployed
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 free
to 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
Removes code in
StorageManager/StorageManagerImplthat has no callers anywhere in the repo:share,chooseHostForStoragePool,isPoolAvailable,findAllSnapshotForVolume,findAllVolumeIdInSnapshotTable,getPrimaryStorageNameLabel,ListByDataCenterHypervisor,canDisconnectHostFromStoragePool,poolProvidesCustomStorageStats,getCustomStorageStats, and the_discoverersfield with its accessors_configServer,storageService,storageManager,_serviceOfferingDetailsDao)updateSecondaryStoragenow returnsvoid(its only caller ignored theHost)The Config→ConfigKey migration that an earlier version of this PR contained has been dropped, because it duplicated #14467.
Part of #12316
Types of changes
🤖 Generated with Claude Code