StorageManager tech debt 1/6: remove unused methods, fields and return values by DaanHoogland · Pull Request #13598 · apache/cloudstack

DaanHoogland · 2026-07-13T10:17:42Z

Description

Stack for #12316 (StorageManager tech debt), one PR per item of the issue checklist, review only the top commit of each:

StorageManager tech debt 1/6: remove unused methods, fields and return values #13598: unused methods, fields and return values
never-thrown checked exceptions
raw types and unchecked casts
never-used parameters
remaining trivial warnings
deprecated SearchCriteria.addAnd usages

This PR (1/6) removes code in StorageManager/StorageManagerImpl that has no callers anywhere in the repo:

methods share, chooseHostForStoragePool, isPoolAvailable, findAllSnapshotForVolume, findAllVolumeIdInSnapshotTable, getPrimaryStorageNameLabel, ListByDataCenterHypervisor, canDisconnectHostFromStoragePool, poolProvidesCustomStorageStats, getCustomStorageStats, and the _discoverers field with its accessors
unused injected fields (_configServer, storageService, storageManager, _serviceOfferingDetailsDao)
updateSecondaryStorage now returns void (its only caller ignored the Host)

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

Cleanup (Code refactoring and cleanup, that may add test cases)

🤖 Generated with Claude Code

DaanHoogland · 2026-07-13T10:18:02Z

@blueorangutan package

blueorangutan · 2026-07-13T10:20:04Z

@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.

Copilot

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 StorageManagerImpl to remove unused methods/fields, reduce raw types, and modernize logging and collection checks.
Moves several global configuration keys from Config enum usage toward ConfigurationManager/StorageManager ConfigKeys 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 `ConfigKey`s and minor boolean cleanup.
engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java	Introduces expunge/extract URL related `ConfigKey`s.
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.

            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);
        }


                DataStoreDownloadFollowRedirects,
                AllowVolumeReSizeBeyondAllocation,
                StoragePoolHostConnectWorkers,
-                ObjectStorageCapacityThreshold,
-                COPY_TEMPLATES_FROM_OTHER_SECONDARY_STORAGES
+                ObjectStorageCapacityThreshold


+    ConfigKey<Integer> VmDiskThrottlingIopsReadRate = new ConfigKey<>(
+            Integer.class,
+            "vm.disk.throttling.iops_read_rate",
+            "Storage",
+            "0",


+    ConfigKey<Integer> VmDiskThrottlingIopsWriteRate = new ConfigKey<>(
+            Integer.class,
+            "vm.disk.throttling.iops_write_rate",
+            "Storage",
+            "0",


+    ConfigKey<Integer> VmDiskThrottlingBytesReadRate = new ConfigKey<>(
+            Integer.class,
+            "vm.disk.throttling.bytes_read_rate",
+            "Storage",
+            "0",


+            ConfigKey.CATEGORY_ADVANCED,
+            Integer.class,
+            "expunge.workers",
+            "10",


codecov · 2026-07-13T10:26:09Z

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 20.77%. Comparing base (d6390bd) to head (58eae14).

Files with missing lines	Patch %	Lines
...ain/java/com/cloud/storage/StorageManagerImpl.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##               main   #13598    +/-   ##
==========================================
  Coverage     20.76%   20.77%            
- Complexity    21903    21906     +3     
==========================================
  Files          6496     6496            
  Lines        591612   591486   -126     
  Branches      72585    72561    -24     
==========================================
+ Hits         122851   122861    +10     
+ Misses       455424   455288   -136     
  Partials      13337    13337

Flag	Coverage Δ
uitests	`3.88% <ø> (ø)`
unittests	`22.08% <0.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

blueorangutan · 2026-07-13T11:07:01Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18550

sonarqubecloud · 2026-07-13T11:22:04Z

Quality Gate failed

Failed conditions
8.5% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube Cloud

github-actions · 2026-07-13T11:22:28Z

🔴 Test Coverage Grade: `D` — Marginal

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

github-actions · 2026-07-14T14:39:33Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

GaOrtiga · 2026-07-13T18:56:14Z

-            throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage());
+        ImageStore result = _storageService.discoverImageStore(null, getUrl(), "NFS", getZoneId(), getDetails());
+        ImageStoreResponse storeResponse = null;
+        if (result != null ) {


@DaanHoogland can we invert the logic to reduce identation?

not my style, but I’ll have to revisit anyway because of conflicts :/ later..

GaOrtiga · 2026-07-13T18:56:39Z

-            throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage());
+        ImageStore result = _storageService.discoverImageStore(getName(), getUrl(), getProviderName(), getZoneId(), getDetails());
+        ImageStoreResponse storeResponse;
+        if (result != null) {


GaOrtiga · 2026-07-13T18:57:08Z

-            throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage());
+        ImageStore result = _storageService.discoverImageStore(null, null, "S3", null, dm);
+        ImageStoreResponse storeResponse;
+        if (result != null) {


GaOrtiga · 2026-07-13T18:57:21Z

-            throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage());
+        ImageStore result = _storageService.discoverImageStore(null, getUrl(), "Swift", null, dm);
+        ImageStoreResponse storeResponse;
+        if (result != null) {


sonarqubecloud · 2026-09-07T09:01:40Z

Quality Gate failed

Failed conditions
8.3% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube Cloud

github-actions · 2026-09-07T09:02:01Z

🔴 Test Coverage Grade: `D` — Marginal

Metric	Value
Line coverage	25.88%
Branch coverage	20.21%

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

github-actions · 2026-10-20T13:54:25Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

Part of #12316 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot

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

Changing updateSecondaryStorage from returning Host to void is a binary-incompatible change… · New
expunge.workers default changed from 1 (previous Config enum default) to 10 here. That increases…
COPY_TEMPLATES_FROM_OTHER_SECONDARY_STORAGES is still a StorageManager ConfigKey and is used…
The thrown CloudRuntimeException message concatenates a literal "{}" placeholder with the exception…
These disk throttling config keys were historically categorized under "Advanced" (see db schema…
These disk throttling config keys were historically categorized under "Advanced" (see db schema…
These disk throttling config keys were historically categorized under "Advanced" (see db schema…

3 resolved since last review

_{Review effort: Lite}

    Host getHost(long hostId);

-    Host updateSecondaryStorage(long secStorageId, String newUrl);
+    void updateSecondaryStorage(long secStorageId, String newUrl);


sonarqubecloud · 2026-11-02T15:24:07Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Copilot AI review requested due to automatic review settings July 13, 2026 10:17

boring-cyborg Bot added component:api component:secondary-storage component:storage labels Jul 13, 2026

Copilot started reviewing on behalf of DaanHoogland July 13, 2026 10:18

View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

github-actions Bot added the status:has-conflicts label Jul 14, 2026

GaOrtiga reviewed Jul 14, 2026

View reviewed changes

DaanHoogland marked this pull request as draft July 20, 2026 11:27

DaanHoogland self-assigned this Jul 20, 2026

DaanHoogland added this to CloudStack Testing Aug 31, 2026

github-project-automation Bot moved this to Backlog in CloudStack Testing Aug 31, 2026

DaanHoogland moved this from Backlog to conflict/waiting for author in CloudStack Testing Aug 31, 2026

github-actions Bot removed the status:has-conflicts label Sep 7, 2026

github-actions Bot added the status:has-conflicts label Oct 20, 2026

DaanHoogland force-pushed the ghi12316-storageManagerTechDebt branch from 50ddab2 to ea758d7 Compare November 2, 2026 13:20

github-actions Bot removed the status:has-conflicts label Nov 2, 2026

storage: remove unused methods, fields and return values

58eae14

Part of #12316 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

DaanHoogland force-pushed the ghi12316-storageManagerTechDebt branch from ea758d7 to 58eae14 Compare November 2, 2026 14:30

DaanHoogland changed the title ~~Deal with Storage Manager tech debt~~ StorageManager tech debt 1/5: remove unused methods, fields and return values Nov 2, 2026

DaanHoogland added this pull request to stack #14563 November 2, 2026 14:31

DaanHoogland marked this pull request as ready for review November 2, 2026 14:31

Copilot AI balanced review requested due to automatic review settings November 2, 2026 14:31

DaanHoogland linked an issue Nov 2, 2026 that may be closed by this pull request

deal with tech debt in StorageManager(Impl) #12316

Open

7 tasks done

Copilot AI reviewed Nov 2, 2026

View reviewed changes

Comment thread engine/components-api/src/main/java/com/cloud/storage/StorageManager.java

Host getHost(long hostId);

Host updateSecondaryStorage(long secStorageId, String newUrl);

void updateSecondaryStorage(long secStorageId, String newUrl);

Copilot started reviewing on behalf of DaanHoogland November 2, 2026 14:54

View session

DaanHoogland mentioned this pull request Nov 2, 2026

StorageManager tech debt 6/6: deprecated SearchCriteria.addAnd #14564

Open

1 task done

DaanHoogland changed the title ~~StorageManager tech debt 1/5: remove unused methods, fields and return values~~ StorageManager tech debt 1/6: remove unused methods, fields and return values Nov 2, 2026

Conversation

DaanHoogland commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Uh oh!

DaanHoogland commented Jul 13, 2026

Uh oh!

blueorangutan commented Jul 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Jul 13, 2026

Uh oh!

sonarqubecloud Bot commented Jul 13, 2026

Quality Gate failed

Uh oh!

github-actions Bot commented Jul 13, 2026

🔴 Test Coverage Grade: D — Marginal

Grade Scale

Uh oh!

github-actions Bot commented Jul 14, 2026

Uh oh!

GaOrtiga Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

GaOrtiga Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

GaOrtiga Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

GaOrtiga Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Sep 7, 2026

Quality Gate failed

Uh oh!

github-actions Bot commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔴 Test Coverage Grade: D — Marginal

Grade Scale

Uh oh!

github-actions Bot commented Oct 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

sonarqubecloud Bot commented Nov 2, 2026

Quality Gate passed

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DaanHoogland commented Jul 13, 2026 •

edited

Loading

codecov Bot commented Jul 13, 2026 •

edited

Loading

🔴 Test Coverage Grade: `D` — Marginal

github-actions Bot commented Sep 7, 2026 •

edited

Loading

🔴 Test Coverage Grade: `D` — Marginal