You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "DaanHoogland (via GitHub)" <gi...@apache.org> on 2026/07/13 10:17:45 UTC
[PR] Deal with Storage Manager tech debt [cloudstack]
DaanHoogland opened a new pull request, #13598:
URL: https://github.com/apache/cloudstack/pull/13598
### Description
This PR...
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
Fixes: #12316
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] StorageManager tech debt: remove unused methods, fields and return values [cloudstack]
Posted by "DaanHoogland (via GitHub) via commits" <co...@cloudstack.apache.org>.
DaanHoogland commented on code in PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#discussion_r4493918472
##########
engine/components-api/src/main/java/com/cloud/storage/StorageManager.java:
##########
@@ -340,11 +332,9 @@ static Boolean getFullCloneConfiguration(Long storeId) {
boolean canHostPrepareStoragePoolAccess(Host host, StoragePool pool);
- boolean canDisconnectHostFromStoragePool(Host host, StoragePool pool);
-
Host getHost(long hostId);
- Host updateSecondaryStorage(long secStorageId, String newUrl);
+ void updateSecondaryStorage(long secStorageId, String newUrl);
Review Comment:
not used outside the module
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Deal with Storage Manager tech debt [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#issuecomment-5568123218
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13598) **Quality Gate failed**
Failed conditions
 [8.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13598&metric=new_coverage&view=list) (required ≥ 40%)
<!-- sqra:qg_revision=8d97dfd33254942ad84b2078430fe34346ee64bb9f44300730d0b22859f4a7d0 -->
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13598)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Deal with Storage Manager tech debt [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#issuecomment-6292516534
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Deal with Storage Manager tech debt [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#discussion_r3569847780
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddImageStoreS3CMD.java:
##########
@@ -48,15 +48,14 @@
import org.apache.cloudstack.api.response.ImageStoreResponse;
import com.cloud.exception.ConcurrentOperationException;
-import com.cloud.exception.DiscoveryException;
import com.cloud.exception.InsufficientCapacityException;
import com.cloud.exception.NetworkRuleConflictException;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.storage.ImageStore;
-@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)
Review Comment:
The command accepts `SECRET_KEY` but the `@APICommand` annotation no longer sets `requestHasSensitiveInfo = true`. This can cause request parameters (including the S3 secret key) to be logged or exposed where the framework expects sensitive requests to be flagged.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -4722,9 +4475,9 @@ public ObjectStore discoverObjectStore(String name, String url, Long size, Strin
store = lifeCycle.initialize(params);
} catch (Exception e) {
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);
}
Review Comment:
The thrown CloudRuntimeException message concatenates a literal "{}" placeholder with the exception message, producing confusing output (e.g. "Failed to add object store: {}<msg>"). Either concatenate normally or use proper formatting (but CloudRuntimeException does not format placeholders).
##########
engine/components-api/src/main/java/com/cloud/storage/StorageManager.java:
##########
@@ -241,6 +241,43 @@ public interface StorageManager extends StorageService {
"while adding a new Secondary Storage. If the copy operation fails, the system falls back to downloading the template from the source URL.",
true, ConfigKey.Scope.Zone, null);
+ ConfigKey<Integer> VmDiskThrottlingIopsReadRate = new ConfigKey<>(
+ Integer.class,
+ "vm.disk.throttling.iops_read_rate",
+ "Storage",
+ "0",
Review Comment:
These disk throttling config keys were historically categorized under "Advanced" (see db schema inserts), but this new ConfigKey definition uses category "Storage". This will move the setting to a different category in the UI/docs and can cause confusion/inconsistency with existing installs.
##########
engine/components-api/src/main/java/com/cloud/storage/StorageManager.java:
##########
@@ -241,6 +241,43 @@ public interface StorageManager extends StorageService {
"while adding a new Secondary Storage. If the copy operation fails, the system falls back to downloading the template from the source URL.",
true, ConfigKey.Scope.Zone, null);
+ ConfigKey<Integer> VmDiskThrottlingIopsReadRate = new ConfigKey<>(
+ Integer.class,
+ "vm.disk.throttling.iops_read_rate",
+ "Storage",
+ "0",
+ "Default disk I/O read rate in requests per second allowed in User vm's disk.",
+ true,
+ ConfigKey.Scope.Global,
+ null);
+ ConfigKey<Integer> VmDiskThrottlingIopsWriteRate = new ConfigKey<>(
+ Integer.class,
+ "vm.disk.throttling.iops_write_rate",
+ "Storage",
+ "0",
+ "Default disk I/O writerate in requests per second allowed in User vm's disk.",
+ true,
+ ConfigKey.Scope.Global,
+ null);
+ ConfigKey<Integer> VmDiskThrottlingBytesReadRate = new ConfigKey<>(
+ Integer.class,
+ "vm.disk.throttling.bytes_read_rate",
+ "Storage",
+ "0",
Review Comment:
These disk throttling config keys were historically categorized under "Advanced" (see db schema inserts), but this new ConfigKey definition uses category "Storage". This will move the setting to a different category in the UI/docs and can cause confusion/inconsistency with existing installs.
##########
engine/components-api/src/main/java/com/cloud/storage/StorageManager.java:
##########
@@ -241,6 +241,43 @@ public interface StorageManager extends StorageService {
"while adding a new Secondary Storage. If the copy operation fails, the system falls back to downloading the template from the source URL.",
true, ConfigKey.Scope.Zone, null);
+ ConfigKey<Integer> VmDiskThrottlingIopsReadRate = new ConfigKey<>(
+ Integer.class,
+ "vm.disk.throttling.iops_read_rate",
+ "Storage",
+ "0",
+ "Default disk I/O read rate in requests per second allowed in User vm's disk.",
+ true,
+ ConfigKey.Scope.Global,
+ null);
+ ConfigKey<Integer> VmDiskThrottlingIopsWriteRate = new ConfigKey<>(
+ Integer.class,
+ "vm.disk.throttling.iops_write_rate",
+ "Storage",
+ "0",
Review Comment:
These disk throttling config keys were historically categorized under "Advanced" (see db schema inserts), but this new ConfigKey definition uses category "Storage". This will move the setting to a different category in the UI/docs and can cause confusion/inconsistency with existing installs.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1002,7 +860,7 @@ public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws Resource
throw new InvalidParameterValueException("zone id can't be null, if scope is zone");
}
- HypervisorType hypervisorType = HypervisorType.KVM;
+ HypervisorType hypervisorType; // defaults to HypervisorType.KVM
Review Comment:
The comment says this variable "defaults to HypervisorType.KVM", but the code no longer initializes it and always assigns it from either the zone hypervisor parameter or the cluster hypervisor type. This comment is now misleading.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1692,7 +1528,7 @@ public boolean deletePool(DeletePoolCmd cmd) {
}
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
Review Comment:
Grammar: the comment is missing "of" ("deletion of any child storage pool").
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -4626,8 +4387,7 @@ public ConfigKey<?>[] getConfigKeys() {
DataStoreDownloadFollowRedirects,
AllowVolumeReSizeBeyondAllocation,
StoragePoolHostConnectWorkers,
- ObjectStorageCapacityThreshold,
- COPY_TEMPLATES_FROM_OTHER_SECONDARY_STORAGES
+ ObjectStorageCapacityThreshold
Review Comment:
`COPY_TEMPLATES_FROM_OTHER_SECONDARY_STORAGES` is still a StorageManager ConfigKey and is used (e.g. in ImageStoreDetailsUtil), but it was removed from StorageManagerImpl.getConfigKeys(). Since this class is Configurable, omitting the key can prevent it from being registered/seeded on new installations and make the feature configuration unavailable.
##########
engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java:
##########
@@ -71,6 +71,53 @@ public interface ConfigurationManager {
"Weight for CPU (as a value between 0 and 1) applied to compute capacity for Pods, Clusters and Hosts for COMBINED capacityType for ordering. Weight for RAM will be (1 - weight of CPU)",
true, ConfigKey.Scope.Global);
+ ConfigKey<Integer> ExpungeDelay = new ConfigKey<>(
+ ConfigKey.CATEGORY_ADVANCED,
+ Integer.class,
+ "expunge.delay",
+ "86400",
+ "Determines how long (in seconds) to wait before actually expunging destroyed vm. The default value = the default value of expunge.interval",
+ false,
+ ConfigKey.Scope.Global,
+ null);
+ ConfigKey<Integer> ExpungeInterval = new ConfigKey<>(
+ ConfigKey.CATEGORY_ADVANCED,
+ Integer.class,
+ "expunge.interval",
+ "86400",
+ "The interval (in seconds) to wait before running the expunge thread.",
+ false,
+ ConfigKey.Scope.Global,
+ null);
+ ConfigKey<Integer> ExpungeWorkers = new ConfigKey<>(
+ ConfigKey.CATEGORY_ADVANCED,
+ Integer.class,
+ "expunge.workers",
+ "10",
Review Comment:
`expunge.workers` default changed from 1 (previous Config enum default) to 10 here. That increases the default thread count on new installations and can have resource/DB impact. If this was not intentional, keep the historical default of 1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Deal with Storage Manager tech debt [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#issuecomment-4956966256
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/13598?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 3.41%. Comparing base ([`38b674f`](https://app.codecov.io/gh/apache/cloudstack/commit/38b674f4426fa8a9ab72ce485b1bdad62e2f1e4f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7864f0d`](https://app.codecov.io/gh/apache/cloudstack/commit/7864f0d5fbe942e925edf7a76955a3e25cfa2ef9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 8 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #13598 +/- ##
======================================
Coverage 3.41% 3.41%
======================================
Files 486 486
Lines 41774 41774
Branches 7899 7899
======================================
Hits 1428 1428
Misses 40148 40148
Partials 198 198
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/13598/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/13598/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `3.41% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/13598?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Deal with Storage Manager tech debt [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#issuecomment-4970440519
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Deal with Storage Manager tech debt [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on code in PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#discussion_r3573344390
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/host/AddSecondaryStorageCmd.java:
##########
@@ -89,20 +88,15 @@ public long getEntityOwnerId() {
@Override
public void execute(){
- try{
- ImageStore result = _storageService.discoverImageStore(null, getUrl(), "NFS", getZoneId(), getDetails());
- ImageStoreResponse storeResponse = null;
- if (result != null ) {
- storeResponse = _responseGenerator.createImageStoreResponse(result);
- storeResponse.setResponseName(getCommandName());
- storeResponse.setObjectName("secondarystorage");
- setResponseObject(storeResponse);
- } else {
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add secondary storage");
- }
- } catch (DiscoveryException ex) {
- logger.warn("Exception: ", ex);
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage());
+ ImageStore result = _storageService.discoverImageStore(null, getUrl(), "NFS", getZoneId(), getDetails());
+ ImageStoreResponse storeResponse = null;
+ if (result != null ) {
Review Comment:
@DaanHoogland can we invert the logic to reduce identation?
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddImageStoreCmd.java:
##########
@@ -123,20 +120,15 @@ public long getEntityOwnerId() {
@Override
public void execute(){
- try{
- ImageStore result = _storageService.discoverImageStore(getName(), getUrl(), getProviderName(), getZoneId(), getDetails());
- ImageStoreResponse storeResponse = null;
- if (result != null) {
- storeResponse = _responseGenerator.createImageStoreResponse(result);
- storeResponse.setResponseName(getCommandName());
- storeResponse.setObjectName("imagestore");
- setResponseObject(storeResponse);
- } else {
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add secondary storage");
- }
- } catch (DiscoveryException ex) {
- logger.warn("Exception: ", ex);
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage());
+ ImageStore result = _storageService.discoverImageStore(getName(), getUrl(), getProviderName(), getZoneId(), getDetails());
+ ImageStoreResponse storeResponse;
+ if (result != null) {
Review Comment:
same here
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/swift/AddSwiftCmd.java:
##########
@@ -83,25 +82,20 @@ public long getEntityOwnerId() {
@Override
public void execute() {
- Map<String, String> dm = new HashMap<String, String>();
+ Map<String, String> dm = new HashMap<>();
dm.put(ApiConstants.ACCOUNT, getAccount());
dm.put(ApiConstants.USERNAME, getUsername());
dm.put(ApiConstants.KEY, getKey());
- try{
- ImageStore result = _storageService.discoverImageStore(null, getUrl(), "Swift", null, dm);
- ImageStoreResponse storeResponse = null;
- if (result != null) {
- storeResponse = _responseGenerator.createImageStoreResponse(result);
- storeResponse.setResponseName(getCommandName());
- storeResponse.setObjectName("secondarystorage");
- setResponseObject(storeResponse);
- } else {
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add Swift secondary storage");
- }
- } catch (DiscoveryException ex) {
- logger.warn("Exception: ", ex);
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage());
+ ImageStore result = _storageService.discoverImageStore(null, getUrl(), "Swift", null, dm);
+ ImageStoreResponse storeResponse;
+ if (result != null) {
Review Comment:
same here
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddImageStoreS3CMD.java:
##########
@@ -127,20 +126,15 @@ public void execute() throws ResourceUnavailableException, InsufficientCapacityE
dm.put(ApiConstants.S3_USE_TCP_KEEPALIVE, getUseTCPKeepAlive().toString());
}
- try{
- ImageStore result = _storageService.discoverImageStore(null, null, "S3", null, dm);
- ImageStoreResponse storeResponse;
- if (result != null) {
- storeResponse = _responseGenerator.createImageStoreResponse(result);
- storeResponse.setResponseName(getCommandName());
- storeResponse.setObjectName("imagestore");
- setResponseObject(storeResponse);
- } else {
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add S3 Image Store.");
- }
- } catch (DiscoveryException ex) {
- logger.warn("Exception: ", ex);
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage());
+ ImageStore result = _storageService.discoverImageStore(null, null, "S3", null, dm);
+ ImageStoreResponse storeResponse;
+ if (result != null) {
Review Comment:
same here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Deal with Storage Manager tech debt [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#issuecomment-4957284823
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18550
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Deal with Storage Manager tech debt [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#issuecomment-4957407357
## 🔴 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](https://github.com/apache/cloudstack/actions/runs/29242252305)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Deal with Storage Manager tech debt [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#issuecomment-5568128065
<!-- coverage-grade-bot -->
## 🔴 Test Coverage Grade: `D` — Marginal
| Metric | Value |
|--------|-------|
| Line coverage | **24.68%** |
| Branch coverage | **18.90%** |
### 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](https://github.com/apache/cloudstack/actions/runs/34097678915)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Deal with Storage Manager tech debt [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#issuecomment-4956917590
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Deal with Storage Manager tech debt [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#issuecomment-4957404295
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13598) **Quality Gate failed**
Failed conditions
 [8.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13598&metric=new_coverage&view=list) (required ≥ 40%)
<!-- sqra:qg_revision=f7ea609aee26bacae798ed51a038be9ed5a6607e52f3fcd9f2878736222729a7 -->
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13598)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] StorageManager tech debt 1/5: remove unused methods, fields and return values [cloudstack]
Posted by "Copilot (via GitHub) via commits" <co...@cloudstack.apache.org>.
Copilot commented on code in PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#discussion_r4443839621
##########
engine/components-api/src/main/java/com/cloud/storage/StorageManager.java:
##########
@@ -340,11 +332,9 @@ static Boolean getFullCloneConfiguration(Long storeId) {
boolean canHostPrepareStoragePoolAccess(Host host, StoragePool pool);
- boolean canDisconnectHostFromStoragePool(Host host, StoragePool pool);
-
Host getHost(long hostId);
- Host updateSecondaryStorage(long secStorageId, String newUrl);
+ void updateSecondaryStorage(long secStorageId, String newUrl);
Review Comment:
Changing `updateSecondaryStorage` from returning `Host` to `void` is a binary-incompatible change for downstream consumers (return type is part of the method signature in the JVM). If you need to preserve compatibility, keep the `Host` return type (callers can still ignore it) or introduce a new method name (e.g., `void updateSecondaryStorageUrl(...)`) and deprecate the old method until the next major version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Deal with Storage Manager tech debt [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#discussion_r3613903960
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/host/AddSecondaryStorageCmd.java:
##########
@@ -89,20 +88,15 @@ public long getEntityOwnerId() {
@Override
public void execute(){
- try{
- ImageStore result = _storageService.discoverImageStore(null, getUrl(), "NFS", getZoneId(), getDetails());
- ImageStoreResponse storeResponse = null;
- if (result != null ) {
- storeResponse = _responseGenerator.createImageStoreResponse(result);
- storeResponse.setResponseName(getCommandName());
- storeResponse.setObjectName("secondarystorage");
- setResponseObject(storeResponse);
- } else {
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add secondary storage");
- }
- } catch (DiscoveryException ex) {
- logger.warn("Exception: ", ex);
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage());
+ ImageStore result = _storageService.discoverImageStore(null, getUrl(), "NFS", getZoneId(), getDetails());
+ ImageStoreResponse storeResponse = null;
+ if (result != null ) {
Review Comment:
not my style, but I’ll have to revisit anyway because of conflicts :/ later..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Deal with Storage Manager tech debt [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#issuecomment-4956900963
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] StorageManager tech debt 1/6: remove unused methods, fields and return values [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub) via commits" <co...@cloudstack.apache.org>.
sonarqubecloud[bot] commented on PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#issuecomment-6580930994
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13598) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13598&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13598&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13598&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13598&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13598&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13598)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] StorageManager tech debt 1/6: remove unused methods, fields and return values [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub) via commits" <co...@cloudstack.apache.org>.
sonarqubecloud[bot] commented on PR #13598:
URL: https://github.com/apache/cloudstack/pull/13598#issuecomment-6685247616
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13598) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13598&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13598&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13598&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13598&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13598&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13598)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org