shwstppr commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r1991041222
##########
api/src/main/java/org/apache/cloudstack/api/response/ObjectStoreResponse.java:
##########
@@ -17,34 +17,40 @@
package org.apache.cloudstack.api.response;
import com.cloud.serializer.Param;
+
+import org.apache.cloudstack.api.ApiConstants;
import org.apache.cloudstack.storage.object.ObjectStore;
import com.google.gson.annotations.SerializedName;
import org.apache.cloudstack.api.BaseResponseWithAnnotations;
import org.apache.cloudstack.api.EntityReference;
@EntityReference(value = ObjectStore.class)
public class ObjectStoreResponse extends BaseResponseWithAnnotations {
- @SerializedName("id")
+ @SerializedName(ApiConstants.ID)
@Param(description = "the ID of the object store")
private String id;
- @SerializedName("name")
+ @SerializedName(ApiConstants.NAME)
@Param(description = "the name of the object store")
private String name;
- @SerializedName("url")
+ @SerializedName(ApiConstants.URL)
@Param(description = "the url of the object store")
private String url;
- @SerializedName("providername")
- @Param(description = "the provider name of the object store")
+ @SerializedName(ApiConstants.PROVIDER)
+ @Param(description = "the name of the object store provider")
private String providerName;
- @SerializedName("storagetotal")
+ @SerializedName(ApiConstants.SIZE)
Review Comment:
This changes the response param itself. Do we need some kind of deprecation
instead?
Similarly for other changed params. Could this cause any issues with
automation/SDKs?
##########
api/src/main/java/org/apache/cloudstack/api/response/BackupResponse.java:
##########
@@ -35,6 +35,14 @@ public class BackupResponse extends BaseResponse {
@Param(description = "ID of the VM backup")
private String id;
+ @SerializedName(ApiConstants.NAME)
+ @Param(description = "name of the backup")
Review Comment:
Do we need to add since key here and other new response params?
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -2340,6 +2349,17 @@ public void doInTransactionWithoutResult(final
TransactionStatus status) throws
throw new CloudRuntimeException("Unable to destroy " +
vm);
} else {
if (expunge) {
+ if (vm.getBackupOfferingId() != null) {
Review Comment:
Possible to refactor to this a method?
##########
plugins/backup/networker/src/main/java/org/apache/cloudstack/backup/NetworkerBackupProvider.java:
##########
@@ -615,6 +611,7 @@ public Backup
createNewBackupEntryForRestorePoint(Backup.RestorePoint restorePoi
backup.setAccountId(vm.getAccountId());
backup.setDomainId(vm.getDomainId());
backup.setZoneId(vm.getDataCenterId());
+ backup.setName(vm.getHostName() + '-' + new
SimpleDateFormat("yyyy-MM-dd'T'HH:mmX").format(new Date()));
Review Comment:
This is a repeating code. Would it make sense to add method in
BackupProvider interface or some other common class to get the backup name?
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -3428,11 +3438,19 @@ protected List<CapacityVO>
listCapacitiesWithDetails(final Long zoneId, final Lo
for (final Long zId : dcList) {
// op_host_Capacity contains only allocated stats and the real
time
// stats are stored "in memory".
- // List secondary storage capacity only when the api is
invoked for the zone layer.
- if ((capacityType == null || capacityType ==
Capacity.CAPACITY_TYPE_SECONDARY_STORAGE) &&
- podId == null && clusterId == null &&
- StringUtils.isEmpty(t)) {
-
taggedCapacities.add(_storageMgr.getSecondaryStorageUsedStats(null, zId));
+ // List secondary and object storage capacities only when the
api is invoked for the zone layer.
+ if (podId == null && clusterId == null &&
StringUtils.isEmpty(t)) {
+ if (capacityType == null) {
Review Comment:
possible to move to a separate method?
##########
server/src/main/java/com/cloud/configuration/Config.java:
##########
@@ -134,6 +134,22 @@ public enum Config {
"0.75",
"Percentage (as a value between 0 and 1) of local storage
utilization above which alerts will be sent about low local storage available.",
null),
+ BackupStorageCapacityThreshold(
Review Comment:
Would it be better to define them as ConfigKey in the respective
manager/service?
##########
ui/src/views/compute/wizard/TemplateIsoSelection.vue:
##########
@@ -114,6 +114,7 @@ export default {
}
}
if (this.preFillContent.templateid) {
+ let i = 0
for (i < this.filterOpts.length; i++;) {
const filter = this.filterOpts[i]
Review Comment:
Maybe `for...of` would read better
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -208,6 +219,7 @@ private BackupVO createBackupObject(VirtualMachine vm,
String backupPath) {
backup.setAccountId(vm.getAccountId());
backup.setDomainId(vm.getDomainId());
backup.setZoneId(vm.getDataCenterId());
+ backup.setName(vm.getHostName() + '-' + new
SimpleDateFormat("yyyy-MM-dd'T'HH:mmX").format(new Date()));
Review Comment:
There is a DateUtil class. It can be used to format date if possible
##########
engine/schema/src/main/resources/META-INF/db/schema-42010to42100.sql:
##########
@@ -19,16 +19,34 @@
-- Schema upgrade from 4.20.1.0 to 4.21.0.0
--;
--- Add columns max_backup and backup_interval_type to backup table
-ALTER TABLE `cloud`.`backup_schedule` ADD COLUMN `max_backups` int(8) default
NULL COMMENT 'maximum number of backups to maintain';
-ALTER TABLE `cloud`.`backups` ADD COLUMN `backup_interval_type` int(5) COMMENT
'type of backup, e.g. manual, recurring - hourly, daily, weekly or monthly';
-
-- Add console_endpoint_creator_address column to cloud.console_session table
CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.console_session',
'console_endpoint_creator_address', 'VARCHAR(45)');
-- Add client_address column to cloud.console_session table
CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.console_session',
'client_address', 'VARCHAR(45)');
+-- Allow default roles to use quotaCreditsList
+INSERT INTO `cloud`.`role_permissions` (uuid, role_id, rule, permission,
sort_order)
+SELECT uuid(), role_id, 'quotaCreditsList', permission, sort_order
+FROM `cloud`.`role_permissions` rp
+WHERE rp.rule = 'quotaStatement'
+AND NOT EXISTS(SELECT 1 FROM cloud.role_permissions rp_ WHERE rp.role_id =
rp_.role_id AND rp_.rule = 'quotaCreditsList');
+
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.host', 'last_mgmt_server_id',
'bigint unsigned DEFAULT NULL COMMENT "last management server this host is
connected to" AFTER `mgmt_server_id`');
+
+-- Add column max_backup to backup_schedule table
+ALTER TABLE `cloud`.`backup_schedule` ADD COLUMN `max_backups` int(8) default
NULL COMMENT 'maximum number of backups to maintain';
+
+-- Add columns name, description and backup_interval_type to backup table
+ALTER TABLE `cloud`.`backups` ADD COLUMN `backup_interval_type` int(5) COMMENT
'type of backup, e.g. manual, recurring - hourly, daily, weekly or monthly';
+ALTER TABLE `cloud`.`backups` ADD COLUMN `name` varchar(255) NOT NULL COMMENT
'name of the backup';
+ALTER TABLE `cloud`.`backups` ADD COLUMN `vm_name` varchar(255) COMMENT 'name
of the vm for which backup is taken, only set for orphaned backups';
+ALTER TABLE `cloud`.`backups` ADD COLUMN `description` varchar(1024) COMMENT
'description for the backup';
Review Comment:
Maybe use `IDEMPOTENT_ADD_COLUMN` instead?
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -90,6 +94,19 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
@Inject
BackupManager backupManager;
+ private Host getUpHostInZone(Long zoneId) {
Review Comment:
Not sure but we may already have a similar method in ResourceManager or
somewhere to find a random Up host in zone
--
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-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
