Mice can you review and respond to my comment below > -----Original Message----- > From: Animesh Chaturvedi [mailto:animesh.chaturv...@citrix.com] > Sent: Thursday, March 07, 2013 5:54 PM > To: cloudstack-dev@incubator.apache.org; 'Mice Xia'; Alex Huang; Mice Xia > Cc: Sangeetha Hariharan > Subject: RE: [MERGE] Support VM Snapshot > > Folks following up on this thread looks like support for XenServer is still > not > settled. > > 1. Mice can we make the feature configurable for each hypervisor to > enable/disable the feature
2. Test the feature with XenServer thoroughly to > check if Volume Snapshot is affected / degraded > > Thanks > Animesh > > > > -----Original Message----- > > From: Anthony Xu [mailto:xuefei...@citrix.com] > > Sent: Friday, February 01, 2013 5:44 PM > > To: 'Mice Xia'; cloudstack-dev@incubator.apache.org; Alex Huang; Mice > > Xia > > Subject: RE: [MERGE] Support VM Snapshot > > > > I see, snapshot manager detected the change in primary storage, and > > create a full snapshot instead, which is supposed to be a delta snapshot. > > > > It doesn’t break volume snapshot function, but this degrades the > > volume snapshot performance. > > > > > > > > This is just a simple test, it cannot prove there is no impact to > > volume snapshot. > > > > I’m not sure what will happen if execute these two commands at the > > same time, is there any mechanism to sync/serialize these two operation? > > > > I’m not sure if revert VM has impact to volume snapshot. > > > > > > > > For now, it is better to have a global configuration to only choose one. > > > > later, we may support both of them in one setup. > > > > > > > > > > > > Anthony > > > > > > > > > > > > > > From: Mice Xia [mailto:mice_...@tcloudcomputing.com] > > Sent: Friday, February 01, 2013 5:30 PM > > To: cloudstack-dev@incubator.apache.org; Alex Huang; Mice Xia; Anthony > > Xu > > Subject: 答复: [MERGE] Support VM Snapshot > > > > > > 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. > > > > >> > > > > > >
