Mice any comments to my email below. Animesh
> -----Original Message----- > From: Animesh Chaturvedi [mailto:animesh.chaturv...@citrix.com] > Sent: Wednesday, March 13, 2013 6:03 PM > To: Mice Xia; cloudstack-dev@incubator.apache.org; Chandan > Purushothama; Anthony Xu > Subject: RE: [MERGE] Support VM Snapshot > > Mice please see concerns from Anthony on Xenserver support., I guess they > can only be proved conclusively by testing with xenserver. Chandan will test > with xenserver. > > But given that there is some doubt on full support for xenserver and kvm > dependency on libvirt-java binding and I assume the effort in making the > capability configurable is trivial. In my opinion we should make it > configurable > and turn on for xenserver and kvm when support is clear. > > > Thanks > Animesh > > -----Original Message----- > > From: Mice Xia [mailto:weiran.x...@gmail.com] > > Sent: Monday, March 11, 2013 2:49 AM > > To: cloudstack-dev@incubator.apache.org > > Cc: Animesh Chaturvedi > > Subject: Re: [MERGE] Support VM Snapshot > > > > Animesh, > > > > sorry for the late reply. > > > > 1. currently in master branch it supports both xenserver and vmware, > > for KVM it needs a new-versioned libvirt java binding, considering > > it's for 4.2, still three months away from the release date, we have > > big chance to ship the new libvirt binding. I dont see a strong need > > to make it configurable for hypervisors unless some functions do not work > at all.. > > > > 2. now delta volume snapshot will become a full one. But I guess > > volume snapshot will be improved soon for xenserver. please correct me > > if we dont have this plan. > > > > Regards > > -Mice > > > > 2013/3/8 Animesh Chaturvedi <animesh.chaturv...@citrix.com>: > > > 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. > > >> > > >> > > > >> > > > > > >
