GutoVeronezi commented on a change in pull request #5030:
URL: https://github.com/apache/cloudstack/pull/5030#discussion_r637957284
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -6365,24 +6329,75 @@ public VirtualMachine
migrateVirtualMachineWithVolume(Long vmId, Host destinatio
}
}
}
+ return volToPoolObjectMap;
+ }
- // Check if all the volumes are in the correct state.
- for (VolumeVO volume : vmVolumes) {
- if (volume.getState() != Volume.State.Ready) {
- throw new CloudRuntimeException("Volume " + volume + " of the
VM is not in Ready state. Cannot " + "migrate the vm with its volumes.");
+ @Override
+ @ActionEvent(eventType = EventTypes.EVENT_VM_MIGRATE, eventDescription =
"migrating VM", async = true)
+ public VirtualMachine migrateVirtualMachineWithVolume(Long vmId, Host
destinationHost, Map<String, String> volumeToPool) throws
ResourceUnavailableException,
+ ConcurrentOperationException, ManagementServerException,
VirtualMachineMigrationException {
+ // Access check - only root administrator can migrate VM.
+ Account caller = CallContext.current().getCallingAccount();
+ if (!_accountMgr.isRootAdmin(caller.getId())) {
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug("Caller is not a root admin, permission denied
to migrate the VM");
}
+ throw new PermissionDeniedException("No permission to migrate VM,
Only Root Admin can migrate a VM!");
}
- // Check max guest vm limit for the destinationHost.
- HostVO destinationHostVO = _hostDao.findById(destinationHost.getId());
- if (_capacityMgr.checkIfHostReachMaxGuestLimit(destinationHostVO)) {
- throw new VirtualMachineMigrationException("Host name: " +
destinationHost.getName() + ", hostId: " + destinationHost.getId()
- + " already has max running vms (count includes system VMs).
Cannot" + " migrate to this host");
+ VMInstanceVO vm = _vmInstanceDao.findById(vmId);
+ if (vm == null) {
+ throw new InvalidParameterValueException("Unable to find the VM by
ID " + vmId);
}
- checkHostsDedication(vm, srcHostId, destinationHost.getId());
+ // OfflineVmwareMigration: this would be it ;) if multiple paths
exist: unify
+ if (vm.getState() != State.Running) {
+ // OfflineVmwareMigration: and not vmware
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug("VM is not Running, unable to migrate the vm "
+ vm);
+ }
+ CloudRuntimeException ex = new
CloudRuntimeException(String.format("Unable to migrate the VM %s (ID: %s) as it
is not in Running state", vm.getInstanceName(), vm.getUuid()));
+ ex.addProxyObject(vm.getUuid(), "vmId");
+ throw ex;
+ }
+
+ if(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(),
GPU.Keys.pciDevice.toString()) != null) {
+ throw new InvalidParameterValueException("Live Migration of GPU
enabled VM is not supported");
+ }
+
+ // OfflineVmwareMigration: this condition is to complicated. (already
a method somewhere)
+ if (!Arrays.asList(new HypervisorType[]{
+ HypervisorType.XenServer,
+ HypervisorType.VMware,
+ HypervisorType.KVM,
+ HypervisorType.Simulator}).contains(vm.getHypervisorType())) {
+ throw new
InvalidParameterValueException(String.format("Unsupported hypervisor: %s for VM
migration, we support XenServer/VMware/KVM only", vm.getHypervisorType()));
+ }
+
+ // Check that Vm does not have VM Snapshots
+ if (_vmSnapshotDao.findByVm(vmId).size() > 0) {
Review comment:
We could assign the result of the method to a var and use
`CollectionUtils` to validate it.
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -6365,24 +6329,75 @@ public VirtualMachine
migrateVirtualMachineWithVolume(Long vmId, Host destinatio
}
}
}
+ return volToPoolObjectMap;
+ }
- // Check if all the volumes are in the correct state.
- for (VolumeVO volume : vmVolumes) {
- if (volume.getState() != Volume.State.Ready) {
- throw new CloudRuntimeException("Volume " + volume + " of the
VM is not in Ready state. Cannot " + "migrate the vm with its volumes.");
+ @Override
+ @ActionEvent(eventType = EventTypes.EVENT_VM_MIGRATE, eventDescription =
"migrating VM", async = true)
+ public VirtualMachine migrateVirtualMachineWithVolume(Long vmId, Host
destinationHost, Map<String, String> volumeToPool) throws
ResourceUnavailableException,
+ ConcurrentOperationException, ManagementServerException,
VirtualMachineMigrationException {
+ // Access check - only root administrator can migrate VM.
+ Account caller = CallContext.current().getCallingAccount();
+ if (!_accountMgr.isRootAdmin(caller.getId())) {
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug("Caller is not a root admin, permission denied
to migrate the VM");
}
+ throw new PermissionDeniedException("No permission to migrate VM,
Only Root Admin can migrate a VM!");
}
- // Check max guest vm limit for the destinationHost.
- HostVO destinationHostVO = _hostDao.findById(destinationHost.getId());
- if (_capacityMgr.checkIfHostReachMaxGuestLimit(destinationHostVO)) {
- throw new VirtualMachineMigrationException("Host name: " +
destinationHost.getName() + ", hostId: " + destinationHost.getId()
- + " already has max running vms (count includes system VMs).
Cannot" + " migrate to this host");
+ VMInstanceVO vm = _vmInstanceDao.findById(vmId);
+ if (vm == null) {
+ throw new InvalidParameterValueException("Unable to find the VM by
ID " + vmId);
}
- checkHostsDedication(vm, srcHostId, destinationHost.getId());
+ // OfflineVmwareMigration: this would be it ;) if multiple paths
exist: unify
+ if (vm.getState() != State.Running) {
+ // OfflineVmwareMigration: and not vmware
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug("VM is not Running, unable to migrate the vm "
+ vm);
+ }
+ CloudRuntimeException ex = new
CloudRuntimeException(String.format("Unable to migrate the VM %s (ID: %s) as it
is not in Running state", vm.getInstanceName(), vm.getUuid()));
+ ex.addProxyObject(vm.getUuid(), "vmId");
+ throw ex;
+ }
+
+ if(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(),
GPU.Keys.pciDevice.toString()) != null) {
+ throw new InvalidParameterValueException("Live Migration of GPU
enabled VM is not supported");
+ }
+
+ // OfflineVmwareMigration: this condition is to complicated. (already
a method somewhere)
+ if (!Arrays.asList(new HypervisorType[]{
+ HypervisorType.XenServer,
+ HypervisorType.VMware,
+ HypervisorType.KVM,
+ HypervisorType.Simulator}).contains(vm.getHypervisorType())) {
+ throw new
InvalidParameterValueException(String.format("Unsupported hypervisor: %s for VM
migration, we support XenServer/VMware/KVM only", vm.getHypervisorType()));
+ }
+
+ // Check that Vm does not have VM Snapshots
Review comment:
As the code is clear and we have a explanation on the exception message,
do still we need this comment?
##########
File path: server/src/main/java/com/cloud/storage/StorageManagerImpl.java
##########
@@ -2393,7 +2393,7 @@ public boolean
storagePoolHasEnoughSpaceForResize(StoragePool pool, long current
}
@Override
- public boolean isStoragePoolComplaintWithStoragePolicy(List<Volume>
volumes, StoragePool pool) throws StorageUnavailableException {
+ public boolean isStoragePoolCompliantWithStoragePolicy(List<Volume>
volumes, StoragePool pool) throws StorageUnavailableException {
if (volumes == null || volumes.isEmpty()) {
Review comment:
We can use `CollectionUtils` to do the validation.
```java
...
if (CollectionUtils.isEmpty(volumes)) {
...
```
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -6365,24 +6329,75 @@ public VirtualMachine
migrateVirtualMachineWithVolume(Long vmId, Host destinatio
}
}
}
+ return volToPoolObjectMap;
+ }
- // Check if all the volumes are in the correct state.
- for (VolumeVO volume : vmVolumes) {
- if (volume.getState() != Volume.State.Ready) {
- throw new CloudRuntimeException("Volume " + volume + " of the
VM is not in Ready state. Cannot " + "migrate the vm with its volumes.");
+ @Override
+ @ActionEvent(eventType = EventTypes.EVENT_VM_MIGRATE, eventDescription =
"migrating VM", async = true)
+ public VirtualMachine migrateVirtualMachineWithVolume(Long vmId, Host
destinationHost, Map<String, String> volumeToPool) throws
ResourceUnavailableException,
+ ConcurrentOperationException, ManagementServerException,
VirtualMachineMigrationException {
+ // Access check - only root administrator can migrate VM.
+ Account caller = CallContext.current().getCallingAccount();
+ if (!_accountMgr.isRootAdmin(caller.getId())) {
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug("Caller is not a root admin, permission denied
to migrate the VM");
}
+ throw new PermissionDeniedException("No permission to migrate VM,
Only Root Admin can migrate a VM!");
}
- // Check max guest vm limit for the destinationHost.
- HostVO destinationHostVO = _hostDao.findById(destinationHost.getId());
- if (_capacityMgr.checkIfHostReachMaxGuestLimit(destinationHostVO)) {
- throw new VirtualMachineMigrationException("Host name: " +
destinationHost.getName() + ", hostId: " + destinationHost.getId()
- + " already has max running vms (count includes system VMs).
Cannot" + " migrate to this host");
+ VMInstanceVO vm = _vmInstanceDao.findById(vmId);
+ if (vm == null) {
+ throw new InvalidParameterValueException("Unable to find the VM by
ID " + vmId);
}
- checkHostsDedication(vm, srcHostId, destinationHost.getId());
+ // OfflineVmwareMigration: this would be it ;) if multiple paths
exist: unify
+ if (vm.getState() != State.Running) {
+ // OfflineVmwareMigration: and not vmware
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug("VM is not Running, unable to migrate the vm "
+ vm);
+ }
+ CloudRuntimeException ex = new
CloudRuntimeException(String.format("Unable to migrate the VM %s (ID: %s) as it
is not in Running state", vm.getInstanceName(), vm.getUuid()));
+ ex.addProxyObject(vm.getUuid(), "vmId");
+ throw ex;
+ }
+
+ if(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(),
GPU.Keys.pciDevice.toString()) != null) {
+ throw new InvalidParameterValueException("Live Migration of GPU
enabled VM is not supported");
+ }
+
+ // OfflineVmwareMigration: this condition is to complicated. (already
a method somewhere)
+ if (!Arrays.asList(new HypervisorType[]{
+ HypervisorType.XenServer,
+ HypervisorType.VMware,
+ HypervisorType.KVM,
+ HypervisorType.Simulator}).contains(vm.getHypervisorType())) {
Review comment:
This array will be created every time that this method is called, so we
could extract this creation to a constant and use it here.
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -6299,38 +6261,40 @@ public VirtualMachine
migrateVirtualMachineWithVolume(Long vmId, Host destinatio
}
}
- if
(!Boolean.TRUE.equals(_hypervisorCapabilitiesDao.isStorageMotionSupported(srcHost.getHypervisorType(),
srcHostVersion))) {
- throw new CloudRuntimeException("Migration with storage isn't
supported for source host ID: " + srcHost.getUuid() + " on hypervisor " +
srcHost.getHypervisorType() + " of version " + srcHost.getHypervisorVersion());
+ if
(!_hypervisorCapabilitiesDao.isStorageMotionSupported(srcHost.getHypervisorType(),
srcHostVersion)) {
+ throw new CloudRuntimeException(String.format("Migration with
storage isn't supported for source host %s (ID: %s) on hypervisor %s with
version %s", srcHost.getName(), srcHost.getUuid(), srcHost.getHypervisorType(),
srcHost.getHypervisorVersion()));
}
if (srcHostVersion == null || !srcHostVersion.equals(destHostVersion))
{
- if
(!Boolean.TRUE.equals(_hypervisorCapabilitiesDao.isStorageMotionSupported(destinationHost.getHypervisorType(),
destHostVersion))) {
- throw new CloudRuntimeException("Migration with storage isn't
supported for target host ID: " + srcHost.getUuid() + " on hypervisor " +
srcHost.getHypervisorType() + " of version " + srcHost.getHypervisorVersion());
+ if
(!_hypervisorCapabilitiesDao.isStorageMotionSupported(destinationHost.getHypervisorType(),
destHostVersion)) {
+ throw new CloudRuntimeException(String.format("Migration with
storage isn't supported for target host %s (ID: %s) on hypervisor %s with
version %s", destinationHost.getName(), destinationHost.getUuid(),
destinationHost.getHypervisorType(), destinationHost.getHypervisorVersion()));
}
}
// Check if destination host is up.
if (destinationHost.getState() != com.cloud.host.Status.Up ||
destinationHost.getResourceState() != ResourceState.Enabled) {
- throw new CloudRuntimeException("Cannot migrate VM, destination
host is not in correct state, has " + "status: " + destinationHost.getState() +
", state: "
- + destinationHost.getResourceState());
+ throw new CloudRuntimeException(String.format("Cannot migrate VM,
destination host %s (ID: %s) is not in correct state, has status: %s, state:
%s",
+ destinationHost.getName(), destinationHost.getUuid(),
destinationHost.getState(), destinationHost.getResourceState()));
}
- // Check that Vm does not have VM Snapshots
- if (_vmSnapshotDao.findByVm(vmId).size() > 0) {
- throw new InvalidParameterValueException("VM with VM Snapshots
cannot be migrated with storage, please remove all VM snapshots");
+ // Check max guest vm limit for the destinationHost.
+ if (_capacityMgr.checkIfHostReachMaxGuestLimit(destinationHost)) {
+ throw new VirtualMachineMigrationException(String.format("Cannot
migrate VM as destination host %s (ID: %s) already has max running vms (count
includes system VMs)",
+ destinationHost.getName(), destinationHost.getUuid()));
}
- List<VolumeVO> vmVolumes =
_volsDao.findUsableVolumesForInstance(vm.getId());
+ return new Pair<>(srcHost, destinationHost);
+ }
+
+ private Map<Long, Long>
getVolumePoolMappingForMigrateVmWithStorage(VMInstanceVO vm, Map<String,
String> volumeToPool) {
Map<Long, Long> volToPoolObjectMap = new HashMap<Long, Long>();
- if (!isVMUsingLocalStorage(vm) && MapUtils.isEmpty(volumeToPool)
- && (destinationHost.getClusterId().equals(srcHost.getClusterId())
|| isVmVolumesOnZoneWideStore(vm))){
- // If volumes do not have to be migrated
- // call migrateVirtualMachine for non-user VMs else throw exception
- if (!VirtualMachine.Type.User.equals(vm.getType())) {
- return migrateVirtualMachine(vmId, destinationHost);
+
+ List<VolumeVO> vmVolumes =
_volsDao.findUsableVolumesForInstance(vm.getId());
+ // Check if all the volumes are in the correct state.
+ for (VolumeVO volume : vmVolumes) {
+ if (volume.getState() != Volume.State.Ready) {
+ throw new CloudRuntimeException("Volume " + volume + " of the
VM is not in Ready state. Cannot " + "migrate the vm with its volumes.");
}
- throw new InvalidParameterValueException("Migration of the vm " +
vm + "from host " + srcHost + " to destination host " + destinationHost
- + " doesn't involve migrating the volumes.");
}
Review comment:
We could extract this code to a method with a properly name and remove
the comment. Also, we could add unit tests to the method too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
