Mice, Thanks!
Anthony, Can you comment on whether VM Snapshot breaks volume snapshot? --Alex > -----Original Message----- > From: Mice Xia [mailto:weiran.x...@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_...@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.hu...@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.re...@citrix.com> wrote: > >> > >> >On 30/01/13 2:24 PM, "Mice Xia" <mice_...@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. > >> > > >
