incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Huang <Alex.Hu...@citrix.com>
Subject RE: [MERGE] Support VM Snapshot
Date Fri, 01 Feb 2013 18:54:12 GMT
Mice,

Thanks!

Anthony,

Can you comment on whether VM Snapshot breaks volume snapshot?

--Alex

> -----Original Message-----
> From: Mice Xia [mailto:weiran.xia1@gmail.com]
> Sent: Friday, February 01, 2013 8:53 AM
> To: cloudstack-dev@incubator.apache.org; Alex Huang
> Subject: Re: [MERGE] Support VM Snapshot
> 
> as Alex suggested
> updated vm-snapshot branch, commit ebca6890fd
> 
> 1. remove snapshotting/revertting state from VM state machine
> 2  prevent VM state change if there are active vm snapshot tasks
> 3  change VMSnapshotService interface, except for ListVMSnapshotCmd,
> need some time to replace it in QueryService, maybe after merging to
> master
> 4  remove unused methods and fix some typos
> 
> Regards
> Mice
> 
> 2013/2/1 Mice Xia <mice_xia@tcloudcomputing.com>:
> > Hi, Alex,
> >
> > Thanks for your feedbacks, please see my comments inline.
> >
> > - VM states is designed for VM lifecycle.  Snapshot is not part of VM life
> cycle so therefore the state should not be there.  I think it make sense to add
> attributes to VM that says "Do Not Change State" and  who changed the VM
> to that state.  Then virtualmachinemanager must obey that until the external
> caller changes the attribute to now you can change state.  The would make
> more sense and decouples snapshotting from vm lifecycle management.
> Snapshotting then has it's own state (which I see it does already).  If we want
> to reflect that a vm snapshot is being taken of a VM, that's a function of the
> apiresponse module that gathers up everything about a vm but it shouldn't
> be changed in the vm states.
> >
> > [mice] the reason that I added snapshotting/reverting state is that VM
> could be in suspend/pause state during snapshoting/reverting, which is
> difficult to be categorized into existing states; and during the process, VM
> should not be allowed to take any operations; and by adding new states to
> VM, the implementation seems more 'natural' and only minimum codes are
> changed to virtualmachinemanager.
> > Of course there are some other ways to prevent operations, such as check
> if associated snapshots are in snapshotting/reverting states either in each
> method (start/stop/migrate/delete...) or hook stateTransitTo(), but in this
> way, it does not reflect VM's real state in hypervisor and more existing codes
> will be touched.
> >
> > -  Does VM Revert operation work in the following way: Stop VM, restore
> to snapshot, and run VM?  Shouldn't this be orchestration inside snapshot
> manager?
> >
> > [mice] if a running VM is reverted to a memory enabled snapshot, current
> implementation is running--> reverting-->running
> > If a stopped VM is reverted to memory disabled snapshot: stopped--
> >reverting->stopped
> > If a running VM is reverted to a memory disabled snapshot: running--(Stop
> VM)-->stopped-->reverting--> stopped
> > If a stopped VM is reverted to a memory enabled snapshot: stopped--
> (Start VM)-->running->reverting-->running
> >
> > These logics are implemented in snapshot manager.
> >
> > - Does VM snapshot interfere with volume snapshot?  Volume snapshot
> today makes the assumption that it is the only code that's making snapshots
> and can break if there are additional snapshots in between.  This is bad
> design in volume snapshot but unfortunately that's how it's design.
> >
> > [mice] about volume snapshot, for xensever, if parent VHD cannot be
> found, it will take a full volume snapshot (this indeed break current
> semantics but it still works)
> > For vmware, the volume snapshot is always a full one.
> >
> > - VMSnapshotService follows the other services in passing the cmds to the
> service.  That's really a bad practice that we should stop.  Cmds are really
> translations between over-the-wire api and java interface.  They shouldn't
> have been passed to down to the java interface.
> > [
> > mice] I'll change it
> >
> > A small note: Would it be better to call it VM restore than VM revert?
> Revert really should be RevertTo which I think is in the code but is not
> consistent.  Some places it's just REVERT (for example, the event is just
> revert).
> >
> > [mice] there is already RESTORE, which is restoring a destroyed VM to
> stopped. RevertTo is fine with me.
> >
> > -Mice
> >
> >
> > -----Original Message-----
> > From: Alex Huang [mailto:Alex.Huang@citrix.com]
> > Sent: Friday, February 01, 2013 9:24 AM
> > To: CloudStack DeveloperList
> > Cc: Mice Xia
> > Subject: RE: [MERGE] Support VM Snapshot
> >
> > Hi Mice,
> >
> > Sorry it took so long to review this.  Wanted to as soon as I saw it on the list
> but was sick and didn't get a chance.  In general, I think the code is excellent.
> I'm impressed how much Cloudstack internal code in touch and how
> comfortable the changes look.  Nicely done!
> >
> > I have a few comments:
> > - VM states is designed for VM lifecycle.  Snapshot is not part of VM life
> cycle so therefore the state should not be there.  I think it make sense to add
> attributes to VM that says "Do Not Change State" and  who changed the VM
> to that state.  Then virtualmachinemanager must obey that until the external
> caller changes the attribute to now you can change state.  The would make
> more sense and decouples snapshotting from vm lifecycle management.
> Snapshotting then has it's own state (which I see it does already).  If we want
> to reflect that a vm snapshot is being taken of a VM, that's a function of the
> apiresponse module that gathers up everything about a vm but it shouldn't
> be changed in the vm states.
> > -  Does VM Revert operation work in the following way: Stop VM, restore
> to snapshot, and run VM?  Shouldn't this be orchestration inside snapshot
> manager?
> > - Does VM snapshot interfere with volume snapshot?  Volume snapshot
> today makes the assumption that it is the only code that's making snapshots
> and can break if there are additional snapshots in between.  This is bad
> design in volume snapshot but unfortunately that's how it's design.
> > - VMSnapshotService follows the other services in passing the cmds to the
> service.  That's really a bad practice that we should stop.  Cmds are really
> translations between over-the-wire api and java interface.  They shouldn't
> have been passed to down to the java interface.
> >
> > A small note: Would it be better to call it VM restore than VM revert?
> Revert really should be RevertTo which I think is in the code but is not
> consistent.  Some places it's just REVERT (for example, the event is just
> revert).
> >
> > --Alex
> >
> >> -----Original Message-----
> >> From: Chiradeep Vittal
> >> Sent: Wednesday, January 30, 2013 4:44 PM
> >> To: CloudStack DeveloperList
> >> Cc: Alex Huang
> >> Subject: Re: [MERGE] Support VM Snapshot
> >>
> >> Can we get Alex to review this? He is the designer of the state machine.
> >>
> >> On 1/30/13 5:26 AM, "Murali Reddy" <Murali.Reddy@citrix.com> wrote:
> >>
> >> >On 30/01/13 2:24 PM, "Mice Xia" <mice_xia@tcloudcomputing.com>
> wrote:
> >> >
> >> >>Agreed.
> >> >>Adding VM states are likely to have some side-effects, but for
> >> >>moveVMToUser case, does it explicitly reject other transient states
> >> >>such as stating/stopping/migrating?
> >> >>
> >> >>-Mice
> >> >
> >> >No, it just accepts any state other than 'Running' (though it should
> >> >have checked for the valid states in which VM can move to other user).
> >> >
> >> >I am just saying, there could such VM state based assumptions, you
> >> >might want to check.
> >> >
> >

Mime
View raw message