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+Storag > > e+XenMotion+for+XenServer > > [4] https://reviews.apache.org/r/10196/ > > > > Regards, > > Devdeep
