Addressed last set of review comments from Wei.

abh1sar · abh1sar · commit eb4b132c09e9 · 2025-04-10T16:42:28.000+05:30
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java
@@ -151,8 +151,8 @@ public class DeployVMCmd extends BaseAsyncCreateCustomIdCmd implements SecurityG
     @Parameter(name = ApiConstants.DATADISKS_DETAILS,
             type = CommandType.MAP,
             since = "4.21.0",
-            description = "Disk offering details for creating multiple data volumes. Mutually exclusibe with diskOfferingId." +
-                    " Example: datadisksdetails[0].diskofferingid=1&datadisksdetails[0].size=10&datadisksdetails[0].miniops=100&datadisksdetails[0].maxiops=200")
+            description = "Disk offering details for creating multiple data volumes. Mutually exclusive with diskOfferingId." +
+                    " Example: datadisksdetails[0].diskofferingid=a2a73a84-19db-4852-8930-dfddef053341&datadisksdetails[0].size=10&datadisksdetails[0].miniops=100&datadisksdetails[0].maxiops=200")
     private Map dataDisksDetails;
 
     @Parameter(name = ApiConstants.GROUP, type = CommandType.STRING, description = "an optional group for the virtual machine")
diff --git a/api/src/main/java/org/apache/cloudstack/api/response/ObjectStoreResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/ObjectStoreResponse.java
@@ -47,7 +47,7 @@ public class ObjectStoreResponse extends BaseResponseWithAnnotations {
     private Long storageTotal;
 
     @SerializedName("storageallocated")
-    @Param(description = "the object store currently allocated size")
+    @Param(description = "the allocated size of the object store")
     private Long storageAllocated;
 
     @SerializedName("storageused")
diff --git a/engine/components-api/src/main/java/com/cloud/storage/StorageManager.java b/engine/components-api/src/main/java/com/cloud/storage/StorageManager.java
@@ -221,7 +221,7 @@ public interface StorageManager extends StorageService {
             "Number of worker threads to be used to connect hosts to a primary storage", true);
 
     ConfigKey<Float> ObjectStorageCapacityThreshold = new ConfigKey<>("Alert", Float.class,
-            "zone.objectStorage.capacity.notificationthreshold",
+            "objectStorage.capacity.notificationthreshold",
             "0.75",
             "Percentage (as a value between 0 and 1) of object storage utilization above which alerts will be sent about low storage available.",
             true,
diff --git a/engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDao.java b/engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDao.java
@@ -187,5 +187,5 @@ List<VMInstanceVO> searchRemovedByRemoveDate(final Date startDate, final Date en
 
     Map<String, Long> getNameIdMapForVmIds(Collection<Long> ids);
 
-    List<VMInstanceVO> listByIds(List<Long> ids);
+    List<VMInstanceVO> listByIdsIncludingRemoved(List<Long> ids);
 }
diff --git a/engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java b/engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java
@@ -1226,7 +1226,7 @@ public Map<String, Long> getNameIdMapForVmIds(Collection<Long> ids) {
     }
 
     @Override
-    public List<VMInstanceVO> listByIds(List<Long> ids) {
+    public List<VMInstanceVO> listByIdsIncludingRemoved(List<Long> ids) {
         SearchBuilder<VMInstanceVO> idsSearch = createSearchBuilder();
         idsSearch.and("ids", idsSearch.entity().getId(), SearchCriteria.Op.IN);
         idsSearch.done();
diff --git a/engine/schema/src/main/java/org/apache/cloudstack/backup/BackupDetailVO.java b/engine/schema/src/main/java/org/apache/cloudstack/backup/BackupDetailVO.java
@@ -39,7 +39,7 @@ public class BackupDetailVO implements ResourceDetail {
     @Column(name = "name")
     private String name;
 
-    @Column(name = "value", length = 1024)
+    @Column(name = "value", length = 65536)
     private String value;
 
     @Column(name = "display")
@@ -48,8 +48,8 @@ public class BackupDetailVO implements ResourceDetail {
     public BackupDetailVO() {
     }
 
-    public BackupDetailVO(long templateId, String name, String value, boolean display) {
-        this.resourceId = templateId;
+    public BackupDetailVO(long backupId, String name, String value, boolean display) {
+        this.resourceId = backupId;
         this.name = name;
         this.value = value;
         this.display = display;
diff --git a/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupDaoImpl.java b/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupDaoImpl.java
@@ -210,7 +210,7 @@ public BackupVO persist(BackupVO backup) {
     public boolean update(Long id, BackupVO backup) {
         return Transaction.execute((TransactionCallback<Boolean>) status -> {
             boolean result = super.update(id, backup);
-            if (result == true) {
+            if (result) {
                 saveDetails(backup);
             }
             return result;
@@ -295,7 +295,7 @@ public BackupResponse newBackupResponse(Backup backup, Boolean listVmDetails) {
         if (vm.getState() != VirtualMachine.State.Expunging) {
             response.setVmId(vm.getUuid());
         }
-        if (vm.getBackupOfferingId() != backup.getBackupOfferingId()) {
+        if (vm.getBackupOfferingId() == null || vm.getBackupOfferingId() != backup.getBackupOfferingId()) {
             response.setVmOfferingRemoved(true);
         }
         response.setExternalId(backup.getExternalId());
diff --git a/engine/schema/src/main/resources/META-INF/db/schema-42010to42100.sql b/engine/schema/src/main/resources/META-INF/db/schema-42010to42100.sql
@@ -35,7 +35,7 @@ AND NOT EXISTS(SELECT 1 FROM cloud.role_permissions rp_ WHERE rp.role_id = rp_.r
 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';
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backup_schedule', 'max_backups', 'int(8) default NULL COMMENT "maximum number of backups to maintain"');
 
 -- Add columns name, description and backup_interval_type to backup table
 CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'name', 'VARCHAR(255) NOT NULL COMMENT "name of the backup"');
@@ -50,7 +50,7 @@ CREATE TABLE `cloud`.`backup_details` (
   `id` bigint unsigned NOT NULL auto_increment,
   `backup_id` bigint unsigned NOT NULL COMMENT 'backup id',
   `name` varchar(255) NOT NULL,
-  `value` varchar(1024) NOT NULL,
+  `value` varchar(65536) NOT NULL,
   `display` tinyint(1) NOT NULL DEFAULT 1 COMMENT 'Should detail be displayed to the end user',
   PRIMARY KEY (`id`),
   CONSTRAINT `fk_backup_details__backup_id` FOREIGN KEY `fk_backup_details__backup_id`(`backup_id`) REFERENCES `backups`(`id`) ON DELETE CASCADE
diff --git a/plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java b/plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java
@@ -27,8 +27,6 @@
 import com.cloud.storage.dao.VolumeDao;
 
 import org.apache.cloudstack.backup.dao.BackupDao;
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 
 import com.cloud.utils.Pair;
 import com.cloud.utils.component.AdapterBase;
@@ -37,8 +35,6 @@
 import com.cloud.vm.VirtualMachine;
 
 public class DummyBackupProvider extends AdapterBase implements BackupProvider {
-    private static final Logger LOG = LogManager.getLogger(DummyBackupProvider.class);
-
     @Inject
     private BackupDao backupDao;
     @Inject
diff --git a/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java b/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java
@@ -716,7 +716,10 @@ protected Map<String, Backup.Metric> processHttpResponseForBackupMetrics(final I
                 throw new CloudRuntimeException("Could not get backup metrics via Veeam B&R API");
             }
             for (final BackupFile backupFile : backupFiles.getBackupFiles()) {
-                String backupFileId = backupFile.getUid().substring(backupFile.getUid().lastIndexOf(':') + 1);
+                String backupFileId = StringUtils.substringAfterLast(backupFile.getUid(), ":");
+                if (backupFileId.isEmpty()) {
+                    continue;
+                }
                 Long backupSize = null;
                 Long dataSize = null;
                 if (backupFile.getBackupSize() != null) {
@@ -795,7 +798,13 @@ private Backup.RestorePoint getRestorePointFromBlock(String[] parts, Map<String,
             }
         }
         Backup.Metric metric = metricsMap.get(id);
-        return new Backup.RestorePoint(id, created, type, metric.getBackupSize(), metric.getDataSize());
+        Long backupSize = null;
+        Long dataSize = null;
+        if (metric != null) {
+            backupSize = metric.getBackupSize();
+            dataSize = metric.getDataSize();
+        }
+        return new Backup.RestorePoint(id, created, type, backupSize, dataSize);
     }
 
     public List<Backup.RestorePoint> listRestorePointsLegacy(String backupName, String vmInternalName, Map<String, Backup.Metric> metricsMap) {
@@ -849,7 +858,7 @@ public List<Backup.RestorePoint> processHttpResponseForVmRestorePoints(InputStre
             final ObjectMapper objectMapper = new XmlMapper();
             final VmRestorePoints vmRestorePoints = objectMapper.readValue(content, VmRestorePoints.class);
             final String hierarchyId = findDCHierarchy(vmwareDcName);
-            final String hierarchyUuid = hierarchyId.substring(hierarchyId.lastIndexOf(":") + 1);
+            final String hierarchyUuid = StringUtils.substringAfterLast(hierarchyId, ":");
             if (vmRestorePoints == null) {
                 throw new CloudRuntimeException("Could not get VM restore points via Veeam B&R API");
             }
@@ -860,7 +869,7 @@ public List<Backup.RestorePoint> processHttpResponseForVmRestorePoints(InputStre
                     continue;
                 }
                 boolean isReady = true;
-                String backupFileId = null;
+                String backupFileId = "";
                 List<Link> links = vmRestorePoint.getLink();
                 for (Link link : links) {
                     if (Arrays.asList(BACKUP_FILE_REFERENCE, RESTORE_POINT_REFERENCE).contains(link.getType()) && !link.getRel().equals("Up")) {
@@ -869,19 +878,19 @@ public List<Backup.RestorePoint> processHttpResponseForVmRestorePoints(InputStre
                         break;
                     }
                     if (link.getType() != null && link.getType().equals(BACKUP_FILE_REFERENCE)) {
-                        backupFileId = link.getHref().substring(link.getHref().lastIndexOf('/') + 1);
+                        backupFileId = StringUtils.substringAfterLast(link.getHref(), "/");
                     }
                 }
                 if (!isReady) {
                     continue;
                 }
-                String vmRestorePointId = vmRestorePoint.getUid().substring(vmRestorePoint.getUid().lastIndexOf(':') + 1);
+                String vmRestorePointId = StringUtils.substringAfterLast(vmRestorePoint.getUid(), ":");
                 Date created = formatDate(vmRestorePoint.getCreationTimeUtc());
                 String type = vmRestorePoint.getPointType();
                 logger.debug(String.format("Adding restore point %s, %s, %s", vmRestorePointId, created, type));
                 Long backupSize = null;
                 Long dataSize = null;
-                if (backupFileId != null) {
+                if (!backupFileId.isEmpty()) {
                     Backup.Metric metric = metricsMap.get(backupFileId);
                     if (metric != null) {
                         backupSize = metric.getBackupSize();
diff --git a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java
@@ -1687,7 +1687,7 @@ private void syncOutOfBandBackups(final BackupProvider backupProvider, DataCente
 
         private void updateBackupUsageRecords(final BackupProvider backupProvider, DataCenter dataCenter) {
             List<Long> vmIdsWithBackups = backupDao.listVmIdsWithBackupsInZone(dataCenter.getId());
-            List<VMInstanceVO> vmsWithBackups = vmInstanceDao.listByIds(vmIdsWithBackups);
+            List<VMInstanceVO> vmsWithBackups = vmInstanceDao.listByIdsIncludingRemoved(vmIdsWithBackups);
             List<VMInstanceVO> vmsWithBackupOffering = vmInstanceDao.listByZoneAndBackupOffering(dataCenter.getId(), null); //should return including removed
             Set<VMInstanceVO> vms =  Stream.concat(vmsWithBackups.stream(), vmsWithBackupOffering.stream()) .collect(Collectors.toSet());
 

Original file line number	Diff line number	Diff line change
`@@ -187,5 +187,5 @@ List<VMInstanceVO> searchRemovedByRemoveDate(final Date startDate, final Date en`
`187`	`187`
`188`	`188`	`Map<String, Long> getNameIdMapForVmIds(Collection<Long> ids);`
`189`	`189`
`190`		`- List<VMInstanceVO> listByIds(List<Long> ids);`
	`190`	`+ List<VMInstanceVO> listByIdsIncludingRemoved(List<Long> ids);`
`191`	`191`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1226,7 +1226,7 @@ public Map<String, Long> getNameIdMapForVmIds(Collection<Long> ids) {`
`1226`	`1226`	`}`
`1227`	`1227`
`1228`	`1228`	`@Override`
`1229`		`- public List<VMInstanceVO> listByIds(List<Long> ids) {`
	`1229`	`+ public List<VMInstanceVO> listByIdsIncludingRemoved(List<Long> ids) {`
`1230`	`1230`	`SearchBuilder<VMInstanceVO> idsSearch = createSearchBuilder();`
`1231`	`1231`	`idsSearch.and("ids", idsSearch.entity().getId(), SearchCriteria.Op.IN);`
`1232`	`1232`	`idsSearch.done();`
Original file line number	Diff line number	Diff line change
`@@ -210,7 +210,7 @@ public BackupVO persist(BackupVO backup) {`
`210`	`210`	`public boolean update(Long id, BackupVO backup) {`
`211`	`211`	`return Transaction.execute((TransactionCallback<Boolean>) status -> {`
`212`	`212`	`boolean result = super.update(id, backup);`
`213`		`- if (result == true) {`
	`213`	`+ if (result) {`
`214`	`214`	`saveDetails(backup);`
`215`	`215`	`}`
`216`	`216`	`return result;`
`@@ -295,7 +295,7 @@ public BackupResponse newBackupResponse(Backup backup, Boolean listVmDetails) {`
`295`	`295`	`if (vm.getState() != VirtualMachine.State.Expunging) {`
`296`	`296`	`response.setVmId(vm.getUuid());`
`297`	`297`	`}`
`298`		`- if (vm.getBackupOfferingId() != backup.getBackupOfferingId()) {`
	`298`	`+ if (vm.getBackupOfferingId() == null \|\| vm.getBackupOfferingId() != backup.getBackupOfferingId()) {`
`299`	`299`	`response.setVmOfferingRemoved(true);`
`300`	`300`	`}`
`301`	`301`	`response.setExternalId(backup.getExternalId());`