From f991e4e5c6d9c593476d266cc831a944708b5884 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Fri, 30 Sep 2022 01:56:39 +0530 Subject: [PATCH 1/9] server, api, ui: allow host auto-select for migrateVirtualMachineWithVolume Fixes #6773 Signed-off-by: Abhishek Kumar --- .../MigrateVirtualMachineWithVolumeCmd.java | 27 +++++--- .../java/com/cloud/vm/UserVmManagerImpl.java | 68 ++++++++++++------- ui/src/views/compute/MigrateWizard.vue | 31 ++++++++- 3 files changed, 91 insertions(+), 35 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java index db513ff4dc0f..915b79fd3917 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java @@ -83,6 +83,12 @@ public class MigrateVirtualMachineWithVolumeCmd extends BaseAsyncCmd { "<1b331390-59f2-4796-9993-bf11c6e76225>&migrateto[2].pool=<41fdb564-9d3b-447d-88ed-7628f7640cbc>") private Map migrateVolumeTo; + @Parameter(name = ApiConstants.AUTO_SELECT, + since = "4.18.0", + type = CommandType.BOOLEAN, + description = "Automatically select a destination host for a running instance, if hostId and storageId are not specified. false by default") + private Boolean autoSelect; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// @@ -160,21 +166,26 @@ public void execute() { throw new InvalidParameterValueException("Unable to find the VM by id=" + getVirtualMachineId()); } - if (!VirtualMachine.State.Running.equals(userVm.getState()) && hostId != null) { + if (!VirtualMachine.State.Running.equals(userVm.getState()) && (hostId != null || Boolean.TRUE.equals(autoSelect))) { throw new InvalidParameterValueException(String.format("VM ID: %s is not in Running state to migrate it to new host", userVm.getUuid())); } - if (!VirtualMachine.State.Stopped.equals(userVm.getState()) && hostId == null) { - throw new InvalidParameterValueException(String.format("VM ID: %s is not in Stopped state to migrate, use %s parameter to migrate it to a new host", userVm.getUuid(), ApiConstants.HOST_ID)); + if (!VirtualMachine.State.Stopped.equals(userVm.getState()) && hostId == null && Boolean.FALSE.equals(autoSelect)) { + throw new InvalidParameterValueException(String.format("VM ID: %s is not in Stopped state to migrate, use %s or %s parameter to migrate it to a new host", + userVm.getUuid(), ApiConstants.HOST_ID,ApiConstants.AUTO_SELECT)); } try { VirtualMachine migratedVm = null; - if (hostId != null) { - Host destinationHost = _resourceService.getHost(getHostId()); - // OfflineVmwareMigration: destination host would have to not be a required parameter for stopped VMs - if (destinationHost == null) { - throw new InvalidParameterValueException("Unable to find the host to migrate the VM, host id =" + getHostId()); + + if (getHostId() != null || Boolean.TRUE.equals(autoSelect)) { + Host destinationHost = null; + if (Boolean.FALSE.equals(autoSelect)) { + destinationHost = _resourceService.getHost(getHostId()); + // OfflineVmwareMigration: destination host would have to not be a required parameter for stopped VMs + if (destinationHost == null) { + throw new InvalidParameterValueException("Unable to find the host to migrate the VM, host id =" + getHostId()); + } } migratedVm = _userVmService.migrateVirtualMachineWithVolume(getVirtualMachineId(), destinationHost, getVolumeToPool()); } else if (MapUtils.isNotEmpty(migrateVolumeTo)) { diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index e1397c4fbce6..025d26a89e6d 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -51,7 +51,6 @@ import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.ParserConfigurationException; -import com.cloud.resourcelimit.CheckedReservation; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.ControlledEntity.ACLType; import org.apache.cloudstack.acl.SecurityChecker.AccessType; @@ -277,6 +276,7 @@ import com.cloud.org.Grouping; import com.cloud.resource.ResourceManager; import com.cloud.resource.ResourceState; +import com.cloud.resourcelimit.CheckedReservation; import com.cloud.server.ManagementService; import com.cloud.server.ResourceTag; import com.cloud.server.StatsCollector; @@ -6265,22 +6265,8 @@ private void checkDestinationHypervisorType(StoragePool destPool, VMInstanceVO v } public boolean isVMUsingLocalStorage(VMInstanceVO vm) { - boolean usesLocalStorage = false; - List volumes = _volsDao.findByInstance(vm.getId()); - for (VolumeVO vol : volumes) { - DiskOfferingVO diskOffering = _diskOfferingDao.findById(vol.getDiskOfferingId()); - if (diskOffering.isUseLocalStorage()) { - usesLocalStorage = true; - break; - } - StoragePoolVO storagePool = _storagePoolDao.findById(vol.getPoolId()); - if (storagePool.isLocal()) { - usesLocalStorage = true; - break; - } - } - return usesLocalStorage; + return isVmVolumesUsingLocalStorage(volumes); } @Override @@ -6339,7 +6325,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr DeployDestination dest = null; if (destinationHost == null) { - dest = chooseVmMigrationDestination(vm, srcHost); + dest = chooseVmMigrationDestination(vm, srcHost, null); } else { dest = checkVmMigrationDestination(vm, srcHost, destinationHost); } @@ -6354,7 +6340,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr return findMigratedVm(vm.getId(), vm.getType()); } - private DeployDestination chooseVmMigrationDestination(VMInstanceVO vm, Host srcHost) { + private DeployDestination chooseVmMigrationDestination(VMInstanceVO vm, Host srcHost, Long poolId) { vm.setLastHostId(null); // Last host does not have higher priority in vm migration final ServiceOfferingVO offering = _offeringDao.findById(vm.getId(), vm.getServiceOfferingId()); final VirtualMachineProfile profile = new VirtualMachineProfileImpl(vm, null, offering, null, null); @@ -6362,12 +6348,12 @@ private DeployDestination chooseVmMigrationDestination(VMInstanceVO vm, Host src final Host host = _hostDao.findById(srcHostId); ExcludeList excludes = new ExcludeList(); excludes.addHost(srcHostId); - final DataCenterDeployment plan = _itMgr.getMigrationDeployment(vm, host, null, excludes); + final DataCenterDeployment plan = _itMgr.getMigrationDeployment(vm, host, poolId, excludes); try { return _planningMgr.planDeployment(profile, plan, excludes, null); } catch (final AffinityConflictException e2) { - s_logger.warn("Unable to create deployment, affinity rules associted to the VM conflict", e2); - throw new CloudRuntimeException("Unable to create deployment, affinity rules associted to the VM conflict"); + s_logger.warn("Unable to create deployment, affinity rules associated to the VM conflict", e2); + throw new CloudRuntimeException("Unable to create deployment, affinity rules associated to the VM conflict"); } catch (final InsufficientServerCapacityException e3) { throw new CloudRuntimeException("Unable to find a server to migrate the vm to"); } @@ -6662,8 +6648,23 @@ private boolean isImplicitPlannerUsedByOffering(long offeringId) { return implicitPlannerUsed; } - private boolean isVmVolumesOnZoneWideStore(VMInstanceVO vm) { - final List volumes = _volsDao.findCreatedByInstance(vm.getId()); + private boolean isVmVolumesUsingLocalStorage(final List volumes) { + if (CollectionUtils.isEmpty(volumes)) { + return false; + } + for (Volume volume : volumes) { + if (volume == null || volume.getPoolId() == null) { + return false; + } + StoragePoolVO pool = _storagePoolDao.findById(volume.getPoolId()); + if (pool == null || !ScopeType.ZONE.equals(pool.getScope())) { + return false; + } + } + return true; + } + + private boolean isVmVolumesOnZoneWideStore(final List volumes) { if (CollectionUtils.isEmpty(volumes)) { return false; } @@ -6687,6 +6688,10 @@ private Pair getHostsForMigrateVmWithStorage(VMInstanceVO vm, Host d throw new InvalidParameterValueException("Cannot migrate VM, host with ID: " + srcHostId + " for VM not found"); } + if (destinationHost == null) { + return new Pair<>(srcHost, null); + } + // Check if source and destination hosts are valid and migrating to same host if (destinationHost.getId() == srcHostId) { throw new InvalidParameterValueException(String.format("Cannot migrate VM as it is already present on host %s (ID: %s), please specify valid destination host to migrate the VM", @@ -6850,8 +6855,9 @@ public VirtualMachine migrateVirtualMachineWithVolume(Long vmId, Host destinatio Pair sourceDestinationHosts = getHostsForMigrateVmWithStorage(vm, destinationHost); Host srcHost = sourceDestinationHosts.first(); - if (!isVMUsingLocalStorage(vm) && MapUtils.isEmpty(volumeToPool) - && (destinationHost.getClusterId().equals(srcHost.getClusterId()) || isVmVolumesOnZoneWideStore(vm))){ + final List volumes = _volsDao.findCreatedByInstance(vm.getId()); + if (!isVmVolumesUsingLocalStorage(volumes) && MapUtils.isEmpty(volumeToPool) && destinationHost != null + && (destinationHost.getClusterId().equals(srcHost.getClusterId()) || isVmVolumesOnZoneWideStore(volumes))) { // If volumes do not have to be migrated // call migrateVirtualMachine for non-user VMs else throw exception if (!VirtualMachine.Type.User.equals(vm.getType())) { @@ -6863,6 +6869,18 @@ public VirtualMachine migrateVirtualMachineWithVolume(Long vmId, Host destinatio Map volToPoolObjectMap = getVolumePoolMappingForMigrateVmWithStorage(vm, volumeToPool); + if (destinationHost == null) { + Long poolId = null; + if (MapUtils.isNotEmpty(volToPoolObjectMap)) { + poolId = new ArrayList<>(volToPoolObjectMap.values()).get(0); + } + DeployDestination deployDestination = chooseVmMigrationDestination(vm, srcHost, poolId); + if (deployDestination == null) { + throw new CloudRuntimeException("Unable to find suitable destination to migrate VM " + vm.getInstanceName()); + } + destinationHost = deployDestination.getHost(); + } + checkHostsDedication(vm, srcHost.getId(), destinationHost.getId()); _itMgr.migrateWithStorage(vm.getUuid(), srcHost.getId(), destinationHost.getId(), volToPoolObjectMap); diff --git a/ui/src/views/compute/MigrateWizard.vue b/ui/src/views/compute/MigrateWizard.vue index 9687138a952d..f4990b7f0e94 100644 --- a/ui/src/views/compute/MigrateWizard.vue +++ b/ui/src/views/compute/MigrateWizard.vue @@ -186,7 +186,8 @@ export default { } ], migrateWithStorage: false, - volumeToPoolSelection: [] + volumeToPoolSelection: [], + volumes: [] } }, created () { @@ -246,6 +247,7 @@ export default { handleSelectedHostChange (host) { if (host.id === -1) { this.migrateWithStorage = false + this.fetchVolumes() } this.selectedHost = host this.selectedVolumeForStoragePoolSelection = {} @@ -257,6 +259,31 @@ export default { handleVolumeToPoolChange (volumeToPool) { this.volumeToPoolSelection = volumeToPool }, + fetchVolumes () { + this.loading = true + this.volumes = [] + api('listVolumes', { + listAll: true, + virtualmachineid: this.resource.id + }).then(response => { + this.volumes = response.listvolumesresponse.volume + }).finally(() => { + this.loading = false + }) + }, + requiresStorageMigration () { + if (this.selectedHost.requiresStorageMotion || this.volumeToPoolSelection.length > 0) { + return true + } + if (this.selectedHost.id === -1 && this.volumes && this.volumes.length > 0) { + for (var volume of this.volumes) { + if (volume.storagetype === 'local') { + return true + } + } + } + return false + }, handleKeyboardSubmit () { if (this.selectedHost.id) { this.submitForm() @@ -269,7 +296,7 @@ export default { if (this.loading) return this.loading = true const migrateApi = this.isUserVm - ? (this.selectedHost.requiresStorageMotion || this.volumeToPoolSelection.length > 0) + ? this.requiresStorageMigration() ? 'migrateVirtualMachineWithVolume' : 'migrateVirtualMachine' : 'migrateSystemVm' From 4b4404f7a6c393b81f9f8258fa928b21add62b2f Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Fri, 30 Sep 2022 02:00:50 +0530 Subject: [PATCH 2/9] fix Signed-off-by: Abhishek Kumar --- .../command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java index 915b79fd3917..bc6cf1a67ad9 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java @@ -86,7 +86,7 @@ public class MigrateVirtualMachineWithVolumeCmd extends BaseAsyncCmd { @Parameter(name = ApiConstants.AUTO_SELECT, since = "4.18.0", type = CommandType.BOOLEAN, - description = "Automatically select a destination host for a running instance, if hostId and storageId are not specified. false by default") + description = "Automatically select a destination host for a running instance, if hostId is not specified. false by default") private Boolean autoSelect; ///////////////////////////////////////////////////// From 0f997aa055603c70f198639ba68d3db2cdaa5335 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Tue, 23 May 2023 17:49:26 +0530 Subject: [PATCH 3/9] fix Signed-off-by: Abhishek Kumar --- .../MigrateVirtualMachineWithVolumeCmd.java | 2 +- .../java/com/cloud/vm/UserVmManagerImpl.java | 51 ++++++++++--------- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java index f7b855d95895..7656b7cf6a73 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java @@ -84,7 +84,7 @@ public class MigrateVirtualMachineWithVolumeCmd extends BaseAsyncCmd { private Map migrateVolumeTo; @Parameter(name = ApiConstants.AUTO_SELECT, - since = "4.18.0", + since = "4.19.0", type = CommandType.BOOLEAN, description = "Automatically select a destination host for a running instance, if hostId is not specified. false by default") private Boolean autoSelect; diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index e66303997265..a4472fb1882d 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -129,6 +129,7 @@ import org.apache.commons.collections.CollectionUtils; import org.apache.commons.collections.MapUtils; import org.apache.commons.lang.math.NumberUtils; +import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.builder.ToStringBuilder; import org.apache.commons.lang3.builder.ToStringStyle; @@ -6355,7 +6356,7 @@ private void checkDestinationHypervisorType(StoragePool destPool, VMInstanceVO v public boolean isVMUsingLocalStorage(VMInstanceVO vm) { List volumes = _volsDao.findByInstance(vm.getId()); - return isVmVolumesUsingLocalStorage(volumes); + return isAnyVmVolumeUsingLocalStorage(volumes); } @Override @@ -6737,23 +6738,21 @@ private boolean isImplicitPlannerUsedByOffering(long offeringId) { return implicitPlannerUsed; } - private boolean isVmVolumesUsingLocalStorage(final List volumes) { - if (CollectionUtils.isEmpty(volumes)) { - return false; - } - for (Volume volume : volumes) { - if (volume == null || volume.getPoolId() == null) { - return false; + private boolean isAnyVmVolumeUsingLocalStorage(final List volumes) { + for (VolumeVO vol : volumes) { + DiskOfferingVO diskOffering = _diskOfferingDao.findById(vol.getDiskOfferingId()); + if (diskOffering.isUseLocalStorage()) { + return true; } - StoragePoolVO pool = _storagePoolDao.findById(volume.getPoolId()); - if (pool == null || !ScopeType.ZONE.equals(pool.getScope())) { - return false; + StoragePoolVO storagePool = _storagePoolDao.findById(vol.getPoolId()); + if (storagePool.isLocal()) { + return true; } } - return true; + return false; } - private boolean isVmVolumesOnZoneWideStore(final List volumes) { + private boolean isAllVmVolumesOnZoneWideStore(final List volumes) { if (CollectionUtils.isEmpty(volumes)) { return false; } @@ -6900,6 +6899,18 @@ private Map getVolumePoolMappingForMigrateVmWithStorage(VMInstanceVO return volToPoolObjectMap; } + protected Host chooseVmMigrationDestination(VMInstanceVO vm, Host srcHost, Map volToPoolObjectMap) { + Long poolId = null; + if (MapUtils.isNotEmpty(volToPoolObjectMap)) { + poolId = new ArrayList<>(volToPoolObjectMap.values()).get(0); + } + DeployDestination deployDestination = chooseVmMigrationDestination(vm, srcHost, poolId); + if (ObjectUtils.anyNull(deployDestination, deployDestination.getHost())) { + throw new CloudRuntimeException("Unable to find suitable destination to migrate VM " + vm.getInstanceName()); + } + return deployDestination.getHost(); + } + @Override @ActionEvent(eventType = EventTypes.EVENT_VM_MIGRATE, eventDescription = "migrating VM", async = true) public VirtualMachine migrateVirtualMachineWithVolume(Long vmId, Host destinationHost, Map volumeToPool) throws ResourceUnavailableException, @@ -6945,8 +6956,8 @@ public VirtualMachine migrateVirtualMachineWithVolume(Long vmId, Host destinatio Host srcHost = sourceDestinationHosts.first(); final List volumes = _volsDao.findCreatedByInstance(vm.getId()); - if (!isVmVolumesUsingLocalStorage(volumes) && MapUtils.isEmpty(volumeToPool) && destinationHost != null - && (destinationHost.getClusterId().equals(srcHost.getClusterId()) || isVmVolumesOnZoneWideStore(volumes))) { + if (!isAnyVmVolumeUsingLocalStorage(volumes) && MapUtils.isEmpty(volumeToPool) && destinationHost != null + && (destinationHost.getClusterId().equals(srcHost.getClusterId()) || isAllVmVolumesOnZoneWideStore(volumes))) { // If volumes do not have to be migrated // call migrateVirtualMachine for non-user VMs else throw exception if (!VirtualMachine.Type.User.equals(vm.getType())) { @@ -6959,15 +6970,7 @@ public VirtualMachine migrateVirtualMachineWithVolume(Long vmId, Host destinatio Map volToPoolObjectMap = getVolumePoolMappingForMigrateVmWithStorage(vm, volumeToPool); if (destinationHost == null) { - Long poolId = null; - if (MapUtils.isNotEmpty(volToPoolObjectMap)) { - poolId = new ArrayList<>(volToPoolObjectMap.values()).get(0); - } - DeployDestination deployDestination = chooseVmMigrationDestination(vm, srcHost, poolId); - if (deployDestination == null) { - throw new CloudRuntimeException("Unable to find suitable destination to migrate VM " + vm.getInstanceName()); - } - destinationHost = deployDestination.getHost(); + destinationHost = chooseVmMigrationDestination(vm, srcHost, volToPoolObjectMap); } checkHostsDedication(vm, srcHost.getId(), destinationHost.getId()); From ca89f2a53a71f090d12c7644fb59ecb47b1fb9cd Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Tue, 23 May 2023 23:43:09 +0530 Subject: [PATCH 4/9] changes Signed-off-by: Abhishek Kumar --- .../MigrateVirtualMachineWithVolumeCmd.java | 9 +- ...igrateVirtualMachineWithVolumeCmdTest.java | 90 +++++++++++-------- 2 files changed, 55 insertions(+), 44 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java index 7656b7cf6a73..c618d222810a 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java @@ -158,25 +158,24 @@ public void execute() { VirtualMachine virtualMachine = _userVmService.getVm(getVirtualMachineId()); if (!VirtualMachine.State.Running.equals(virtualMachine.getState()) && (hostId != null || Boolean.TRUE.equals(autoSelect))) { - throw new InvalidParameterValueException(String.format("%s is not in the Running state to migrate it to a new host", virtualMachine)); + throw new InvalidParameterValueException(String.format("%s is not in the Running state to migrate it to the new host.", virtualMachine)); } if (!VirtualMachine.State.Stopped.equals(virtualMachine.getState()) && hostId == null && Boolean.FALSE.equals(autoSelect)) { - throw new InvalidParameterValueException(String.format("%s is not in the Stopped state to migrate, use %s or %s parameter to migrate it to a new host", + throw new InvalidParameterValueException(String.format("%s is not in the Stopped state to migrate, use the %s or %s parameter to migrate it to a new host.", virtualMachine, ApiConstants.HOST_ID,ApiConstants.AUTO_SELECT)); } try { VirtualMachine migratedVm = null; - if (getHostId() != null || Boolean.TRUE.equals(autoSelect)) { Host destinationHost = null; - if (Boolean.FALSE.equals(autoSelect)) { + if (!Boolean.TRUE.equals(autoSelect)) { destinationHost = _resourceService.getHost(getHostId()); // OfflineVmwareMigration: destination host would have to not be a required parameter for stopped VMs if (destinationHost == null) { s_logger.error(String.format("Unable to find the host with ID [%s].", getHostId())); - throw new InvalidParameterValueException("Unable to find the host to migrate the VM, host id =" + getHostId()); + throw new InvalidParameterValueException("Unable to find the specified host to migrate the VM."); } } migratedVm = _userVmService.migrateVirtualMachineWithVolume(getVirtualMachineId(), destinationHost, getVolumeToPool()); diff --git a/api/src/test/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmdTest.java b/api/src/test/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmdTest.java index 7a98626ca45c..a4556d4eb125 100644 --- a/api/src/test/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmdTest.java +++ b/api/src/test/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmdTest.java @@ -16,16 +16,10 @@ // under the License. package org.apache.cloudstack.api.command.admin.vm; -import com.cloud.exception.InvalidParameterValueException; -import com.cloud.exception.ManagementServerException; -import com.cloud.exception.ResourceUnavailableException; -import com.cloud.exception.VirtualMachineMigrationException; -import com.cloud.host.Host; -import com.cloud.resource.ResourceService; -import com.cloud.uservm.UserVm; -import com.cloud.utils.db.UUIDManager; -import com.cloud.vm.UserVmService; -import com.cloud.vm.VirtualMachine; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.ResponseGenerator; import org.apache.cloudstack.api.ResponseObject; @@ -40,13 +34,21 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.Spy; -import org.powermock.modules.junit4.PowerMockRunner; +import org.mockito.junit.MockitoJUnitRunner; import org.springframework.test.util.ReflectionTestUtils; -import java.util.List; -import java.util.Map; +import com.cloud.exception.InvalidParameterValueException; +import com.cloud.exception.ManagementServerException; +import com.cloud.exception.ResourceUnavailableException; +import com.cloud.exception.VirtualMachineMigrationException; +import com.cloud.host.Host; +import com.cloud.resource.ResourceService; +import com.cloud.uservm.UserVm; +import com.cloud.utils.db.UUIDManager; +import com.cloud.vm.UserVmService; +import com.cloud.vm.VirtualMachine; -@RunWith(PowerMockRunner.class) +@RunWith(MockitoJUnitRunner.class) public class MigrateVirtualMachineWithVolumeCmdTest { @Mock UserVmService userVmServiceMock; @@ -68,24 +70,25 @@ public class MigrateVirtualMachineWithVolumeCmdTest { @Spy @InjectMocks - MigrateVirtualMachineWithVolumeCmd cmdSpy = new MigrateVirtualMachineWithVolumeCmd(); + MigrateVirtualMachineWithVolumeCmd cmdSpy; private Long hostId = 1L; - private Long virtualMachineUuid = 1L; + private Long virtualMachineId = 1L; private String virtualMachineName = "VM-name"; - private Map migrateVolumeTo = Map.of("key","value"); + private Map migrateVolumeTo = null; private SystemVmResponse systemVmResponse = new SystemVmResponse(); private UserVmResponse userVmResponse = new UserVmResponse(); @Before - public void setup() { - Mockito.when(cmdSpy.getVirtualMachineId()).thenReturn(virtualMachineUuid); - Mockito.when(cmdSpy.getHostId()).thenReturn(hostId); - Mockito.when(cmdSpy.getVolumeToPool()).thenReturn(migrateVolumeTo); + public void setUp() throws Exception { + ReflectionTestUtils.setField(cmdSpy, "virtualMachineId", virtualMachineId); + migrateVolumeTo = new HashMap<>(); + migrateVolumeTo.put("volume", "abc"); + migrateVolumeTo.put("pool", "xyz"); } @Test - public void executeTestHostIdIsNullAndMigrateVolumeToIsNullThrowsInvalidParameterValueException(){ + public void executeTestHostIdIsNullAndMigrateVolumeToIsNullThrowsInvalidParameterValueException() { ReflectionTestUtils.setField(cmdSpy, "hostId", null); ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", null); @@ -99,13 +102,13 @@ public void executeTestHostIdIsNullAndMigrateVolumeToIsNullThrowsInvalidParamete } @Test - public void executeTestVMIsStoppedAndHostIdIsNotNullThrowsInvalidParameterValueException(){ + public void executeTestVMIsStoppedAndHostIdIsNotNullThrowsInvalidParameterValueException() { ReflectionTestUtils.setField(cmdSpy, "hostId", hostId); ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", migrateVolumeTo); Mockito.when(userVmServiceMock.getVm(Mockito.anyLong())).thenReturn(virtualMachineMock); Mockito.when(virtualMachineMock.getState()).thenReturn(VirtualMachine.State.Stopped); - Mockito.when(virtualMachineMock.toString()).thenReturn(String.format("VM [uuid: %s, name: %s]", virtualMachineUuid, virtualMachineName)); + Mockito.when(virtualMachineMock.toString()).thenReturn(String.format("VM [uuid: %s, name: %s]", virtualMachineId, virtualMachineName)); try { cmdSpy.execute(); @@ -117,33 +120,35 @@ public void executeTestVMIsStoppedAndHostIdIsNotNullThrowsInvalidParameterValueE } @Test - public void executeTestVMIsRunningAndHostIdIsNullThrowsInvalidParameterValueException(){ + public void executeTestVMIsRunningAndHostIdIsNullThrowsInvalidParameterValueException() { ReflectionTestUtils.setField(cmdSpy, "hostId", null); + ReflectionTestUtils.setField(cmdSpy, "autoSelect", false); ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", migrateVolumeTo); Mockito.when(userVmServiceMock.getVm(Mockito.anyLong())).thenReturn(virtualMachineMock); Mockito.when(virtualMachineMock.getState()).thenReturn(VirtualMachine.State.Running); - Mockito.when(virtualMachineMock.toString()).thenReturn(String.format("VM [uuid: %s, name: %s]", virtualMachineUuid, virtualMachineName)); + Mockito.when(virtualMachineMock.toString()).thenReturn(String.format("VM [uuid: %s, name: %s]", virtualMachineId, virtualMachineName)); try { cmdSpy.execute(); } catch (Exception e) { Assert.assertEquals(InvalidParameterValueException.class, e.getClass()); - String expected = String.format("%s is not in the Stopped state to migrate, use the %s parameter to migrate it to a new host.", virtualMachineMock, - ApiConstants.HOST_ID); + String expected = String.format("%s is not in the Stopped state to migrate, use the %s or %s parameter to migrate it to a new host.", virtualMachineMock, + ApiConstants.HOST_ID, ApiConstants.AUTO_SELECT); Assert.assertEquals(expected , e.getMessage()); } } @Test - public void executeTestHostIdIsNullThrowsInvalidParameterValueException(){ + public void executeTestHostIdIsNullThrowsInvalidParameterValueException() { + ReflectionTestUtils.setField(cmdSpy, "virtualMachineId", virtualMachineId); ReflectionTestUtils.setField(cmdSpy, "hostId", hostId); ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", migrateVolumeTo); + ReflectionTestUtils.setField(cmdSpy, "autoSelect", false); Mockito.when(userVmServiceMock.getVm(Mockito.anyLong())).thenReturn(virtualMachineMock); Mockito.when(virtualMachineMock.getState()).thenReturn(VirtualMachine.State.Running); Mockito.when(resourceServiceMock.getHost(Mockito.anyLong())).thenReturn(null); - Mockito.when(uuidManagerMock.getUuid(Host.class, virtualMachineUuid)).thenReturn(virtualMachineUuid.toString()); try { cmdSpy.execute(); @@ -154,15 +159,22 @@ public void executeTestHostIdIsNullThrowsInvalidParameterValueException(){ } } + private Map getMockedMigrateVolumeToApiCmdParam() { + Map migrateVolumeTo = new HashMap<>(); + migrateVolumeTo.put("volume", "abc"); + migrateVolumeTo.put("pool", "xyz"); + return Map.of("", migrateVolumeTo); + } + @Test public void executeTestHostIsNotNullMigratedVMIsNullThrowsServerApiException() throws ManagementServerException, ResourceUnavailableException, VirtualMachineMigrationException { ReflectionTestUtils.setField(cmdSpy, "hostId", hostId); - ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", migrateVolumeTo); + ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", getMockedMigrateVolumeToApiCmdParam()); Mockito.when(userVmServiceMock.getVm(Mockito.anyLong())).thenReturn(virtualMachineMock); Mockito.when(virtualMachineMock.getState()).thenReturn(VirtualMachine.State.Running); - Mockito.when(resourceServiceMock.getHost(Mockito.anyLong())).thenReturn(hostMock); - Mockito.when(userVmServiceMock.migrateVirtualMachineWithVolume(virtualMachineUuid, hostMock, migrateVolumeTo)).thenReturn(null); + Mockito.when(resourceServiceMock.getHost(hostId)).thenReturn(hostMock); + Mockito.when(userVmServiceMock.migrateVirtualMachineWithVolume(Mockito.anyLong(), Mockito.any(), Mockito.anyMap())).thenReturn(null); try { cmdSpy.execute(); @@ -176,11 +188,11 @@ public void executeTestHostIsNotNullMigratedVMIsNullThrowsServerApiException() t @Test public void executeTestHostIsNullMigratedVMIsNullThrowsServerApiException() { ReflectionTestUtils.setField(cmdSpy, "hostId", null); - ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", migrateVolumeTo); + ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", getMockedMigrateVolumeToApiCmdParam()); Mockito.when(userVmServiceMock.getVm(Mockito.anyLong())).thenReturn(virtualMachineMock); Mockito.when(virtualMachineMock.getState()).thenReturn(VirtualMachine.State.Stopped); - Mockito.when(userVmServiceMock.vmStorageMigration(virtualMachineUuid, migrateVolumeTo)).thenReturn(null); + Mockito.when(userVmServiceMock.vmStorageMigration(Mockito.anyLong(), Mockito.anyMap())).thenReturn(null); try { cmdSpy.execute(); @@ -194,11 +206,11 @@ public void executeTestHostIsNullMigratedVMIsNullThrowsServerApiException() { @Test public void executeTestSystemVMMigratedWithSuccess() { ReflectionTestUtils.setField(cmdSpy, "hostId", null); - ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", migrateVolumeTo); + ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", getMockedMigrateVolumeToApiCmdParam()); Mockito.when(userVmServiceMock.getVm(Mockito.anyLong())).thenReturn(virtualMachineMock); Mockito.when(virtualMachineMock.getState()).thenReturn(VirtualMachine.State.Stopped); - Mockito.when(userVmServiceMock.vmStorageMigration(virtualMachineUuid, migrateVolumeTo)).thenReturn(virtualMachineMock); + Mockito.when(userVmServiceMock.vmStorageMigration(Mockito.anyLong(), Mockito.anyMap())).thenReturn(virtualMachineMock); Mockito.when(virtualMachineMock.getType()).thenReturn(VirtualMachine.Type.ConsoleProxy); Mockito.when(responseGeneratorMock.createSystemVmResponse(virtualMachineMock)).thenReturn(systemVmResponse); @@ -211,11 +223,11 @@ public void executeTestSystemVMMigratedWithSuccess() { public void executeTestUserVMMigratedWithSuccess() { UserVm userVmMock = Mockito.mock(UserVm.class); ReflectionTestUtils.setField(cmdSpy, "hostId", null); - ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", migrateVolumeTo); + ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", getMockedMigrateVolumeToApiCmdParam()); Mockito.when(userVmServiceMock.getVm(Mockito.anyLong())).thenReturn(userVmMock); Mockito.when(userVmMock.getState()).thenReturn(VirtualMachine.State.Stopped); - Mockito.when(userVmServiceMock.vmStorageMigration(virtualMachineUuid, migrateVolumeTo)).thenReturn(userVmMock); + Mockito.when(userVmServiceMock.vmStorageMigration(Mockito.anyLong(), Mockito.anyMap())).thenReturn(userVmMock); Mockito.when(userVmMock.getType()).thenReturn(VirtualMachine.Type.User); Mockito.when(responseGeneratorMock.createUserVmResponse(ResponseObject.ResponseView.Full, "virtualmachine", userVmMock)).thenReturn(List.of(userVmResponse)); From 919c4e634f609160b99304eef99a7c1678cef5e6 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Tue, 23 May 2023 23:56:04 +0530 Subject: [PATCH 5/9] fix Signed-off-by: Abhishek Kumar --- server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 1019444fdafb..fda3df42f0bb 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -6902,7 +6902,7 @@ private Map getVolumePoolMappingForMigrateVmWithStorage(VMInstanceVO return volToPoolObjectMap; } - protected Host chooseVmMigrationDestination(VMInstanceVO vm, Host srcHost, Map volToPoolObjectMap) { + protected Host chooseVmMigrationDestinationUsingVolumePoolMap(VMInstanceVO vm, Host srcHost, Map volToPoolObjectMap) { Long poolId = null; if (MapUtils.isNotEmpty(volToPoolObjectMap)) { poolId = new ArrayList<>(volToPoolObjectMap.values()).get(0); @@ -6973,7 +6973,7 @@ public VirtualMachine migrateVirtualMachineWithVolume(Long vmId, Host destinatio Map volToPoolObjectMap = getVolumePoolMappingForMigrateVmWithStorage(vm, volumeToPool); if (destinationHost == null) { - destinationHost = chooseVmMigrationDestination(vm, srcHost, volToPoolObjectMap); + destinationHost = chooseVmMigrationDestinationUsingVolumePoolMap(vm, srcHost, volToPoolObjectMap); } checkHostsDedication(vm, srcHost.getId(), destinationHost.getId()); From 1d6b143210f08b5c4d0499b5a1269ad6deff29e1 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Fri, 26 May 2023 16:42:15 +0530 Subject: [PATCH 6/9] api: fix check Signed-off-by: Abhishek Kumar --- .../command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java index c618d222810a..6a789692b4cb 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java @@ -152,8 +152,8 @@ public ApiCommandResourceType getApiResourceType() { @Override public void execute() { - if (hostId == null && MapUtils.isEmpty(migrateVolumeTo)) { - throw new InvalidParameterValueException(String.format("Either %s or %s must be passed for migrating the VM.", ApiConstants.HOST_ID, ApiConstants.MIGRATE_TO)); + if (hostId == null && MapUtils.isEmpty(migrateVolumeTo) && !Boolean.TRUE.equals(autoSelect)) { + throw new InvalidParameterValueException(String.format("Either %s or %s must be passed or %s must be true for migrating the VM.", ApiConstants.HOST_ID, ApiConstants.MIGRATE_TO, ApiConstants.AUTO_SELECT)); } VirtualMachine virtualMachine = _userVmService.getVm(getVirtualMachineId()); From f30597f307790ebc34bcd54bcc9d038dca1bcb0f Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Fri, 26 May 2023 18:01:56 +0530 Subject: [PATCH 7/9] tests Signed-off-by: Abhishek Kumar --- ...igrateVirtualMachineWithVolumeCmdTest.java | 7 +- .../java/com/cloud/vm/UserVmManagerImpl.java | 7 +- .../com/cloud/vm/UserVmManagerImplTest.java | 158 +++++++++++++++++- 3 files changed, 157 insertions(+), 15 deletions(-) diff --git a/api/src/test/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmdTest.java b/api/src/test/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmdTest.java index a4556d4eb125..61a3c8fb9e65 100644 --- a/api/src/test/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmdTest.java +++ b/api/src/test/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmdTest.java @@ -88,15 +88,16 @@ public void setUp() throws Exception { } @Test - public void executeTestHostIdIsNullAndMigrateVolumeToIsNullThrowsInvalidParameterValueException() { + public void executeTestRequiredArgsNullThrowsInvalidParameterValueException() { ReflectionTestUtils.setField(cmdSpy, "hostId", null); ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", null); + ReflectionTestUtils.setField(cmdSpy, "autoSelect", null); try { cmdSpy.execute(); } catch (Exception e) { Assert.assertEquals(InvalidParameterValueException.class, e.getClass()); - String expected = String.format("Either %s or %s must be passed for migrating the VM.", ApiConstants.HOST_ID, ApiConstants.MIGRATE_TO); + String expected = String.format("Either %s or %s must be passed or %s must be true for migrating the VM.", ApiConstants.HOST_ID, ApiConstants.MIGRATE_TO, ApiConstants.AUTO_SELECT); Assert.assertEquals(expected , e.getMessage()); } } @@ -120,7 +121,7 @@ public void executeTestVMIsStoppedAndHostIdIsNotNullThrowsInvalidParameterValueE } @Test - public void executeTestVMIsRunningAndHostIdIsNullThrowsInvalidParameterValueException() { + public void executeTestVMIsRunningHostIdIsNullAndAutoSelectIsFalseThrowsInvalidParameterValueException() { ReflectionTestUtils.setField(cmdSpy, "hostId", null); ReflectionTestUtils.setField(cmdSpy, "autoSelect", false); ReflectionTestUtils.setField(cmdSpy, "migrateVolumeTo", migrateVolumeTo); diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index fda3df42f0bb..0c38de1ef59c 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -129,7 +129,6 @@ import org.apache.commons.collections.CollectionUtils; import org.apache.commons.collections.MapUtils; import org.apache.commons.lang.math.NumberUtils; -import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.builder.ToStringBuilder; import org.apache.commons.lang3.builder.ToStringStyle; @@ -6741,7 +6740,7 @@ private boolean isImplicitPlannerUsedByOffering(long offeringId) { return implicitPlannerUsed; } - private boolean isAnyVmVolumeUsingLocalStorage(final List volumes) { + protected boolean isAnyVmVolumeUsingLocalStorage(final List volumes) { for (VolumeVO vol : volumes) { DiskOfferingVO diskOffering = _diskOfferingDao.findById(vol.getDiskOfferingId()); if (diskOffering.isUseLocalStorage()) { @@ -6755,7 +6754,7 @@ private boolean isAnyVmVolumeUsingLocalStorage(final List volumes) { return false; } - private boolean isAllVmVolumesOnZoneWideStore(final List volumes) { + protected boolean isAllVmVolumesOnZoneWideStore(final List volumes) { if (CollectionUtils.isEmpty(volumes)) { return false; } @@ -6908,7 +6907,7 @@ protected Host chooseVmMigrationDestinationUsingVolumePoolMap(VMInstanceVO vm, H poolId = new ArrayList<>(volToPoolObjectMap.values()).get(0); } DeployDestination deployDestination = chooseVmMigrationDestination(vm, srcHost, poolId); - if (ObjectUtils.anyNull(deployDestination, deployDestination.getHost())) { + if (deployDestination == null || deployDestination.getHost() == null) { throw new CloudRuntimeException("Unable to find suitable destination to migrate VM " + vm.getInstanceName()); } return deployDestination.getHost(); diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index 18b4a8919173..93995bc860a7 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -16,8 +16,6 @@ // under the License. package com.cloud.vm; -import com.cloud.storage.Volume; -import com.cloud.storage.dao.VolumeDao; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -38,18 +36,15 @@ import java.util.List; import java.util.Map; -import com.cloud.template.VirtualMachineTemplate; -import com.cloud.user.UserData; -import com.cloud.user.UserDataVO; -import com.cloud.user.dao.UserDataDao; -import com.cloud.utils.exception.CloudRuntimeException; import org.apache.cloudstack.api.BaseCmd.HTTPMethod; -import org.apache.cloudstack.api.command.user.vm.ResetVMUserDataCmd; import org.apache.cloudstack.api.command.user.vm.DeployVMCmd; +import org.apache.cloudstack.api.command.user.vm.ResetVMUserDataCmd; import org.apache.cloudstack.api.command.user.vm.UpdateVMCmd; import org.apache.cloudstack.api.command.user.volume.ResizeVolumeCmd; import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService; +import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -67,11 +62,19 @@ import com.cloud.dc.DataCenter; import com.cloud.dc.DataCenterVO; import com.cloud.dc.dao.DataCenterDao; +import com.cloud.deploy.DataCenterDeployment; +import com.cloud.deploy.DeployDestination; +import com.cloud.deploy.DeploymentPlanner; +import com.cloud.deploy.DeploymentPlanningManager; import com.cloud.exception.InsufficientAddressCapacityException; import com.cloud.exception.InsufficientCapacityException; +import com.cloud.exception.InsufficientServerCapacityException; import com.cloud.exception.InvalidParameterValueException; import com.cloud.exception.ResourceAllocationException; import com.cloud.exception.ResourceUnavailableException; +import com.cloud.host.Host; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor; import com.cloud.network.NetworkModel; import com.cloud.network.dao.NetworkDao; @@ -81,21 +84,30 @@ import com.cloud.service.dao.ServiceOfferingDao; import com.cloud.storage.DiskOfferingVO; import com.cloud.storage.GuestOSVO; +import com.cloud.storage.ScopeType; import com.cloud.storage.VMTemplateVO; +import com.cloud.storage.Volume; import com.cloud.storage.VolumeApiService; import com.cloud.storage.VolumeVO; import com.cloud.storage.dao.DiskOfferingDao; import com.cloud.storage.dao.GuestOSDao; import com.cloud.storage.dao.VMTemplateDao; +import com.cloud.storage.dao.VolumeDao; +import com.cloud.template.VirtualMachineTemplate; import com.cloud.user.Account; import com.cloud.user.AccountManager; import com.cloud.user.AccountService; import com.cloud.user.AccountVO; import com.cloud.user.ResourceLimitService; +import com.cloud.user.UserData; +import com.cloud.user.UserDataVO; import com.cloud.user.UserVO; import com.cloud.user.dao.AccountDao; +import com.cloud.user.dao.UserDataDao; import com.cloud.uservm.UserVm; +import com.cloud.utils.Pair; import com.cloud.utils.db.EntityManager; +import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.UserVmDetailsDao; @@ -181,6 +193,18 @@ public class UserVmManagerImplTest { @Mock UserDataDao userDataDao; + @Mock + PrimaryDataStoreDao primaryDataStoreDao; + + @Mock + VirtualMachineManager virtualMachineManager; + + @Mock + DeploymentPlanningManager planningManager; + + @Mock + HostDao hostDao; + @Mock private VolumeVO volumeVOMock; @@ -912,4 +936,122 @@ public void createVirtualMachineWithInactiveServiceOffering() throws ResourceUna userVmManagerImpl.createVirtualMachine(deployVMCmd); } + + private List mockVolumesForIsAnyVmVolumeUsingLocalStorageTest(int localVolumes, int nonLocalVolumes) { + List volumes = new ArrayList<>(); + for (int i=0; i< localVolumes + nonLocalVolumes; ++i) { + VolumeVO vol = Mockito.mock(VolumeVO.class); + long index = i + 1; + Mockito.when(vol.getDiskOfferingId()).thenReturn(index); + Mockito.when(vol.getPoolId()).thenReturn(index); + DiskOfferingVO diskOffering = Mockito.mock(DiskOfferingVO.class); + Mockito.when(diskOfferingDao.findById(index)).thenReturn(diskOffering); + StoragePoolVO storagePool = Mockito.mock(StoragePoolVO.class); + Mockito.when(primaryDataStoreDao.findById(index)).thenReturn(storagePool); + if (i < localVolumes) { + if ((localVolumes + nonLocalVolumes) % 2 == 0) { + Mockito.when(diskOffering.isUseLocalStorage()).thenReturn(true); + } else { + + Mockito.when(diskOffering.isUseLocalStorage()).thenReturn(false); + Mockito.when(storagePool.isLocal()).thenReturn(true); + } + } else { + Mockito.when(diskOffering.isUseLocalStorage()).thenReturn(false); + Mockito.when(storagePool.isLocal()).thenReturn(false); + } + volumes.add(vol); + } + return volumes; + } + + @Test + public void testIsAnyVmVolumeUsingLocalStorage() { + Assert.assertTrue(userVmManagerImpl.isAnyVmVolumeUsingLocalStorage(mockVolumesForIsAnyVmVolumeUsingLocalStorageTest(1, 0))); + Assert.assertTrue(userVmManagerImpl.isAnyVmVolumeUsingLocalStorage(mockVolumesForIsAnyVmVolumeUsingLocalStorageTest(2, 0))); + Assert.assertTrue(userVmManagerImpl.isAnyVmVolumeUsingLocalStorage(mockVolumesForIsAnyVmVolumeUsingLocalStorageTest(1, 1))); + Assert.assertFalse(userVmManagerImpl.isAnyVmVolumeUsingLocalStorage(mockVolumesForIsAnyVmVolumeUsingLocalStorageTest(0, 2))); + Assert.assertFalse(userVmManagerImpl.isAnyVmVolumeUsingLocalStorage(mockVolumesForIsAnyVmVolumeUsingLocalStorageTest(0, 0))); + } + + private List mockVolumesForIsAllVmVolumesOnZoneWideStore(int nullPoolIdVolumes, int nullPoolVolumes, int zoneVolumes, int nonZoneVolumes) { + List volumes = new ArrayList<>(); + for (int i=0; i< nullPoolIdVolumes + nullPoolVolumes + zoneVolumes + nonZoneVolumes; ++i) { + VolumeVO vol = Mockito.mock(VolumeVO.class); + volumes.add(vol); + if (i < nullPoolIdVolumes) { + Mockito.when(vol.getPoolId()).thenReturn(null); + continue; + } + long index = i + 1; + Mockito.when(vol.getPoolId()).thenReturn(index); + if (i < nullPoolVolumes) { + Mockito.when(primaryDataStoreDao.findById(index)).thenReturn(null); + continue; + } + StoragePoolVO storagePool = Mockito.mock(StoragePoolVO.class); + Mockito.when(primaryDataStoreDao.findById(index)).thenReturn(storagePool); + if (i < zoneVolumes) { + Mockito.when(storagePool.getScope()).thenReturn(ScopeType.ZONE); + } else { + Mockito.when(storagePool.getScope()).thenReturn(ScopeType.CLUSTER); + } + } + return volumes; + } + + @Test + public void testIsAllVmVolumesOnZoneWideStoreCombinations() { + Assert.assertTrue(userVmManagerImpl.isAllVmVolumesOnZoneWideStore(mockVolumesForIsAllVmVolumesOnZoneWideStore(0, 0, 1, 0))); + Assert.assertTrue(userVmManagerImpl.isAllVmVolumesOnZoneWideStore(mockVolumesForIsAllVmVolumesOnZoneWideStore(0, 0, 2, 0))); + Assert.assertFalse(userVmManagerImpl.isAllVmVolumesOnZoneWideStore(mockVolumesForIsAllVmVolumesOnZoneWideStore(0, 0, 1, 1))); + Assert.assertFalse(userVmManagerImpl.isAllVmVolumesOnZoneWideStore(mockVolumesForIsAllVmVolumesOnZoneWideStore(0, 0, 0, 0))); + Assert.assertFalse(userVmManagerImpl.isAllVmVolumesOnZoneWideStore(mockVolumesForIsAllVmVolumesOnZoneWideStore(1, 0, 1, 1))); + Assert.assertFalse(userVmManagerImpl.isAllVmVolumesOnZoneWideStore(mockVolumesForIsAllVmVolumesOnZoneWideStore(0, 1, 1, 1))); + } + + private Pair mockObjectsForChooseVmMigrationDestinationUsingVolumePoolMapTest(boolean nullPlan, Host destinationHost) { + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm.getId()).thenReturn(1L); + Mockito.when(vm.getServiceOfferingId()).thenReturn(1L); + Host host = Mockito.mock(Host.class); + Mockito.when(host.getId()).thenReturn(1L); + Mockito.when(hostDao.findById(1L)).thenReturn(Mockito.mock(HostVO.class)); + Mockito.when(virtualMachineManager.getMigrationDeployment(Mockito.any(VirtualMachine.class), + Mockito.any(Host.class), Mockito.nullable(Long.class), + Mockito.any(DeploymentPlanner.ExcludeList.class))) + .thenReturn(Mockito.mock(DataCenterDeployment.class)); + if (!nullPlan) { + try { + DeployDestination destination = Mockito.mock(DeployDestination.class); + Mockito.when(destination.getHost()).thenReturn(destinationHost); + Mockito.when(planningManager.planDeployment(Mockito.any(VirtualMachineProfile.class), + Mockito.any(DataCenterDeployment.class), Mockito.any(DeploymentPlanner.ExcludeList.class), + Mockito.nullable(DeploymentPlanner.class))) + .thenReturn(destination); + } catch (InsufficientServerCapacityException e) { + Assert.fail("Failed to mock DeployDestination"); + } + } + return new Pair<>(vm, host); + } + + @Test(expected = CloudRuntimeException.class) + public void testChooseVmMigrationDestinationUsingVolumePoolMapNullDestination() { + Pair pair = mockObjectsForChooseVmMigrationDestinationUsingVolumePoolMapTest(true, null); + userVmManagerImpl.chooseVmMigrationDestinationUsingVolumePoolMap(pair.first(), pair.second(), null); + } + + @Test(expected = CloudRuntimeException.class) + public void testChooseVmMigrationDestinationUsingVolumePoolMapNullHost() { + Pair pair = mockObjectsForChooseVmMigrationDestinationUsingVolumePoolMapTest(false, null); + userVmManagerImpl.chooseVmMigrationDestinationUsingVolumePoolMap(pair.first(), pair.second(), null); + } + + @Test + public void testChooseVmMigrationDestinationUsingVolumePoolMapValid() { + Host destinationHost = Mockito.mock(Host.class); + Pair pair = mockObjectsForChooseVmMigrationDestinationUsingVolumePoolMapTest(false, destinationHost); + Assert.assertEquals(destinationHost, userVmManagerImpl.chooseVmMigrationDestinationUsingVolumePoolMap(pair.first(), pair.second(), null)); + } } From d85781d1c96bc9fb6802f29e7f2a3b0f4d776c68 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Tue, 30 May 2023 15:25:43 +0530 Subject: [PATCH 8/9] review comments Signed-off-by: Abhishek Kumar --- .../admin/vm/MigrateVirtualMachineWithVolumeCmd.java | 2 +- .../src/main/java/com/cloud/vm/UserVmManagerImpl.java | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java index 6a789692b4cb..b3a5363c8005 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java @@ -170,7 +170,7 @@ public void execute() { VirtualMachine migratedVm = null; if (getHostId() != null || Boolean.TRUE.equals(autoSelect)) { Host destinationHost = null; - if (!Boolean.TRUE.equals(autoSelect)) { + if (getHostId() != null) { destinationHost = _resourceService.getHost(getHostId()); // OfflineVmwareMigration: destination host would have to not be a required parameter for stopped VMs if (destinationHost == null) { diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 0c38de1ef59c..608a4e71681b 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -6901,6 +6901,13 @@ private Map getVolumePoolMappingForMigrateVmWithStorage(VMInstanceVO return volToPoolObjectMap; } + protected boolean isVmCanBeMigratedWithoutStorage(Host srcHost, Host destinationHost, List volumes, + Map volumeToPool) { + return !isAnyVmVolumeUsingLocalStorage(volumes) && + MapUtils.isEmpty(volumeToPool) && destinationHost != null + && (destinationHost.getClusterId().equals(srcHost.getClusterId()) || isAllVmVolumesOnZoneWideStore(volumes)); + } + protected Host chooseVmMigrationDestinationUsingVolumePoolMap(VMInstanceVO vm, Host srcHost, Map volToPoolObjectMap) { Long poolId = null; if (MapUtils.isNotEmpty(volToPoolObjectMap)) { @@ -6958,8 +6965,7 @@ public VirtualMachine migrateVirtualMachineWithVolume(Long vmId, Host destinatio Host srcHost = sourceDestinationHosts.first(); final List volumes = _volsDao.findCreatedByInstance(vm.getId()); - if (!isAnyVmVolumeUsingLocalStorage(volumes) && MapUtils.isEmpty(volumeToPool) && destinationHost != null - && (destinationHost.getClusterId().equals(srcHost.getClusterId()) || isAllVmVolumesOnZoneWideStore(volumes))) { + if (isVmCanBeMigratedWithoutStorage(srcHost, destinationHost, volumes, volumeToPool)) { // If volumes do not have to be migrated // call migrateVirtualMachine for non-user VMs else throw exception if (!VirtualMachine.Type.User.equals(vm.getType())) { From 175367baaa4419cf7a1320fd035376c7e12a86df Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Wed, 31 May 2023 15:46:09 +0530 Subject: [PATCH 9/9] another review comment Signed-off-by: Abhishek Kumar --- .../MigrateVirtualMachineWithVolumeCmd.java | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java index b3a5363c8005..549d02b45791 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/MigrateVirtualMachineWithVolumeCmd.java @@ -150,6 +150,19 @@ public ApiCommandResourceType getApiResourceType() { return ApiCommandResourceType.VirtualMachine; } + private Host getDestinationHost() { + if (getHostId() == null) { + return null; + } + Host destinationHost = _resourceService.getHost(getHostId()); + // OfflineVmwareMigration: destination host would have to not be a required parameter for stopped VMs + if (destinationHost == null) { + s_logger.error(String.format("Unable to find the host with ID [%s].", getHostId())); + throw new InvalidParameterValueException("Unable to find the specified host to migrate the VM."); + } + return destinationHost; + } + @Override public void execute() { if (hostId == null && MapUtils.isEmpty(migrateVolumeTo) && !Boolean.TRUE.equals(autoSelect)) { @@ -169,15 +182,7 @@ public void execute() { try { VirtualMachine migratedVm = null; if (getHostId() != null || Boolean.TRUE.equals(autoSelect)) { - Host destinationHost = null; - if (getHostId() != null) { - destinationHost = _resourceService.getHost(getHostId()); - // OfflineVmwareMigration: destination host would have to not be a required parameter for stopped VMs - if (destinationHost == null) { - s_logger.error(String.format("Unable to find the host with ID [%s].", getHostId())); - throw new InvalidParameterValueException("Unable to find the specified host to migrate the VM."); - } - } + Host destinationHost = getDestinationHost(); migratedVm = _userVmService.migrateVirtualMachineWithVolume(getVirtualMachineId(), destinationHost, getVolumeToPool()); } else if (MapUtils.isNotEmpty(migrateVolumeTo)) { migratedVm = _userVmService.vmStorageMigration(getVirtualMachineId(), getVolumeToPool());