Anthony, Thanks for your comments.
Tested on a datadisk with steps you provide on xenserver, all the files (test1, test2, test3) are present, the function is not affected. But as i have replied, volume snapshot (s2) is not a delta snapshot, it is a full one. Users need to be aware of this if they want to use both snapshots simultaneously. Regards Mice -----Original Message----- From: Anthony Xu [mailto:xuefei...@citrix.com] Sent: 2013-2-2 (星期六) 4:05 To: Alex Huang; Mice Xia; cloudstack-dev@incubator.apache.org Subject: RE: [MERGE] Support VM Snapshot CS uses XenServer delta snapshot, snapshot manager records a VHD chain in snapshot DB for each volume. VM snapshot creation/revert also operate on volume snapshot, if snapshot manager doesn't know the VM snapshot , volume snapshot might be broken. You can try following test, 1. create a VM. 2. create empty file test1 inside this VM. 3. create a volume snapshot(s1) 4. create empty file test2 inside this VM 5. create a VM snapshot (vm1) 6. create empty file test3 inside this VM 7. create a volume snapshot (s2) 8. create a volume from snapshot (s2) 9. attach this volume to a VM 10. if one of test1, test2, test3 is missing in this volume, might mean volume snapshot is broken. It might be difficult to support both VM snapshot and volume snapshot in the same time for hypervisor which supports delta snapshot. Maybe we need to provide a zone level configuration for it, only one is supported in a zone, volume snapshot or vm snapshot. Anthony > -----Original Message----- > From: Alex Huang > Sent: Friday, February 01, 2013 10:54 AM > To: Mice Xia; cloudstack-dev@incubator.apache.org > Cc: Anthony Xu > Subject: RE: [MERGE] Support VM Snapshot > > 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. > > >> > > > >
