mike-tutkowski commented on a change in pull request #2761: Add managed storage
pool constraints to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#discussion_r205245877
##########
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -2282,7 +2282,7 @@ protected void migrate(final VMInstanceVO vm, final long
srcHostId, final Deploy
* Create the mapping of volumes and storage pools. If the user did not
enter a mapping on her/his own, we create one using {@link
#getDefaultMappingOfVolumesAndStoragePoolForMigration(VirtualMachineProfile,
Host)}.
* If the user provided a mapping, we use whatever the user has provided
(check the method {@link
#createMappingVolumeAndStoragePoolEnteredByUser(VirtualMachineProfile, Host,
Map)}).
*/
- private Map<Volume, StoragePool>
getPoolListForVolumesForMigration(VirtualMachineProfile profile, Host
targetHost, Map<Long, Long> volumeToPool) {
+ protected Map<Volume, StoragePool>
getPoolListForVolumesForMigration(VirtualMachineProfile profile, Host
targetHost, Map<Long, Long> volumeToPool) {
Review comment:
The old code used to walk through each volume. It would see if the volume
was specified in the volumeToPool mapping (provided by the user). If it was, it
would verify that the target storage pool could, in fact, be used. If the
specified target storage pool could not be used, an exception was thrown.
If the volume in question was not specified in the volumeToPool mapping, the
old code would look to see if the volume could stay on the same storage pool or
if it had to be migrated to a new storage pool. The new code does not perform
this function. The new code only tries to migrate volumes that were specified
by the user in the provided mapping. It could be that the user wants to specify
that volume X gets migrated to storage pool Y, but that the user doesn't care
where volume A gets migrated to (he/she is letting CloudStack decide on that
one). The new code would just ignore the migration of volume A (which may not
be acceptable in the case of a VM is being migrated to a new cluster).
Instead of making an empty mapping be a special use case (the if statement
in the new code's getPoolListForVolumesForMigration method), I would recommend
an approach more similar to the old code: Just iterate over each volume in the
VM to migrate. Check the user-provided mapping for a target storage pool. If
specified, make sure it can be used by the target host. If not specified, use
the storage pool allocators to try to find a match. If the current storage pool
is one of the matches, then there is nothing to migrate for this volume. If the
volume is specified in the map and its target pool is the same as its current
pool, make sure the target host can see that pool. If so, there is nothing to
migrate for this volume; else, thrown an exception.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
