From f991e4e5c6d9c593476d266cc831a944708b5884 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Fri, 30 Sep 2022 01:56:39 +0530 Subject: [PATCH 1/5] 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/5] 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/5] 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/5] 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/5] 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());