Skip to content

Commit a91155c

Browse files
sureshanapartidhslove
sureshanaparti
authored and
dhslove
committed
Fixed VMware import issue - check and update pools in the order of the disks (do not update by position) (apache#10409)
1 parent ab12930 commit a91155c

File tree

7 files changed

+74
-47
lines changed

7 files changed

+74
-47
lines changed

‎api/src/main/java/com/cloud/storage/VolumeApiService.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,13 +139,13 @@ Snapshot takeSnapshot(Long volumeId, Long policyId, Long snapshotId, Account acc
139139
Snapshot allocSnapshotForVm(Long vmId, Long volumeId, String snapshotName) throws ResourceAllocationException;
140140

141141
/**
142-
* Checks if the target storage supports the disk offering.
142+
* Checks if the storage pool supports the disk offering tags.
143143
* 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.
144144
*
145145
* The scenarios when this method returns true or false is presented in the following table.
146146
* <table border="1">
147147
* <tr>
148-
* <th>#</th><th>Disk offering tags</th><th>Storage tags</th><th>Does the storage support the disk offering?</th>
148+
* <th>#</th><th>Disk offering diskOfferingTags</th><th>Storage diskOfferingTags</th><th>Does the storage support the disk offering?</th>
149149
* </tr>
150150
* <body>
151151
* <tr>
@@ -169,7 +169,7 @@ Snapshot takeSnapshot(Long volumeId, Long policyId, Long snapshotId, Account acc
169169
* </body>
170170
* </table>
171171
*/
172-
boolean doesTargetStorageSupportDiskOffering(StoragePool destPool, String diskOfferingTags);
172+
boolean doesStoragePoolSupportDiskOfferingTags(StoragePool destPool, String diskOfferingTags);
173173

174174
Volume destroyVolume(long volumeId, Account caller, boolean expunge, boolean forceExpunge);
175175

‎server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3822,7 +3822,7 @@ protected void validateConditionsToReplaceDiskOfferingOfVolume(VolumeVO volume,
38223822
if ((destPool.isShared() && newDiskOffering.isUseLocalStorage()) || destPool.isLocal() && newDiskOffering.isShared()) {
38233823
throw new InvalidParameterValueException("You cannot move the volume to a shared storage and assign a disk offering for local storage and vice versa.");
38243824
}
3825-
if (!doesTargetStorageSupportDiskOffering(destPool, newDiskOffering)) {
3825+
if (!doesStoragePoolSupportDiskOffering(destPool, newDiskOffering)) {
38263826
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(),
38273827
storagePoolTagsDao.getStoragePoolTags(destPool.getId()), volume.getName(), volume.getUuid(), newDiskOffering.getTags()));
38283828
}
@@ -3848,7 +3848,7 @@ protected void validateConditionsToReplaceDiskOfferingOfVolume(VolumeVO volume,
38483848
}
38493849

38503850
/**
3851-
* Checks if the target storage supports the new disk offering.
3851+
* Checks if the storage pool supports the new disk offering.
38523852
* 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.
38533853
*
38543854
* The scenarios when this method returns true or false is presented in the following table.
@@ -3879,9 +3879,9 @@ protected void validateConditionsToReplaceDiskOfferingOfVolume(VolumeVO volume,
38793879
* </body>
38803880
* </table>
38813881
*/
3882-
protected boolean doesTargetStorageSupportDiskOffering(StoragePool destPool, DiskOfferingVO diskOffering) {
3883-
String targetStoreTags = diskOffering.getTags();
3884-
return doesTargetStorageSupportDiskOffering(destPool, targetStoreTags);
3882+
protected boolean doesStoragePoolSupportDiskOffering(StoragePool destPool, DiskOfferingVO diskOffering) {
3883+
String offeringTags = diskOffering.getTags();
3884+
return doesStoragePoolSupportDiskOfferingTags(destPool, offeringTags);
38853885
}
38863886

38873887
public static boolean doesNewDiskOfferingHasTagsAsOldDiskOffering(DiskOfferingVO oldDO, DiskOfferingVO newDO) {
@@ -3897,18 +3897,18 @@ public static boolean doesNewDiskOfferingHasTagsAsOldDiskOffering(DiskOfferingVO
38973897
}
38983898

38993899
@Override
3900-
public boolean doesTargetStorageSupportDiskOffering(StoragePool destPool, String diskOfferingTags) {
3900+
public boolean doesStoragePoolSupportDiskOfferingTags(StoragePool destPool, String diskOfferingTags) {
39013901
Pair<List<String>, Boolean> storagePoolTags = getStoragePoolTags(destPool);
39023902
if ((storagePoolTags == null || !storagePoolTags.second()) && org.apache.commons.lang.StringUtils.isBlank(diskOfferingTags)) {
39033903
if (storagePoolTags == null) {
3904-
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()));
3904+
logger.debug("Storage pool [{}] does not have any tags, and so does the disk offering. Therefore, they are compatible", destPool.getUuid());
39053905
} else {
3906-
logger.debug("Destination storage pool has tags [%s], and the disk offering has no tags. Therefore, they are compatible.");
3906+
logger.debug("Storage pool has tags [%s], and the disk offering has no tags. Therefore, they are compatible.", destPool.getUuid());
39073907
}
39083908
return true;
39093909
}
39103910
if (storagePoolTags == null || CollectionUtils.isEmpty(storagePoolTags.first())) {
3911-
logger.debug(String.format("Destination storage pool [%s] has no tags, while disk offering has tags [%s]. Therefore, they are not compatible", destPool.getUuid(),
3911+
logger.debug("Destination storage pool [{}] has no tags, while disk offering has tags [{}]. Therefore, they are not compatible", destPool.getUuid(),
39123912
diskOfferingTags));
39133913
return false;
39143914
}
@@ -3921,7 +3921,7 @@ public boolean doesTargetStorageSupportDiskOffering(StoragePool destPool, String
39213921
} else {
39223922
result = CollectionUtils.isSubCollection(Arrays.asList(newDiskOfferingTagsAsStringArray), storageTagsList);
39233923
}
3924-
logger.debug(String.format("Destination storage pool [%s] accepts tags [%s]? %s", destPool.getUuid(), diskOfferingTags, result));
3924+
logger.debug(String.format("Destination storage pool [{}] accepts tags [{}]? {}", destPool.getUuid(), diskOfferingTags, result));
39253925
return result;
39263926
}
39273927

‎server/src/main/java/org/apache/cloudstack/storage/volume/VolumeImportUnmanageManagerImpl.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ public VolumeResponse importVolume(ImportVolumeCmd cmd) {
206206
if (diskOffering.isCustomized()) {
207207
volumeApiService.validateCustomDiskOfferingSizeRange(volume.getVirtualSize() / ByteScaleUtils.GiB);
208208
}
209-
if (!volumeApiService.doesTargetStorageSupportDiskOffering(pool, diskOffering.getTags())) {
209+
if (!volumeApiService.doesStoragePoolSupportDiskOffering(pool, diskOffering)) {
210210
logFailureAndThrowException(String.format("Disk offering: %s storage tags are not compatible with selected storage pool: %s", diskOffering, pool));
211211
}
212212

‎server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java‎

Lines changed: 46 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -460,7 +460,7 @@ private boolean storagePoolSupportsDiskOffering(StoragePool pool, DiskOffering d
460460
if (diskOffering == null) {
461461
return false;
462462
}
463-
return volumeApiService.doesTargetStorageSupportDiskOffering(pool, diskOffering.getTags());
463+
return volumeApiService.doesStoragePoolSupportDiskOfferingTags(pool, diskOffering.getTags());
464464
}
465465

466466
private ServiceOfferingVO getUnmanagedInstanceServiceOffering(final UnmanagedInstanceTO instance, ServiceOfferingVO serviceOffering, final Account owner, final DataCenter zone, final Map<String, String> details, Hypervisor.HypervisorType hypervisorType)
@@ -550,7 +550,7 @@ private StoragePool getStoragePool(final UnmanagedInstanceTO.Disk disk, final Da
550550
for (StoragePool pool : pools) {
551551
if (pool.getDataCenterId() == zone.getId() &&
552552
(pool.getClusterId() == null || pool.getClusterId().equals(cluster.getId())) &&
553-
volumeApiService.doesTargetStorageSupportDiskOffering(pool, diskOfferingTags)) {
553+
volumeApiService.doesStoragePoolSupportDiskOfferingTags(pool, diskOfferingTags)) {
554554
storagePool = pool;
555555
break;
556556
}
@@ -1768,9 +1768,9 @@ private void sanitizeConvertedInstance(UnmanagedInstanceTO convertedInstance, Un
17681768
convertedInstance.setPowerState(UnmanagedInstanceTO.PowerState.PowerOff);
17691769
List<UnmanagedInstanceTO.Disk> convertedInstanceDisks = convertedInstance.getDisks();
17701770
List<UnmanagedInstanceTO.Disk> sourceVMwareInstanceDisks = sourceVMwareInstance.getDisks();
1771-
for (UnmanagedInstanceTO.Disk sourceVMwareInstanceDisk : sourceVMwareInstanceDisks) {
1772-
UnmanagedInstanceTO.Disk convertedDisk = convertedInstanceDisks.get(sourceVMwareInstanceDisk.getPosition());
1773-
convertedDisk.setDiskId(sourceVMwareInstanceDisk.getDiskId());
1771+
for (int i = 0; i < convertedInstanceDisks.size(); i++) {
1772+
UnmanagedInstanceTO.Disk disk = convertedInstanceDisks.get(i);
1773+
disk.setDiskId(sourceVMwareInstanceDisks.get(i).getDiskId());
17741774
}
17751775
List<UnmanagedInstanceTO.Nic> convertedInstanceNics = convertedInstance.getNics();
17761776
List<UnmanagedInstanceTO.Nic> sourceVMwareInstanceNics = sourceVMwareInstance.getNics();
@@ -2054,7 +2054,25 @@ private List<StoragePoolVO> findInstanceConversionStoragePoolsInCluster(
20542054
List<StoragePoolVO> pools = new ArrayList<>();
20552055
pools.addAll(primaryDataStoreDao.findClusterWideStoragePoolsByHypervisorAndPoolType(destinationCluster.getId(), Hypervisor.HypervisorType.KVM, Storage.StoragePoolType.NetworkFilesystem));
20562056
pools.addAll(primaryDataStoreDao.findZoneWideStoragePoolsByHypervisorAndPoolType(destinationCluster.getDataCenterId(), Hypervisor.HypervisorType.KVM, Storage.StoragePoolType.NetworkFilesystem));
2057-
List<String> diskOfferingTags = new ArrayList<>();
2057+
if (pools.isEmpty()) {
2058+
String msg = String.format("Cannot find suitable storage pools in the cluster %s for the conversion", destinationCluster.getName());
2059+
LOGGER.error(msg);
2060+
throw new CloudRuntimeException(msg);
2061+
}
2062+
2063+
if (serviceOffering.getDiskOfferingId() != null) {
2064+
DiskOfferingVO diskOffering = diskOfferingDao.findById(serviceOffering.getDiskOfferingId());
2065+
if (diskOffering == null) {
2066+
String msg = String.format("Cannot find disk offering with ID %s that belongs to the service offering %s", serviceOffering.getDiskOfferingId(), serviceOffering.getName());
2067+
LOGGER.error(msg);
2068+
throw new CloudRuntimeException(msg);
2069+
}
2070+
if (getStoragePoolWithTags(pools, diskOffering.getTags()) == null) {
2071+
String msg = String.format("Cannot find suitable storage pool for disk offering %s that belongs to the service offering %s", diskOffering.getName(), serviceOffering.getName());
2072+
LOGGER.error(msg);
2073+
throw new CloudRuntimeException(msg);
2074+
}
2075+
}
20582076
for (Long diskOfferingId : dataDiskOfferingMap.values()) {
20592077
DiskOfferingVO diskOffering = diskOfferingDao.findById(diskOfferingId);
20602078
if (diskOffering == null) {
@@ -2099,34 +2117,43 @@ private List<StoragePoolVO> getPoolsWithMatchingTags(List<StoragePoolVO> pools,
20992117
throw new CloudRuntimeException(msg);
21002118
}
21012119
}
2102-
return poolsSupportingTags;
2120+
2121+
return pools;
2122+
}
2123+
2124+
private StoragePoolVO getStoragePoolWithTags(List<StoragePoolVO> pools, String tags) {
2125+
if (StringUtils.isEmpty(tags)) {
2126+
return pools.get(0);
2127+
}
2128+
for (StoragePoolVO pool : pools) {
2129+
if (volumeApiService.doesStoragePoolSupportDiskOfferingTags(pool, tags)) {
2130+
return pool;
2131+
}
2132+
}
2133+
return null;
21032134
}
21042135

21052136
private List<String> selectInstanceConversionStoragePools(
21062137
List<StoragePoolVO> pools, List<UnmanagedInstanceTO.Disk> disks,
21072138
ServiceOfferingVO serviceOffering, Map<String, Long> dataDiskOfferingMap
21082139
) {
21092140
List<String> storagePools = new ArrayList<>(disks.size());
2110-
for (int i = 0; i < disks.size(); i++) {
2111-
storagePools.add(null);
2112-
}
21132141
Set<String> dataDiskIds = dataDiskOfferingMap.keySet();
21142142
for (UnmanagedInstanceTO.Disk disk : disks) {
2115-
Long diskOfferingId = dataDiskOfferingMap.get(disk.getDiskId());
2116-
if (diskOfferingId == null && !dataDiskIds.contains(disk.getDiskId())) {
2143+
Long diskOfferingId = null;
2144+
if (dataDiskIds.contains(disk.getDiskId())) {
2145+
diskOfferingId = dataDiskOfferingMap.get(disk.getDiskId());
2146+
} else {
21172147
diskOfferingId = serviceOffering.getDiskOfferingId();
21182148
}
2149+
21192150
//TODO: Choose pools by capacity
21202151
if (diskOfferingId == null) {
2121-
storagePools.set(disk.getPosition(), pools.get(0).getUuid());
2152+
storagePools.add(pools.get(0).getUuid());
21222153
} else {
21232154
DiskOfferingVO diskOffering = diskOfferingDao.findById(diskOfferingId);
2124-
for (StoragePoolVO pool : pools) {
2125-
if (volumeApiService.doesTargetStorageSupportDiskOffering(pool, diskOffering.getTags())) {
2126-
storagePools.set(disk.getPosition(), pool.getUuid());
2127-
break;
2128-
}
2129-
}
2155+
StoragePoolVO pool = getStoragePoolWithTags(pools, diskOffering.getTags());
2156+
storagePools.add(pool.getUuid());
21302157
}
21312158
}
21322159
return storagePools;

‎server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1178,7 +1178,7 @@ public void doesTargetStorageSupportDiskOfferingTestDiskOfferingMoreTagsThanStor
11781178
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
11791179
Mockito.doReturn(new Pair<>(List.of("A"), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
11801180

1181-
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
1181+
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
11821182

11831183
Assert.assertFalse(result);
11841184
}
@@ -1191,7 +1191,7 @@ public void doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsIsSubSetOfSt
11911191
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
11921192
Mockito.doReturn(new Pair<>(List.of("A","B","C","D","X","Y"), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
11931193

1194-
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
1194+
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
11951195

11961196
Assert.assertTrue(result);
11971197
}
@@ -1204,7 +1204,7 @@ public void doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsEmptyAndStor
12041204
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
12051205
Mockito.lenient().doReturn(new Pair<>(List.of("A,B,C,D,X,Y"), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
12061206

1207-
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
1207+
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
12081208

12091209
Assert.assertTrue(result);
12101210
}
@@ -1217,7 +1217,7 @@ public void doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsNotEmptyAndS
12171217
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
12181218
Mockito.doReturn(new Pair<>(List.of(""), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
12191219

1220-
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
1220+
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
12211221

12221222
Assert.assertFalse(result);
12231223
}
@@ -1230,7 +1230,7 @@ public void doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsEmptyAndStor
12301230
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
12311231
Mockito.lenient().doReturn(new Pair<>(List.of(""), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
12321232

1233-
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
1233+
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
12341234

12351235
Assert.assertTrue(result);
12361236
}
@@ -1243,7 +1243,7 @@ public void doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsDifferentFro
12431243
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
12441244
Mockito.doReturn(new Pair<>(List.of("C,D"), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
12451245

1246-
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
1246+
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
12471247

12481248
Assert.assertFalse(result);
12491249
}
@@ -1256,7 +1256,7 @@ public void doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsEqualsStorag
12561256
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
12571257
Mockito.doReturn(new Pair<>(List.of("A"), false)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
12581258

1259-
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
1259+
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
12601260

12611261
Assert.assertTrue(result);
12621262
}
@@ -1269,7 +1269,7 @@ public void doesTargetStorageSupportDiskOfferingTestStorageRuleTagWithDiskOfferi
12691269
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
12701270
Mockito.doReturn(new Pair<>(List.of("tags[0] == 'A'"), true)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
12711271

1272-
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
1272+
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
12731273

12741274
Assert.assertTrue(result);
12751275
}
@@ -1282,7 +1282,7 @@ public void doesTargetStorageSupportDiskOfferingTestStorageRuleTagWithDiskOfferi
12821282
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
12831283
Mockito.doReturn(new Pair<>(List.of("tags[0] == 'A'"), true)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
12841284

1285-
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
1285+
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
12861286

12871287
Assert.assertFalse(result);
12881288
}
@@ -1295,7 +1295,7 @@ public void doesTargetStorageSupportDiskOfferingTestStorageRuleTagWithNullDiskOf
12951295
StoragePool storagePoolMock = Mockito.mock(StoragePool.class);
12961296
Mockito.doReturn(new Pair<>(List.of("tags[0] == 'A'"), true)).when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock);
12971297

1298-
boolean result = volumeApiServiceImpl.doesTargetStorageSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
1298+
boolean result = volumeApiServiceImpl.doesStoragePoolSupportDiskOffering(storagePoolMock, diskOfferingVoMock);
12991299

13001300
Assert.assertFalse(result);
13011301
}

‎server/src/test/java/org/apache/cloudstack/storage/volume/VolumeImportUnmanageManagerImplTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ public void testImportVolumeAllGood() throws ResourceAllocationException {
275275
when(diskOffering.isCustomized()).thenReturn(true);
276276
doReturn(diskOffering).when(volumeImportUnmanageManager).getOrCreateDiskOffering(account, diskOfferingId, zoneId, isLocal);
277277
doNothing().when(volumeApiService).validateCustomDiskOfferingSizeRange(anyLong());
278-
doReturn(true).when(volumeApiService).doesTargetStorageSupportDiskOffering(any(), isNull());
278+
doReturn(true).when(volumeApiService).doesStoragePoolSupportDiskOfferingTags(any(), isNull());
279279
doReturn(diskProfile).when(volumeManager).importVolume(any(), anyString(), any(), eq(virtualSize), isNull(), isNull(), anyLong(),
280280
any(), isNull(), isNull(), any(), isNull(), anyLong(), anyString(), isNull());
281281
when(diskProfile.getVolumeId()).thenReturn(volumeId);

0 commit comments

Comments
 (0)