Copilot commented on code in PR #13884:
URL: https://github.com/apache/cloudstack/pull/13884#discussion_r3911817989
##########
engine/components-api/src/main/java/com/cloud/storage/StorageManager.java:
##########
@@ -172,6 +172,22 @@ public interface StorageManager extends StorageService {
ConfigKey<Integer> MaxDataMigrationWaitTime = new ConfigKey<>("Advanced",
Integer.class, "max.data.migration.wait.time", "15",
"Maximum wait time (in minutes) for a data migration task before
spawning a new SSVM", false, ConfigKey.Scope.Global);
+
+ ConfigKey<Integer> VmDiskThrottlingIopsReadRate = new
ConfigKey<>("Advanced", Integer.class, "vm.disk.throttling.iops_read_rate", "0",
+ "Default disk I/O read rate in requests per second allowed in User
vm's disk.", true);
+
+ ConfigKey<Integer> VmDiskThrottlingIopsWriteRate = new
ConfigKey<>("Advanced", Integer.class, "vm.disk.throttling.iops_write_rate",
"0",
+ "Default disk I/O writerate in requests per second allowed in User
vm's disk.", true);
+
+ ConfigKey<Integer> VmDiskThrottlingBytesReadRate = new
ConfigKey<>("Advanced", Integer.class, "vm.disk.throttling.bytes_read_rate",
"0",
+ "Default disk I/O read rate in bytes per second allowed in User
vm's disk.", true);
+
+ ConfigKey<Integer> VmDiskThrottlingBytesWriteRate = new
ConfigKey<>("Advanced", Integer.class, "vm.disk.throttling.bytes_write_rate",
"0",
+ "Default disk I/O writerate in bytes per second allowed in User
vm's disk.", true);
Review Comment:
VmDiskThrottlingBytesReadRate/WriteRate are defined as Integer, but these
values represent bytes-per-second and can exceed 32-bit ranges on modern
systems (and call sites treat them as long). Using ConfigKey<Long> avoids
truncation/validation issues for higher throughput limits.
##########
plugins/hypervisors/baremetal/src/main/java/com/cloud/baremetal/manager/BaremetalManager.java:
##########
@@ -21,11 +21,30 @@
import com.cloud.utils.component.Manager;
import com.cloud.utils.component.PluggableService;
import org.apache.cloudstack.api.BaremetalProvisionDoneNotificationCmd;
+import org.apache.cloudstack.framework.config.ConfigKey;
public interface BaremetalManager extends Manager, PluggableService {
public static final String EchoSecurityGroupAgent =
"EchoSecurityGroupAgent";
public static final String ExternalBaremetalSystemUrl =
"ExternalBaremetalSystemUrl";
public static final String DO_PXE = "doPxe";
+ ConfigKey<String> BaremetalInternalStorageServer = new
ConfigKey<>("Advanced", String.class, "baremetal.internal.storage.server.ip",
null,
+ "the ip address of server that stores kickstart file, kernel,
initrd, ISO for advanced networking baremetal provisioning", true);
+
+ ConfigKey<Integer> BaremetalProvisionDoneNotificationTimeout = new
ConfigKey<>("Advanced", Integer.class,
"baremetal.provision.done.notification.timeout", "1800",
+ "the max time to wait before treating a baremetal provision as
failure if no provision done notification is not received, in secs", true);
+
+ ConfigKey<String> ExternalBaremetalResourceClassName = new
ConfigKey<>("Advanced", String.class, "external.baremetal.resource.classname",
null,
+ "class name for handling external baremetal resource", true);
+
+ ConfigKey<Boolean> EnableBaremetalSecurityGroupAgentEcho = new
ConfigKey<>("Advanced", Boolean.class,
"enable.baremetal.securitygroup.agent.echo", "false",
+ "After starting provision process, periodcially echo security
agent installed in the template. Treat provisioning as success only if echo
successfully", true);
+
+ ConfigKey<String> BaremetalIpmiLanInterface = new ConfigKey<>("Advanced",
String.class, "baremetal.ipmi.lan.interface", "default",
+ "option specified in -I option of impitool. candidates are:
open/bmc/lipmi/lan/lanplus/free/imb, see ipmitool man page for details. default
value 'default' means using default option of ipmitool", true);
+
+ ConfigKey<String> BaremetalIpmiRetryTimes = new ConfigKey<>("Advanced",
String.class, "baremetal.ipmi.fail.retry", "5",
+ "ipmi interface will be temporary out of order after power
operations(e.g. cycle, on), it leads following commands fail immediately. The
value specifies retry times before accounting it as real failure", true);
Review Comment:
BaremetalIpmiRetryTimes is used as an integer retry count but is declared as
ConfigKey<String>. Using ConfigKey<Integer> would provide type safety and avoid
manual parsing at call sites (and allow consistent validation).
##########
server/src/main/java/com/cloud/network/ExternalLoadBalancerDeviceManager.java:
##########
@@ -36,6 +38,9 @@ public interface ExternalLoadBalancerDeviceManager extends
Manager {
public static final int DEFAULT_LOAD_BALANCER_CAPACITY = 50;
+ ConfigKey<String> DefaultExternalLoadBalancerCapacity = new
ConfigKey<>("Advanced", String.class, "external.lb.default.capacity", "50",
+ "default number of networks permitted per external load balancer
device", true);
Review Comment:
DefaultExternalLoadBalancerCapacity represents a numeric capacity but is
declared as ConfigKey<String>, which weakens type validation and forces call
sites to parse/handle invalid values at runtime. Prefer a numeric ConfigKey
type (e.g., Integer/Long) and update usages to consume the typed value directly.
##########
server/src/main/java/com/cloud/network/ExternalFirewallDeviceManager.java:
##########
@@ -34,14 +36,16 @@
public interface ExternalFirewallDeviceManager extends Manager {
+ ConfigKey<String> DefaultExternalFirewallCapacity = new
ConfigKey<>("Advanced", String.class, "external.firewall.default.capacity",
"50",
+ "default number of networks permitted per external load firewall
device", true);
Review Comment:
DefaultExternalFirewallCapacity is a numeric capacity but is declared as
ConfigKey<String>, which bypasses ConfigKey's numeric typing/validation and
pushes parsing errors to runtime. Prefer a numeric ConfigKey type (e.g.,
Integer/Long) and adjust call sites accordingly.
##########
plugins/hypervisors/baremetal/src/main/java/com/cloud/baremetal/manager/BaremetalManager.java:
##########
@@ -21,11 +21,30 @@
import com.cloud.utils.component.Manager;
import com.cloud.utils.component.PluggableService;
import org.apache.cloudstack.api.BaremetalProvisionDoneNotificationCmd;
+import org.apache.cloudstack.framework.config.ConfigKey;
public interface BaremetalManager extends Manager, PluggableService {
public static final String EchoSecurityGroupAgent =
"EchoSecurityGroupAgent";
public static final String ExternalBaremetalSystemUrl =
"ExternalBaremetalSystemUrl";
public static final String DO_PXE = "doPxe";
+ ConfigKey<String> BaremetalInternalStorageServer = new
ConfigKey<>("Advanced", String.class, "baremetal.internal.storage.server.ip",
null,
+ "the ip address of server that stores kickstart file, kernel,
initrd, ISO for advanced networking baremetal provisioning", true);
+
+ ConfigKey<Integer> BaremetalProvisionDoneNotificationTimeout = new
ConfigKey<>("Advanced", Integer.class,
"baremetal.provision.done.notification.timeout", "1800",
+ "the max time to wait before treating a baremetal provision as
failure if no provision done notification is not received, in secs", true);
+
+ ConfigKey<String> ExternalBaremetalResourceClassName = new
ConfigKey<>("Advanced", String.class, "external.baremetal.resource.classname",
null,
+ "class name for handling external baremetal resource", true);
+
+ ConfigKey<Boolean> EnableBaremetalSecurityGroupAgentEcho = new
ConfigKey<>("Advanced", Boolean.class,
"enable.baremetal.securitygroup.agent.echo", "false",
+ "After starting provision process, periodcially echo security
agent installed in the template. Treat provisioning as success only if echo
successfully", true);
+
+ ConfigKey<String> BaremetalIpmiLanInterface = new ConfigKey<>("Advanced",
String.class, "baremetal.ipmi.lan.interface", "default",
+ "option specified in -I option of impitool. candidates are:
open/bmc/lipmi/lan/lanplus/free/imb, see ipmitool man page for details. default
value 'default' means using default option of ipmitool", true);
Review Comment:
Typo in config description: "impitool" should be "ipmitool".
##########
server/src/main/java/com/cloud/storage/secondary/SecondaryStorageVmManager.java:
##########
@@ -50,6 +50,41 @@ public interface SecondaryStorageVmManager extends Manager {
true, ConfigKey.Scope.Zone, null, "User Data for SSVMs",
null, ConfigKey.GROUP_SYSTEM_VMS,
ConfigKey.SUBGROUP_SEC_STORAGE_VM);
+ ConfigKey<Boolean> UseSecondaryStorageVm = new ConfigKey<>("Hidden",
Boolean.class, "secondary.storage.vm", "false",
+ "Deploys a VM per zone to manage secondary storage if true,
otherwise secondary storage is mounted on management server", true);
+
+ ConfigKey<String> MountParent = new ConfigKey<>("Advanced", String.class,
"mount.parent", "/var/cloudstack/mnt",
+ "The mount point on the Management Server for Secondary Storage.",
true);
+
+ ConfigKey<Boolean> SystemVMAutoReserveCapacity = new
ConfigKey<>("Advanced", Boolean.class, "system.vm.auto.reserve.capacity",
"true",
+ "Indicates whether or not to automatically reserver system VM
standby capacity.", true);
Review Comment:
Typo in config description: "reserver" should be "reserve".
--
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]
