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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
