apache · DaanHoogland · Feb 18, 2025 · Feb 17, 2025
diff --git a/api/src/main/java/com/cloud/storage/VolumeApiService.java b/api/src/main/java/com/cloud/storage/VolumeApiService.java
@@ -133,13 +133,13 @@ Snapshot takeSnapshot(Long volumeId, Long policyId, Long snapshotId, Account acc
     Snapshot allocSnapshotForVm(Long vmId, Long volumeId, String snapshotName) throws ResourceAllocationException;
 
     /**
-     *  Checks if the target storage supports the disk offering.
+     *  Checks if the storage pool supports the disk offering tags.
      *  This validation is consistent with the mechanism used to select a storage pool to deploy a volume when a virtual machine is deployed or when a data disk is allocated.
      *
      *  The scenarios when this method returns true or false is presented in the following table.
      *   <table border="1">
      *      <tr>
-     *          <th>#</th><th>Disk offering tags</th><th>Storage tags</th><th>Does the storage support the disk offering?</th>
+     *          <th>#</th><th>Disk offering diskOfferingTags</th><th>Storage diskOfferingTags</th><th>Does the storage support the disk offering?</th>
      *      </tr>
      *      <body>
      *      <tr>
@@ -163,7 +163,7 @@ Snapshot takeSnapshot(Long volumeId, Long policyId, Long snapshotId, Account acc
      *      </body>
      *   </table>
      */
-    boolean doesTargetStorageSupportDiskOffering(StoragePool destPool, String diskOfferingTags);
+    boolean doesStoragePoolSupportDiskOfferingTags(StoragePool destPool, String diskOfferingTags);
 
     Volume destroyVolume(long volumeId, Account caller, boolean expunge, boolean forceExpunge);
 

diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
@@ -3519,7 +3519,7 @@ protected void validateConditionsToReplaceDiskOfferingOfVolume(VolumeVO volume,
         if ((destPool.isShared() && newDiskOffering.isUseLocalStorage()) || destPool.isLocal() && newDiskOffering.isShared()) {
             throw new InvalidParameterValueException("You cannot move the volume to a shared storage and assign a disk offering for local storage and vice versa.");
         }
-        if (!doesTargetStorageSupportDiskOffering(destPool, newDiskOffering)) {
+        if (!doesStoragePoolSupportDiskOffering(destPool, newDiskOffering)) {
             throw new InvalidParameterValueException(String.format("Migration failed: target pool [%s, tags:%s] has no matching tags for volume [%s, uuid:%s, tags:%s]", destPool.getName(),
                     storagePoolTagsDao.getStoragePoolTags(destPool.getId()), volume.getName(), volume.getUuid(), newDiskOffering.getTags()));
         }
@@ -3546,7 +3546,7 @@ protected void validateConditionsToReplaceDiskOfferingOfVolume(VolumeVO volume,
     }
 
     /**
-     *  Checks if the target storage supports the new disk offering.
+     *  Checks if the storage pool supports the new disk offering.
      *  This validation is consistent with the mechanism used to select a storage pool to deploy a volume when a virtual machine is deployed or when a new data disk is allocated.
      *
      *  The scenarios when this method returns true or false is presented in the following table.
@@ -3577,9 +3577,9 @@ protected void validateConditionsToReplaceDiskOfferingOfVolume(VolumeVO volume,
      *      </body>
      *   </table>
      */
-    protected boolean doesTargetStorageSupportDiskOffering(StoragePool destPool, DiskOfferingVO diskOffering) {
-        String targetStoreTags = diskOffering.getTags();
-        return doesTargetStorageSupportDiskOffering(destPool, targetStoreTags);
+    protected boolean doesStoragePoolSupportDiskOffering(StoragePool destPool, DiskOfferingVO diskOffering) {
+        String offeringTags = diskOffering.getTags();
+        return doesStoragePoolSupportDiskOfferingTags(destPool, offeringTags);
     }
 
     public static boolean doesNewDiskOfferingHasTagsAsOldDiskOffering(DiskOfferingVO oldDO, DiskOfferingVO newDO) {
@@ -3595,18 +3595,18 @@ public static boolean doesNewDiskOfferingHasTagsAsOldDiskOffering(DiskOfferingVO
     }
 
     @Override
-    public boolean doesTargetStorageSupportDiskOffering(StoragePool destPool, String diskOfferingTags) {
+    public boolean doesStoragePoolSupportDiskOfferingTags(StoragePool destPool, String diskOfferingTags) {
         Pair<List<String>, Boolean> storagePoolTags = getStoragePoolTags(destPool);
         if ((storagePoolTags == null || !storagePoolTags.second()) && org.apache.commons.lang.StringUtils.isBlank(diskOfferingTags)) {
             if (storagePoolTags == null) {
-                s_logger.debug(String.format("Destination storage pool [%s] does not have any tags, and so does the disk offering. Therefore, they are compatible", destPool.getUuid()));
+                s_logger.debug(String.format("Storage pool [%s] does not have any tags, and so does the disk offering. Therefore, they are compatible", destPool.getUuid()));
             } else {
-                s_logger.debug("Destination storage pool has tags [%s], and the disk offering has no tags. Therefore, they are compatible.");
+                s_logger.debug("Storage pool has tags [%s], and the disk offering has no tags. Therefore, they are compatible.");
             }
             return true;
         }
         if (storagePoolTags == null || CollectionUtils.isEmpty(storagePoolTags.first())) {
-            s_logger.debug(String.format("Destination storage pool [%s] has no tags, while disk offering has tags [%s]. Therefore, they are not compatible", destPool.getUuid(),
+            s_logger.debug(String.format("Storage pool [%s] has no tags, while disk offering has tags [%s]. Therefore, they are not compatible", destPool.getUuid(),
                     diskOfferingTags));
             return false;
         }
@@ -3619,7 +3619,7 @@ public boolean doesTargetStorageSupportDiskOffering(StoragePool destPool, String
         } else {
             result = CollectionUtils.isSubCollection(Arrays.asList(newDiskOfferingTagsAsStringArray), storageTagsList);
         }
-        s_logger.debug(String.format("Destination storage pool [%s] accepts tags [%s]? %s", destPool.getUuid(), diskOfferingTags, result));
+        s_logger.debug(String.format("Storage pool [%s] accepts tags [%s]? %s", destPool.getUuid(), diskOfferingTags, result));
         return result;
     }
 

diff --git a/...r/src/main/java/org/apache/cloudstack/storage/volume/VolumeImportUnmanageManagerImpl.java b/...r/src/main/java/org/apache/cloudstack/storage/volume/VolumeImportUnmanageManagerImpl.java
@@ -205,7 +205,7 @@ public VolumeResponse importVolume(ImportVolumeCmd cmd) {
         if (diskOffering.isCustomized()) {
             volumeApiService.validateCustomDiskOfferingSizeRange(volume.getVirtualSize() / ByteScaleUtils.GiB);
         }
-        if (!volumeApiService.doesTargetStorageSupportDiskOffering(pool, diskOffering.getTags())) {
+        if (!volumeApiService.doesStoragePoolSupportDiskOfferingTags(pool, diskOffering.getTags())) {
             logFailureAndThrowException(String.format("Disk offering: %s storage tags are not compatible with selected storage pool: %s", diskOffering.getUuid(), pool.getUuid()));
         }
 

diff --git a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java
@@ -456,7 +456,7 @@
         if (diskOffering == null) {
             return false;
         }
-        return volumeApiService.doesTargetStorageSupportDiskOffering(pool, diskOffering.getTags());
+        return volumeApiService.doesStoragePoolSupportDiskOfferingTags(pool, diskOffering.getTags());
     }
 
     private ServiceOfferingVO getUnmanagedInstanceServiceOffering(final UnmanagedInstanceTO instance, ServiceOfferingVO serviceOffering, final Account owner, final DataCenter zone, final Map<String, String> details, Hypervisor.HypervisorType hypervisorType)
@@ -547,7 +547,7 @@
             for (StoragePool pool : pools) {
                 if (pool.getDataCenterId() == zone.getId() &&
                         (pool.getClusterId() == null || pool.getClusterId().equals(cluster.getId())) &&
-                        volumeApiService.doesTargetStorageSupportDiskOffering(pool, diskOfferingTags)) {
+                        volumeApiService.doesStoragePoolSupportDiskOfferingTags(pool, diskOfferingTags)) {
                     storagePool = pool;
                     break;
                 }
@@ -560,7 +560,7 @@
             for (StoragePool pool : pools) {
                 String searchPoolParam = StringUtils.isNotBlank(dsPath) ? dsPath : dsName;
                 if (StringUtils.contains(pool.getPath(), searchPoolParam) &&
-                        volumeApiService.doesTargetStorageSupportDiskOffering(pool, diskOfferingTags)) {
+                        volumeApiService.doesStoragePoolSupportDiskOfferingTags(pool, diskOfferingTags)) {
                     storagePool = pool;
                     break;
                 }
@@ -1732,9 +1732,9 @@
         convertedInstance.setPowerState(UnmanagedInstanceTO.PowerState.PowerOff);
         List<UnmanagedInstanceTO.Disk> convertedInstanceDisks = convertedInstance.getDisks();
         List<UnmanagedInstanceTO.Disk> sourceVMwareInstanceDisks = sourceVMwareInstance.getDisks();
-        for (UnmanagedInstanceTO.Disk sourceVMwareInstanceDisk : sourceVMwareInstanceDisks) {
-            UnmanagedInstanceTO.Disk convertedDisk = convertedInstanceDisks.get(sourceVMwareInstanceDisk.getPosition());
-            convertedDisk.setDiskId(sourceVMwareInstanceDisk.getDiskId());
+        for (int i = 0; i < convertedInstanceDisks.size(); i++) {
+            UnmanagedInstanceTO.Disk disk = convertedInstanceDisks.get(i);
+            disk.setDiskId(sourceVMwareInstanceDisks.get(i).getDiskId());
         }
         List<UnmanagedInstanceTO.Nic> convertedInstanceNics = convertedInstance.getNics();
         List<UnmanagedInstanceTO.Nic> sourceVMwareInstanceNics = sourceVMwareInstance.getNics();
@@ -2018,79 +2018,75 @@
         List<StoragePoolVO> pools = new ArrayList<>();
         pools.addAll(primaryDataStoreDao.findClusterWideStoragePoolsByHypervisorAndPoolType(destinationCluster.getId(), Hypervisor.HypervisorType.KVM, Storage.StoragePoolType.NetworkFilesystem));
         pools.addAll(primaryDataStoreDao.findZoneWideStoragePoolsByHypervisorAndPoolType(destinationCluster.getDataCenterId(), Hypervisor.HypervisorType.KVM, Storage.StoragePoolType.NetworkFilesystem));
-        List<String> diskOfferingTags = new ArrayList<>();
+        if (pools.isEmpty()) {
+            String msg = String.format("Cannot find suitable storage pools in the cluster %s for the conversion", destinationCluster.getName());
+            LOGGER.error(msg);
+            throw new CloudRuntimeException(msg);
+        }
+
+        if (serviceOffering.getDiskOfferingId() != null) {
+            DiskOfferingVO diskOffering = diskOfferingDao.findById(serviceOffering.getDiskOfferingId());
+            if (diskOffering == null) {
+                String msg = String.format("Cannot find disk offering with ID %s that belongs to the service offering %s", serviceOffering.getDiskOfferingId(), serviceOffering.getName());
+                LOGGER.error(msg);
+                throw new CloudRuntimeException(msg);
+            }
+            if (getStoragePoolWithTags(pools, diskOffering.getTags()) == null) {
+                String msg = String.format("Cannot find suitable storage pool for disk offering %s that belongs to the service offering %s", diskOffering.getName(), serviceOffering.getName());
+                LOGGER.error(msg);
+                throw new CloudRuntimeException(msg);
+            }
+        }
         for (Long diskOfferingId : dataDiskOfferingMap.values()) {
             DiskOfferingVO diskOffering = diskOfferingDao.findById(diskOfferingId);
             if (diskOffering == null) {
                 String msg = String.format("Cannot find disk offering with ID %s", diskOfferingId);
                 LOGGER.error(msg);
                 throw new CloudRuntimeException(msg);
             }
-            diskOfferingTags.add(diskOffering.getTags());
-        }
-        if (serviceOffering.getDiskOfferingId() != null) {
-            DiskOfferingVO diskOffering = diskOfferingDao.findById(serviceOffering.getDiskOfferingId());
-            if (diskOffering != null) {
-                diskOfferingTags.add(diskOffering.getTags());
+            if (getStoragePoolWithTags(pools, diskOffering.getTags()) == null) {
+                String msg = String.format("Cannot find suitable storage pool for disk offering %s", diskOffering.getName());
+                LOGGER.error(msg);
+                throw new CloudRuntimeException(msg);
             }
         }
 
-        pools = getPoolsWithMatchingTags(pools, diskOfferingTags);
-        if (pools.isEmpty()) {
-            String msg = String.format("Cannot find suitable storage pools in cluster %s for the conversion", destinationCluster.getName());
-            LOGGER.error(msg);
-            throw new CloudRuntimeException(msg);
-        }
         return pools;
     }
 
-    private List<StoragePoolVO> getPoolsWithMatchingTags(List<StoragePoolVO> pools, List<String> diskOfferingTags) {
-        if (diskOfferingTags.isEmpty()) {
-            return pools;
+    private StoragePoolVO getStoragePoolWithTags(List<StoragePoolVO> pools, String tags) {
+        if (StringUtils.isEmpty(tags)) {
+            return pools.get(0);
         }
-        List<StoragePoolVO> poolsSupportingTags = new ArrayList<>(pools);
-        for (String tags : diskOfferingTags) {
-            boolean tagsMatched = false;
-            for (StoragePoolVO pool : pools) {
-                if (volumeApiService.doesTargetStorageSupportDiskOffering(pool, tags)) {
-                    poolsSupportingTags.add(pool);
-                    tagsMatched = true;
-                }
-            }
-            if (!tagsMatched) {
-                String msg = String.format("Cannot find suitable storage pools for the conversion with disk offering tags %s", tags);
-                LOGGER.error(msg);
-                throw new CloudRuntimeException(msg);
+        for (StoragePoolVO pool : pools) {
+            if (volumeApiService.doesStoragePoolSupportDiskOfferingTags(pool, tags)) {
+                return pool;
             }
         }
-        return poolsSupportingTags;
+        return null;
     }
 
     private List<String> selectInstanceConversionStoragePools(
             List<StoragePoolVO> pools, List<UnmanagedInstanceTO.Disk> disks,
             ServiceOfferingVO serviceOffering, Map<String, Long> dataDiskOfferingMap
     ) {
         List<String> storagePools = new ArrayList<>(disks.size());
-        for (int i = 0; i < disks.size(); i++) {
-            storagePools.add(null);
-        }
         Set<String> dataDiskIds = dataDiskOfferingMap.keySet();
         for (UnmanagedInstanceTO.Disk disk : disks) {
-            Long diskOfferingId = dataDiskOfferingMap.get(disk.getDiskId());
-            if (diskOfferingId == null && !dataDiskIds.contains(disk.getDiskId())) {
+            Long diskOfferingId = null;
+            if (dataDiskIds.contains(disk.getDiskId())) {
+                diskOfferingId = dataDiskOfferingMap.get(disk.getDiskId());
+            } else {
                 diskOfferingId = serviceOffering.getDiskOfferingId();
             }
+
             //TODO: Choose pools by capacity
             if (diskOfferingId == null) {
-                storagePools.set(disk.getPosition(), pools.get(0).getUuid());
+                storagePools.add(pools.get(0).getUuid());
             } else {
                 DiskOfferingVO diskOffering = diskOfferingDao.findById(diskOfferingId);
-                for (StoragePoolVO pool : pools) {
-                    if (volumeApiService.doesTargetStorageSupportDiskOffering(pool, diskOffering.getTags())) {
-                        storagePools.set(disk.getPosition(), pool.getUuid());
-                        break;
-                    }
-                }
+                StoragePoolVO pool = getStoragePoolWithTags(pools, diskOffering.getTags());
+                storagePools.add(pool.getUuid());
             }
         }
         return storagePools;