You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Edison Su <Ed...@citrix.com> on 2013/04/03 23:51:19 UTC

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.singh@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

RE: Review request for storage motion on xenserver

Posted by Devdeep Singh <de...@citrix.com>.
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.singh@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.su@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.singh@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

RE: Review request for storage motion on xenserver

Posted by Devdeep Singh <de...@citrix.com>.
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.su@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.singh@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