GabrielBrascher commented on pull request #5410: URL: https://github.com/apache/cloudstack/pull/5410#issuecomment-930554729
@rhtyd @nvazquez @shwstppr @GutoVeronezi thanks for the review. I am adding a few contexts and conclusions on how this implementation has evolved. I did a few changes on past Friday and have tested it since then [see [this comment](https://github.com/apache/cloudstack/pull/5410#issuecomment-926646375)]. #### Recap and adding context on this PR ##### There were two options to fix: 1. Before calling the copy command -- map only the volumes that are going to be migrated which are either just the root disk, or root disk (Local) + data disks (shared) that the user has explicitly added to be migrated in the API call. 2. At the `KvmNonManagedStorageDataMotionStrategy` -- this change would impact on the "canHandle" and add a few changes in the workflow in order to "ignore" some of the mapped volumes; which require more work to ensure the volumes should indeed be ignored. ##### Considerations for each option **Option 1:** it is interesting as so far works like a charm and it is simple to implement; however, it requires specific logic for KVM in the "generic" class for migrating VM. **Option 2:** makes sense to narrow it to the KVM strategy; however, at that point, all VMs are mapped as migrating in the DB and it does not make much sense to have such volumes in the migrating state. The implementation also was a lot more complex to be done. #### Tests and conclusion I was successful with the Local + RBD migrations. looks like a great addition to 4.16 in this scenario. However, Local + NFS still not working on my tests. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
