Hi Edison, I have updated the patch [1] after incorporating your review comments. It also includes the changes to address the review comments given by Alex. Kindly take a look at the changes and let me know your comments.
[1] https://reviews.apache.org/r/10196/ Regards, Devdeep > -----Original Message----- > From: Devdeep Singh [mailto:devdeep.si...@citrix.com] > Sent: Friday, April 05, 2013 3:37 PM > To: dev@cloudstack.apache.org > Subject: RE: Review request for storage motion on xenserver > > Hi Edison, > > Thanks a lot for looking at the changes. I am going through your feedback and > working on a patch which addresses your review comments. I'll get back to > you if I have any queries. > > Regards, > Devdeep > > > -----Original Message----- > > From: Edison Su [mailto:edison...@citrix.com] > > Sent: Thursday, April 04, 2013 3:21 AM > > To: dev@cloudstack.apache.org > > Subject: RE: Review request for storage motion on xenserver > > > > Sorry for the late. Following is my comments on storage motion: > > 1. We shouldn't touch volume state in virutalmachinemanager. I spend a > > lot of time trying to move volume related operations from > > virtualmachinemanager to storage subsystem, better to follow the same > pattern. > > 2. The current storage subsystem apis, can only operate on one data > > object (volume/snapshot or template) at one time. In storage migration > > case, it has to deal with multiple volumes at one time. > > My suggestion is that add a new method, like, > > migrateVolumes(Map<volumeInfo, DataStore> volumeMaps), which can > > migrate a group of volumes at one time. > > 3. The code follow will look like: > > virtualmachineManagerImpl:migrateWithStorage(change vm state) -> > > volumemanager: migrateVolumes{do basic check} -> > > volumeService:migrateVolumes{which will change volume state} -> > > datamotionservice:copyAsync(which will find a data motion strategy > > which can handle storage migration for xenserver) -> > > xenserverstoragemigrationstategy:copyAsync(send commands to xenserver > > resource ) > > > > You need to write a datamotion strategy for xenserver storage > > migration, and need to add a method on both datamotionservice, > > volumeservice, volumemanager and DataMotionStrategy, so that they can > > operate on multiple volumes at one time. > > > > 4. Don't need to add a new api called migrateasync, copyasync has the > > same meaning. > > > > Does it make sense? > > > > > -----Original Message----- > > > From: Devdeep Singh [mailto:devdeep.si...@citrix.com] > > > Sent: Friday, March 29, 2013 8:21 AM > > > To: dev@cloudstack.apache.org > > > Subject: Review request for storage motion on xenserver > > > > > > I have put the feature proposed [1] and developed in the feature > > > branch [2] up for review. Code for this feature conforms to what was > > proposed in FS [3]. > > > The patch available at [4]. It includes marvin tests and unit tests > > > for verifying the functionality. Please take a look at it and let me > > > know your > > comments. > > > > > > [1] http://markmail.org/message/numdk6pdab2hekdp > > > [2] https://github.com/devdeep/cloudstack/commits/sm3 > > > [3] > > > https://cwiki.apache.org/confluence/display/CLOUDSTACK/Enabling+Stor > > > ag > > > e+XenMotion+for+XenServer > > > [4] https://reviews.apache.org/r/10196/ > > > > > > Regards, > > > Devdeep
