Fixed VMware import issue - check and update pools in the order of the disks (do not update by position) (#10409)

This commit is contained in:
Suresh Kumar Anaparti 2025-02-18 13:17:05 +05:30 committed by GitHub
parent 212f2a3898
commit b6cebe22f9
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 77 additions and 81 deletions

View File

@ -133,13 +133,13 @@ public interface VolumeApiService {
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 @@ public interface VolumeApiService {
* </body>
* </table>
*/
boolean doesTargetStorageSupportDiskOffering(StoragePool destPool, String diskOfferingTags);
boolean doesStoragePoolSupportDiskOfferingTags(StoragePool destPool, String diskOfferingTags);
Volume destroyVolume(long volumeId, Account caller, boolean expunge, boolean forceExpunge);

View File

@ -3519,7 +3519,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
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 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
}
/**
* 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 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
* </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 class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
}
@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 class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
} 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;
}

View File

@ -205,7 +205,7 @@ public class VolumeImportUnmanageManagerImpl implements VolumeImportUnmanageServ
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()));
}

View File

@ -456,7 +456,7 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager {
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 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager {
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 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager {
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 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager {
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,7 +2018,25 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager {
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) {
@ -2026,44 +2044,26 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager {
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());
}
}
pools = getPoolsWithMatchingTags(pools, diskOfferingTags);
if (pools.isEmpty()) {
String msg = String.format("Cannot find suitable storage pools in cluster %s for the conversion", destinationCluster.getName());
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);
}
}
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 (volumeApiService.doesStoragePoolSupportDiskOfferingTags(pool, tags)) {
return pool;
}
}
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);
}
}
return poolsSupportingTags;
return null;
}
private List<String> selectInstanceConversionStoragePools(
@ -2071,26 +2071,22 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager {
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;

View File

@ -1164,7 +1164,7 @@ public class VolumeApiServiceImplTest {
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
Mockito.doReturn(new Pair<>(List.of("A"), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
Assert.assertFalse(result);
}
@ -1177,7 +1177,7 @@ public class VolumeApiServiceImplTest {
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
Mockito.doReturn(new Pair<>(List.of("A","B","C","D","X","Y"), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
Assert.assertTrue(result);
}
@ -1190,7 +1190,7 @@ public class VolumeApiServiceImplTest {
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
Mockito.lenient().doReturn(new Pair<>(List.of("A,B,C,D,X,Y"), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
Assert.assertTrue(result);
}
@ -1203,7 +1203,7 @@ public class VolumeApiServiceImplTest {
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
Mockito.doReturn(new Pair<>(List.of(""), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
Assert.assertFalse(result);
}
@ -1216,7 +1216,7 @@ public class VolumeApiServiceImplTest {
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
Mockito.lenient().doReturn(new Pair<>(List.of(""), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
Assert.assertTrue(result);
}
@ -1229,7 +1229,7 @@ public class VolumeApiServiceImplTest {
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
Mockito.doReturn(new Pair<>(List.of("C,D"), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
Assert.assertFalse(result);
}
@ -1242,7 +1242,7 @@ public class VolumeApiServiceImplTest {
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
Mockito.doReturn(new Pair<>(List.of("A"), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
Assert.assertTrue(result);
}
@ -1255,7 +1255,7 @@ public class VolumeApiServiceImplTest {
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
Mockito.doReturn(new Pair<>(List.of("tags[0] == 'A'"), true)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
Assert.assertTrue(result);
}
@ -1268,7 +1268,7 @@ public class VolumeApiServiceImplTest {
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
Mockito.doReturn(new Pair<>(List.of("tags[0] == 'A'"), true)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
Assert.assertFalse(result);
}
@ -1281,7 +1281,7 @@ public class VolumeApiServiceImplTest {
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
Mockito.doReturn(new Pair<>(List.of("tags[0] == 'A'"), true)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
Assert.assertFalse(result);
}

View File

@ -275,7 +275,7 @@ public class VolumeImportUnmanageManagerImplTest {
when(diskOffering.isCustomized()).thenReturn(true);
doReturn(diskOffering).when(volumeImportUnmanageManager).getOrCreateDiskOffering(account, diskOfferingId, zoneId, isLocal);
doNothing().when(volumeApiService).validateCustomDiskOfferingSizeRange(anyLong());
doReturn(true).when(volumeApiService).doesTargetStorageSupportDiskOffering(any(), isNull());
doReturn(true).when(volumeApiService).doesStoragePoolSupportDiskOfferingTags(any(), isNull());
doReturn(diskProfile).when(volumeManager).importVolume(any(), anyString(), any(), eq(virtualSize), isNull(), isNull(), anyLong(),
any(), isNull(), isNull(), any(), isNull(), anyLong(), anyString(), isNull());
when(diskProfile.getVolumeId()).thenReturn(volumeId);

View File

@ -424,7 +424,7 @@ public class UnmanagedVMsManagerImplTest {
ImportUnmanagedInstanceCmd importUnmanageInstanceCmd = Mockito.mock(ImportUnmanagedInstanceCmd.class);
when(importUnmanageInstanceCmd.getName()).thenReturn("TestInstance");
when(importUnmanageInstanceCmd.getDomainId()).thenReturn(null);
when(volumeApiService.doesTargetStorageSupportDiskOffering(any(StoragePool.class), any())).thenReturn(true);
when(volumeApiService.doesStoragePoolSupportDiskOfferingTags(any(StoragePool.class), any())).thenReturn(true);
try (MockedStatic<UsageEventUtils> ignored = Mockito.mockStatic(UsageEventUtils.class)) {
unmanagedVMsManager.importUnmanagedInstance(importUnmanageInstanceCmd);
}
@ -704,7 +704,7 @@ public class UnmanagedVMsManagerImplTest {
when(agentManager.send(Mockito.eq(convertHostId), Mockito.any(CheckConvertInstanceCommand.class))).thenReturn(checkConvertInstanceAnswer);
}
when(volumeApiService.doesTargetStorageSupportDiskOffering(any(StoragePool.class), any())).thenReturn(true);
when(volumeApiService.doesStoragePoolSupportDiskOfferingTags(any(StoragePool.class), any())).thenReturn(true);
ConvertInstanceAnswer convertInstanceAnswer = mock(ConvertInstanceAnswer.class);
ImportConvertedInstanceAnswer convertImportedInstanceAnswer = mock(ImportConvertedInstanceAnswer.class);
@ -769,7 +769,7 @@ public class UnmanagedVMsManagerImplTest {
storagePools.add(storagePool);
when(primaryDataStoreDao.findLocalStoragePoolsByHostAndTags(anyLong(), any())).thenReturn(storagePools);
when(primaryDataStoreDao.findById(anyLong())).thenReturn(storagePool);
when(volumeApiService.doesTargetStorageSupportDiskOffering(any(StoragePool.class), any())).thenReturn(true);
when(volumeApiService.doesStoragePoolSupportDiskOfferingTags(any(StoragePool.class), any())).thenReturn(true);
StoragePoolHostVO storagePoolHost = Mockito.mock(StoragePoolHostVO.class);
when(storagePoolHostDao.findByPoolHost(anyLong(), anyLong())).thenReturn(storagePoolHost);
try (MockedStatic<UsageEventUtils> ignored = Mockito.mockStatic(UsageEventUtils.class)) {