Skip to content

Commit 47946db

Browse files
shwstpprharikrishna-patnalayadvrGutoVeronezi
authored
server: fix volume migration on user vm scale (#6704)
Fixes #6701 When volume migration is initiated by system, account check is not needed. Introduces a new global setting - allow.diskoffering.change.during.scale.vm. This determines whether to allow or disallow disk offering change for root volume during scaling of a stopped or running VM. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com> Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com> Co-authored-by: Harikrishna Patnala <harikrishna.patnala@gmail.com> Co-authored-by: Rohit Yadav <rohityadav89@gmail.com> Co-authored-by: Daniel Augusto Veronezi Salvador <38945620+GutoVeronezi@users.noreply.github.com> Co-authored-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
1 parent dbc2032 commit 47946db

File tree

7 files changed

+517
-67
lines changed

7 files changed

+517
-67
lines changed

‎api/src/main/java/com/cloud/server/ManagementService.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -405,7 +405,7 @@ public interface ManagementService {
405405
*/
406406
Pair<List<? extends StoragePool>, List<? extends StoragePool>> listStoragePoolsForMigrationOfVolume(Long volumeId);
407407

408-
Pair<List<? extends StoragePool>, List<? extends StoragePool>> listStoragePoolsForMigrationOfVolumeInternal(Long volumeId, Long newDiskOfferingId, Long newSize, Long newMinIops, Long newMaxIops, boolean keepSourceStoragePool, boolean bypassStorageTypeCheck);
408+
Pair<List<? extends StoragePool>, List<? extends StoragePool>> listStoragePoolsForSystemMigrationOfVolume(Long volumeId, Long newDiskOfferingId, Long newSize, Long newMinIops, Long newMaxIops, boolean keepSourceStoragePool, boolean bypassStorageTypeCheck);
409409

410410
String[] listEventTypes();
411411

‎server/src/main/java/com/cloud/server/ManagementServerImpl.java‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1526,7 +1526,7 @@ private boolean hasSuitablePoolsForVolume(final VolumeVO volume, final Host host
15261526
@Override
15271527
public Pair<List<? extends StoragePool>, List<? extends StoragePool>> listStoragePoolsForMigrationOfVolume(final Long volumeId) {
15281528

1529-
Pair<List<? extends StoragePool>, List<? extends StoragePool>> allPoolsAndSuitablePoolsPair = listStoragePoolsForMigrationOfVolumeInternal(volumeId, null, null, null, null, false, true);
1529+
Pair<List<? extends StoragePool>, List<? extends StoragePool>> allPoolsAndSuitablePoolsPair = listStoragePoolsForMigrationOfVolumeInternal(volumeId, null, null, null, null, false, true, false);
15301530
List<? extends StoragePool> allPools = allPoolsAndSuitablePoolsPair.first();
15311531
List<? extends StoragePool> suitablePools = allPoolsAndSuitablePoolsPair.second();
15321532
List<StoragePool> avoidPools = new ArrayList<>();
@@ -1542,13 +1542,20 @@ public Pair<List<? extends StoragePool>, List<? extends StoragePool>> listStorag
15421542
return new Pair<List<? extends StoragePool>, List<? extends StoragePool>>(allPools, suitablePools);
15431543
}
15441544

1545-
public Pair<List<? extends StoragePool>, List<? extends StoragePool>> listStoragePoolsForMigrationOfVolumeInternal(final Long volumeId, Long newDiskOfferingId, Long newSize, Long newMinIops, Long newMaxIops, boolean keepSourceStoragePool, boolean bypassStorageTypeCheck) {
1546-
final Account caller = getCaller();
1547-
if (!_accountMgr.isRootAdmin(caller.getId())) {
1548-
if (s_logger.isDebugEnabled()) {
1549-
s_logger.debug("Caller is not a root admin, permission denied to migrate the volume");
1545+
@Override
1546+
public Pair<List<? extends StoragePool>, List<? extends StoragePool>> listStoragePoolsForSystemMigrationOfVolume(final Long volumeId, Long newDiskOfferingId, Long newSize, Long newMinIops, Long newMaxIops, boolean keepSourceStoragePool, boolean bypassStorageTypeCheck) {
1547+
return listStoragePoolsForMigrationOfVolumeInternal(volumeId, newDiskOfferingId, newSize, newMinIops, newMaxIops, keepSourceStoragePool, bypassStorageTypeCheck, true);
1548+
}
1549+
1550+
public Pair<List<? extends StoragePool>, List<? extends StoragePool>> listStoragePoolsForMigrationOfVolumeInternal(final Long volumeId, Long newDiskOfferingId, Long newSize, Long newMinIops, Long newMaxIops, boolean keepSourceStoragePool, boolean bypassStorageTypeCheck, boolean bypassAccountCheck) {
1551+
if (!bypassAccountCheck) {
1552+
final Account caller = getCaller();
1553+
if (!_accountMgr.isRootAdmin(caller.getId())) {
1554+
if (s_logger.isDebugEnabled()) {
1555+
s_logger.debug("Caller is not a root admin, permission denied to migrate the volume");
1556+
}
1557+
throw new PermissionDeniedException("No permission to migrate volume, only root admin can migrate a volume");
15501558
}
1551-
throw new PermissionDeniedException("No permission to migrate volume, only root admin can migrate a volume");
15521559
}
15531560

15541561
final VolumeVO volume = _volumeDao.findById(volumeId);

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@
9696
import org.apache.cloudstack.utils.volume.VirtualMachineDiskInfo;
9797
import org.apache.commons.collections.CollectionUtils;
9898
import org.apache.commons.collections.MapUtils;
99+
import org.apache.commons.lang3.ObjectUtils;
99100
import org.apache.commons.lang3.StringUtils;
100101
import org.apache.commons.lang3.builder.ToStringBuilder;
101102
import org.apache.commons.lang3.builder.ToStringStyle;
@@ -1767,14 +1768,14 @@ private Volume changeDiskOfferingForVolumeInternal(VolumeVO volume, Long newDisk
17671768
return volume;
17681769
}
17691770

1770-
if (currentSize != newSize || newMaxIops != volume.getMaxIops() || newMinIops != volume.getMinIops()) {
1771+
if (currentSize != newSize || !compareEqualsIncludingNullOrZero(newMaxIops, volume.getMaxIops()) || !compareEqualsIncludingNullOrZero(newMinIops, volume.getMinIops())) {
17711772
volumeResizeRequired = true;
17721773
validateVolumeReadyStateAndHypervisorChecks(volume, currentSize, newSize);
17731774
}
17741775

17751776
StoragePoolVO existingStoragePool = _storagePoolDao.findById(volume.getPoolId());
17761777

1777-
Pair<List<? extends StoragePool>, List<? extends StoragePool>> poolsPair = managementService.listStoragePoolsForMigrationOfVolumeInternal(volume.getId(), newDiskOffering.getId(), newSize, newMinIops, newMaxIops, true, false);
1778+
Pair<List<? extends StoragePool>, List<? extends StoragePool>> poolsPair = managementService.listStoragePoolsForSystemMigrationOfVolume(volume.getId(), newDiskOffering.getId(), newSize, newMinIops, newMaxIops, true, false);
17781779
List<? extends StoragePool> suitableStoragePools = poolsPair.second();
17791780

17801781
if (!suitableStoragePools.stream().anyMatch(p -> (p.getId() == existingStoragePool.getId()))) {
@@ -1823,6 +1824,21 @@ private Volume changeDiskOfferingForVolumeInternal(VolumeVO volume, Long newDisk
18231824
return volume;
18241825
}
18251826

1827+
/**
1828+
* This method is to compare long values, in miniops and maxiops a or b can be null or 0.
1829+
* Use this method to treat 0 and null as same
1830+
*
1831+
* @param a
1832+
* @param b
1833+
* @return true if a and b are equal excluding 0 and null values.
1834+
*/
1835+
private boolean compareEqualsIncludingNullOrZero(Long a, Long b) {
1836+
a = ObjectUtils.defaultIfNull(a, 0L);
1837+
b = ObjectUtils.defaultIfNull(b, 0L);
1838+
1839+
return a.equals(b);
1840+
}
1841+
18261842
/**
18271843
* Returns true if the new disk offering is the same than current offering, and the respective Service offering is a custom (constraint or unconstraint) offering.
18281844
*/

‎server/src/main/java/com/cloud/vm/UserVmManager.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,12 @@
4747
*/
4848
public interface UserVmManager extends UserVmService {
4949
String EnableDynamicallyScaleVmCK = "enable.dynamic.scale.vm";
50+
String AllowDiskOfferingChangeDuringScaleVmCK = "allow.diskoffering.change.during.scale.vm";
5051
String AllowUserExpungeRecoverVmCK ="allow.user.expunge.recover.vm";
5152
ConfigKey<Boolean> EnableDynamicallyScaleVm = new ConfigKey<Boolean>("Advanced", Boolean.class, EnableDynamicallyScaleVmCK, "false",
5253
"Enables/Disables dynamically scaling a vm", true, ConfigKey.Scope.Zone);
54+
ConfigKey<Boolean> AllowDiskOfferingChangeDuringScaleVm = new ConfigKey<Boolean>("Advanced", Boolean.class, AllowDiskOfferingChangeDuringScaleVmCK, "false",
55+
"Determines whether to allow or disallow disk offering change for root volume during scaling of a stopped or running vm", true, ConfigKey.Scope.Zone);
5356
ConfigKey<Boolean> AllowUserExpungeRecoverVm = new ConfigKey<Boolean>("Advanced", Boolean.class, AllowUserExpungeRecoverVmCK, "false",
5457
"Determines whether users can expunge or recover their vm", true, ConfigKey.Scope.Account);
5558
ConfigKey<Boolean> DisplayVMOVFProperties = new ConfigKey<Boolean>("Advanced", Boolean.class, "vm.display.ovf.properties", "false",

‎server/src/main/java/com/cloud/vm/UserVmManagerImpl.java‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1241,7 +1241,7 @@ private UserVm upgradeStoppedVirtualMachine(Long vmId, Long svcOffId, Map<String
12411241

12421242
// resize and migrate the root volume if required
12431243
DiskOfferingVO newDiskOffering = _diskOfferingDao.findById(newServiceOffering.getDiskOfferingId());
1244-
changeDiskOfferingForRootVolume(vmId, newDiskOffering, customParameters);
1244+
changeDiskOfferingForRootVolume(vmId, newDiskOffering, customParameters, vmInstance.getDataCenterId());
12451245

12461246
_itMgr.upgradeVmDb(vmId, newServiceOffering, currentServiceOffering);
12471247

@@ -2000,7 +2000,7 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
20002000

20012001
// #3 resize or migrate the root volume if required
20022002
DiskOfferingVO newDiskOffering = _diskOfferingDao.findById(newServiceOffering.getDiskOfferingId());
2003-
changeDiskOfferingForRootVolume(vmId, newDiskOffering, customParameters);
2003+
changeDiskOfferingForRootVolume(vmId, newDiskOffering, customParameters, vmInstance.getDataCenterId());
20042004

20052005
// #4 scale the vm now
20062006
vmInstance = _vmInstanceDao.findById(vmId);
@@ -2036,7 +2036,14 @@ private void validateDiskOfferingChecks(ServiceOfferingVO currentServiceOffering
20362036
}
20372037
}
20382038

2039-
private void changeDiskOfferingForRootVolume(Long vmId, DiskOfferingVO newDiskOffering, Map<String, String> customParameters) throws ResourceAllocationException {
2039+
private void changeDiskOfferingForRootVolume(Long vmId, DiskOfferingVO newDiskOffering, Map<String, String> customParameters, Long zoneId) throws ResourceAllocationException {
2040+
2041+
if (!AllowDiskOfferingChangeDuringScaleVm.valueIn(zoneId)) {
2042+
if (s_logger.isDebugEnabled()) {
2043+
s_logger.debug(String.format("Changing the disk offering of the root volume during the compute offering change operation is disabled. Please check the setting [%s].", AllowDiskOfferingChangeDuringScaleVm.key()));
2044+
}
2045+
return;
2046+
}
20402047

20412048
List<VolumeVO> vols = _volsDao.findReadyAndAllocatedRootVolumesByInstance(vmId);
20422049

@@ -7791,7 +7798,7 @@ public String getConfigComponentName() {
77917798

77927799
@Override
77937800
public ConfigKey<?>[] getConfigKeys() {
7794-
return new ConfigKey<?>[] {EnableDynamicallyScaleVm, AllowUserExpungeRecoverVm, VmIpFetchWaitInterval, VmIpFetchTrialMax,
7801+
return new ConfigKey<?>[] {EnableDynamicallyScaleVm, AllowDiskOfferingChangeDuringScaleVm, AllowUserExpungeRecoverVm, VmIpFetchWaitInterval, VmIpFetchTrialMax,
77957802
VmIpFetchThreadPoolMax, VmIpFetchTaskWorkers, AllowDeployVmIfGivenHostFails, EnableAdditionalVmConfig, DisplayVMOVFProperties,
77967803
KvmAdditionalConfigAllowList, XenServerAdditionalConfigAllowList, VmwareAdditionalConfigAllowList};
77977804
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -794,7 +794,7 @@ private UserVm migrateImportedVM(HostVO sourceHost, VirtualMachineTemplate templ
794794
continue;
795795
}
796796
LOGGER.debug(String.format("Volume %s needs to be migrated", volumeVO.getUuid()));
797-
Pair<List<? extends StoragePool>, List<? extends StoragePool>> poolsPair = managementService.listStoragePoolsForMigrationOfVolumeInternal(profile.getVolumeId(), null, null, null, null, false, true);
797+
Pair<List<? extends StoragePool>, List<? extends StoragePool>> poolsPair = managementService.listStoragePoolsForSystemMigrationOfVolume(profile.getVolumeId(), null, null, null, null, false, true);
798798
if (CollectionUtils.isEmpty(poolsPair.first()) && CollectionUtils.isEmpty(poolsPair.second())) {
799799
cleanupFailedImportVM(vm);
800800
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("VM import failed for unmanaged vm: %s during volume ID: %s migration as no suitable pool(s) found", userVm.getInstanceName(), volumeVO.getUuid()));

0 commit comments

Comments
 (0)