Addressed last set of review comments

abh1sar · abh1sar · commit 1123adfc889c · 2025-03-17T15:25:10.000+05:30
diff --git a/api/src/main/java/org/apache/cloudstack/api/response/BackupResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/BackupResponse.java
@@ -36,11 +36,11 @@ public class BackupResponse extends BaseResponse {
     private String id;
 
     @SerializedName(ApiConstants.NAME)
-    @Param(description = "name of the backup")
+    @Param(description = "name of the backup", since = "4.21.0")
     private String name;
 
     @SerializedName(ApiConstants.DESCRIPTION)
-    @Param(description = "description for the backup")
+    @Param(description = "description for the backup", since = "4.21.0")
     private String description;
 
     @SerializedName(ApiConstants.VIRTUAL_MACHINE_ID)
@@ -116,11 +116,11 @@ public class BackupResponse extends BaseResponse {
     private Map<String, String> vmDetails;
 
     @SerializedName(ApiConstants.INTERVAL_TYPE)
-    @Param(description = "the interval type of the backup schedule")
+    @Param(description = "the interval type of the backup schedule", since = "4.21.0")
     private String intervalType;
 
     @SerializedName(ApiConstants.IS_ORPHAN)
-    @Param(description = "The backups is orphaned and no associated with an existing VM. A new VM can still be created using the backup")
+    @Param(description = "The backups is orphaned and not associated with an existing VM. A new VM can still be created using the backup", since = "4.21.0")
     private Boolean isOrphan;
 
     public String getId() {
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
@@ -38,19 +38,19 @@ public class ObjectStoreResponse extends BaseResponseWithAnnotations {
     @Param(description = "the url of the object store")
     private String url;
 
-    @SerializedName(ApiConstants.PROVIDER)
-    @Param(description = "the name of the object store provider")
+    @SerializedName("providername")
+    @Param(description = "the provider name of the object store")
     private String providerName;
 
-    @SerializedName(ApiConstants.SIZE)
+    @SerializedName("storagetotal")
     @Param(description = "the total size of the object store")
     private Long storageTotal;
 
-    @SerializedName(ApiConstants.ALLOCATED)
+    @SerializedName("storageallocated")
     @Param(description = "the object store currently allocated size")
     private Long storageAllocated;
 
-    @SerializedName(ApiConstants.USED)
+    @SerializedName("storageused")
     @Param(description = "the object store currently used size")
     private Long storageUsed;
 
diff --git a/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java b/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java
@@ -144,6 +144,14 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
             ConfigKey.Scope.Global,
             null);
 
+    ConfigKey<Float> BackupStorageCapacityThreshold = new ConfigKey<>("Alert", Float.class,
+            "zone.backupStorage.capacity.notificationthreshold",
+            "0.75",
+            "Percentage (as a value between 0 and 1) of backup storage utilization above which alerts will be sent about low storage available.",
+            true,
+            ConfigKey.Scope.Zone,
+            null);
+
     /**
      * List backup provider offerings
      * @param zoneId zone id
@@ -254,6 +262,8 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
 
     Map<String, String> getDiskOfferingDetailsForBackup(Long vmId);
 
+    String getBackupNameFromVM(VirtualMachine vm);
+
     void updateOrphanedBackups(VirtualMachine vm);
 
     Capacity getBackupStorageUsedStats(Long zoneId);
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
@@ -220,6 +220,14 @@ public interface StorageManager extends StorageService {
             "storage.pool.host.connect.workers", "1",
             "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",
+            "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,
+            ConfigKey.Scope.Global,
+            null);
+
     /**
      * should we execute in sequence not involving any storages?
      * @return tru if commands should execute in sequence
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
@@ -38,10 +38,10 @@ CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.host', 'last_mgmt_server_id', 'bigin
 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';
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'name', 'VARCHAR(255) NOT NULL COMMENT "name of the backup"');
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'description', 'VARCHAR(1024) COMMENT "description for the backup"');
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'backup_interval_type', 'int(5) COMMENT "type of backup, e.g. manual, recurring - hourly, daily, weekly or monthly"');
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backups', 'vm_name', 'VARCHAR(255) COMMENT "name of the vm for which backup is taken, set only for orphaned backups"');
 UPDATE `cloud`.`backups` JOIN `cloud`.`vm_instance` ON `backups`.`vm_id` = `vm_instance`.`id` SET `backups`.`name` = `vm_instance`.`name`;
 
 -- Make the column vm_id in backups table nullable to handle orphan backups
@@ -59,7 +59,7 @@ CREATE TABLE `cloud`.`backup_details` (
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
 
 -- Add column allocated_size to object_store table. Rename column 'used_bytes' to 'used_size'
-ALTER TABLE `cloud`.`object_store` ADD COLUMN `allocated_size` bigint unsigned COMMENT 'allocated size in bytes';
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.object_store', 'allocated_size', 'bigint unsigned COMMENT "allocated size in bytes"');
 ALTER TABLE `cloud`.`object_store` CHANGE COLUMN `used_bytes` `used_size` BIGINT UNSIGNED COMMENT 'used size in bytes';
 ALTER TABLE `cloud`.`object_store` MODIFY COLUMN `total_size` bigint unsigned COMMENT 'total size in bytes';
 UPDATE `cloud`.`object_store`
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
@@ -16,7 +16,6 @@
 // under the License.
 package org.apache.cloudstack.backup;
 
-import java.text.SimpleDateFormat;
 import java.util.Arrays;
 import java.util.Date;
 import java.util.HashMap;
@@ -157,7 +156,7 @@ public Pair<Boolean, Backup> takeBackup(VirtualMachine vm) {
         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()));
+        backup.setName(backupManager.getBackupNameFromVM(vm));
         backup.setBackedUpVolumes(BackupManagerImpl.createVolumeInfoFromVolumes(volumeDao.findByInstance(vm.getId())));
         Map<String, String> details = backupManager.getVmDetailsForBackup(vm);
         backup.setDetails(details);
diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java
@@ -24,6 +24,7 @@
 import com.cloud.host.Status;
 import com.cloud.host.dao.HostDao;
 import com.cloud.hypervisor.Hypervisor;
+import com.cloud.resource.ResourceManager;
 import com.cloud.storage.ScopeType;
 import com.cloud.storage.StoragePoolHostVO;
 import com.cloud.storage.Volume;
@@ -94,16 +95,16 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co
     @Inject
     BackupManager backupManager;
 
+    @Inject
+    ResourceManager resourceManager;
+
     private Host getUpHostInZone(Long zoneId) {
         List<HostVO> hosts = hostDao.listByDataCenterIdAndHypervisorType(zoneId, Hypervisor.HypervisorType.KVM);
-        if (hosts.size() == 0) {
+        if (CollectionUtils.isNotEmpty(hosts)) {
             return null;
         }
-
-        Random random = new Random();
-        int randomIndex = random.nextInt(hosts.size());
-        HostVO hostInZone = hosts.get(randomIndex);
-        LOG.debug("Found Host {} in zone {}", zoneId);
+        HostVO hostInZone = hosts.get(new Random().nextInt(hosts.size()));
+        LOG.debug("Found Host {} in zone {}", hostInZone, zoneId);
         return hostInZone;
     }
 
@@ -127,7 +128,7 @@ protected Host getLastVMHypervisorHost(VirtualMachine vm) {
             }
         }
         // Try to find any Host in the zone
-        return getUpHostInZone(host.getDataCenterId());
+        return resourceManager.findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM, host.getDataCenterId());
     }
 
     protected Host getVMHypervisorHost(VirtualMachine vm) {
@@ -219,7 +220,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()));
+        backup.setName(backupManager.getBackupNameFromVM(vm));
         Map<String, String> details = backupManager.getVmDetailsForBackup(vm);
         backup.setDetails(details);
 
@@ -375,7 +376,7 @@ public boolean deleteBackup(Backup backup, boolean forced) {
         if (vm != null) {
             host = getLastVMHypervisorHost(vm);
         } else {
-            host = getUpHostInZone(backup.getZoneId());
+            host = resourceManager.findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM, backup.getZoneId());
         }
 
         DeleteBackupCommand command = new DeleteBackupCommand(backup.getExternalId(), backupRepository.getType(),
@@ -473,7 +474,7 @@ public Pair<Long, Long> getBackupStorageStats(Long zoneId) {
     @Override
     public void syncBackupStorageStats(Long zoneId) {
         final List<BackupRepository> repositories = backupRepositoryDao.listByZoneAndProvider(zoneId, getName());
-        final Host host = getUpHostInZone(zoneId);
+        final Host host = resourceManager.findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM, zoneId);
         for (final BackupRepository repository : repositories) {
             GetBackupStorageStatsCommand command = new GetBackupStorageStatsCommand(repository.getType(), repository.getAddress(), repository.getMountOptions());
             BackupStorageStatsAnswer answer;
diff --git a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java
@@ -45,6 +45,7 @@
 import com.cloud.host.Status;
 import com.cloud.host.dao.HostDao;
 import com.cloud.hypervisor.Hypervisor;
+import com.cloud.resource.ResourceManager;
 import com.cloud.storage.Volume;
 import com.cloud.storage.VolumeVO;
 import com.cloud.storage.dao.VolumeDao;
@@ -83,6 +84,9 @@ public class NASBackupProviderTest {
     @Mock
     private BackupManager backupManager;
 
+    @Mock
+    private ResourceManager resourceManager;
+
     @Test
     public void testDeleteBackup() throws OperationTimedoutException, AgentUnavailableException {
         Long hostId = 1L;
@@ -135,7 +139,7 @@ public void testSyncBackupStorageStats() throws AgentUnavailableException, Opera
                 "nfs", "address", "sync", 1024L);
 
         HostVO host = mock(HostVO.class);
-        Mockito.when(hostDao.listByDataCenterIdAndHypervisorType(1L, Hypervisor.HypervisorType.KVM)).thenReturn(Collections.singletonList(host));
+        Mockito.when(resourceManager.findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM, 1L)).thenReturn(host);
 
         Mockito.when(backupRepositoryDao.listByZoneAndProvider(1L, "nas")).thenReturn(Collections.singletonList(backupRepository));
         GetBackupStorageStatsCommand command = new GetBackupStorageStatsCommand("nfs", "address", "sync");
@@ -198,7 +202,6 @@ public void takeBackupSuccessfully() throws AgentUnavailableException, Operation
         Mockito.when(vm.getAccountId()).thenReturn(accountId);
         Mockito.when(vm.getDomainId()).thenReturn(domainId);
         Mockito.when(vm.getDataCenterId()).thenReturn(zoneId);
-        Mockito.when(vm.getHostName()).thenReturn("test-host");
         Mockito.when(vm.getState()).thenReturn(VMInstanceVO.State.Running);
 
         BackupRepository backupRepository = mock(BackupRepository.class);
diff --git a/plugins/backup/networker/src/main/java/org/apache/cloudstack/backup/NetworkerBackupProvider.java b/plugins/backup/networker/src/main/java/org/apache/cloudstack/backup/NetworkerBackupProvider.java
@@ -611,7 +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()));
+            backup.setName(backupManager.getBackupNameFromVM(vm));
 
             HashMap<String, String> details = new HashMap<>();
             details.put(ApiConstants.HYPERVISOR, vm.getHypervisorType().toString());
diff --git a/plugins/backup/networker/src/main/java/org/apache/cloudstack/backup/networker/NetworkerClient.java b/plugins/backup/networker/src/main/java/org/apache/cloudstack/backup/networker/NetworkerClient.java
@@ -24,6 +24,7 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import org.apache.cloudstack.api.ApiErrorCode;
 import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.backup.BackupManager;
 import org.apache.cloudstack.backup.BackupOffering;
 import org.apache.cloudstack.backup.BackupVO;
 import org.apache.cloudstack.backup.networker.api.NetworkerBackup;
@@ -45,6 +46,7 @@
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
 
+import javax.inject.Inject;
 import javax.net.ssl.SSLContext;
 import javax.net.ssl.X509TrustManager;
 import java.io.IOException;
@@ -65,6 +67,9 @@
 import static org.apache.cloudstack.backup.NetworkerBackupProvider.BACKUP_IDENTIFIER;
 
 public class NetworkerClient {
+    @Inject
+    BackupManager backupManager;
+
     private static final Logger LOG = LogManager.getLogger(NetworkerClient.class);
     private final URI apiURI;
     private final String apiName;
@@ -267,7 +272,7 @@ public BackupVO registerBackupForVm(VirtualMachine vm, Date backupJobStart, Stri
             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()));
+            backup.setName(backupManager.getBackupNameFromVM(vm));
 
             return backup;
         } catch (final IOException e) {
diff --git a/plugins/backup/networker/src/test/java/org/apache/backup/networker/NetworkerClientTest.java b/plugins/backup/networker/src/test/java/org/apache/backup/networker/NetworkerClientTest.java
@@ -30,8 +30,11 @@
 import java.time.Instant;
 import java.util.Date;
 import java.util.List;
+
 import com.cloud.vm.VMInstanceVO;
 import com.github.tomakehurst.wiremock.client.VerificationException;
+
+import org.apache.cloudstack.backup.BackupManager;
 import org.apache.cloudstack.backup.BackupOffering;
 import org.apache.cloudstack.backup.BackupVO;
 import org.apache.cloudstack.backup.networker.NetworkerClient;
@@ -40,6 +43,9 @@
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.mockito.Mockito;
+import org.springframework.test.util.ReflectionTestUtils;
+
 import com.github.tomakehurst.wiremock.client.BasicCredentials;
 import com.github.tomakehurst.wiremock.junit.WireMockRule;
 
@@ -58,6 +64,7 @@ public void setUp() throws Exception {
                         .willReturn(aResponse()
                         .withStatus(200)));
         client = new NetworkerClient(url, adminUsername, adminPassword, false, 60);
+        ReflectionTestUtils.setField(client, "backupManager", Mockito.mock(BackupManager.class));
     }
 
     @Test
diff --git a/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java b/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java
@@ -20,9 +20,7 @@
 import java.net.URISyntaxException;
 import java.security.KeyManagementException;
 import java.security.NoSuchAlgorithmException;
-import java.text.SimpleDateFormat;
 import java.util.ArrayList;
-import java.util.Date;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -337,7 +335,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()));
+        backup.setName(backupManager.getBackupNameFromVM(vm));
         backup.setBackedUpVolumes(BackupManagerImpl.createVolumeInfoFromVolumes(volumeDao.findByInstance(vm.getId())));
         Map<String, String> details = backupManager.getVmDetailsForBackup(vm);
         backup.setDetails(details);
diff --git a/server/src/main/java/com/cloud/alert/AlertManagerImpl.java b/server/src/main/java/com/cloud/alert/AlertManagerImpl.java
@@ -189,8 +189,8 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
         String vlanCapacityThreshold = _configDao.getValue(Config.VlanCapacityThreshold.key());
         String directNetworkPublicIpCapacityThreshold = _configDao.getValue(Config.DirectNetworkPublicIpCapacityThreshold.key());
         String localStorageCapacityThreshold = _configDao.getValue(Config.LocalStorageCapacityThreshold.key());
-        String backupStorageCapacityThreshold = _configDao.getValue(Config.BackupStorageCapacityThreshold.key());
-        String objectStorageCapacityThreshold = _configDao.getValue(Config.ObjectStorageCapacityThreshold.key());
+        String backupStorageCapacityThreshold = _configDao.getValue(BackupManager.BackupStorageCapacityThreshold.key());
+        String objectStorageCapacityThreshold = _configDao.getValue(_storageMgr.ObjectStorageCapacityThreshold.key());
 
         if (publicIPCapacityThreshold != null) {
             _publicIPCapacityThreshold = Double.parseDouble(publicIPCapacityThreshold);
diff --git a/server/src/main/java/com/cloud/configuration/Config.java b/server/src/main/java/com/cloud/configuration/Config.java
@@ -134,22 +134,6 @@ 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(
-            "Alert",
-            ManagementServer.class,
-            Float.class,
-            "zone.backupStorage.capacity.notificationthreshold",
-            "0.75",
-            "Percentage (as a value between 0 and 1) of backup storage utilization above which alerts will be sent about low storage available.",
-            null),
-    ObjectStorageCapacityThreshold(
-            "Alert",
-            ManagementServer.class,
-            Float.class,
-            "zone.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.",
-            null),
 
     // Storage
 
diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java
diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java
diff --git a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java
diff --git a/ui/src/views/compute/wizard/TemplateIsoSelection.vue b/ui/src/views/compute/wizard/TemplateIsoSelection.vue
