Mice - You might want to read the conversation at the link below http://markmail.org/message/u6nys3zo6w2rntqx?q=Scaling+up+CPU+and+RAM+for+r unning+VMs+list:org%2Eapache%2Eincubator%2Ecloudstack-dev+date:201303+&page =1
As Alex suggests that we already do some kind of syncronization when the API is invoked on a particular resource, we might be able to leverage that itself in our case. Thanks, -Nitin On 11/03/13 8:03 AM, "Mice Xia" <mice_...@tcloudcomputing.com> wrote: >Nitin, > >Could you give an example for this? >I'm not aware of how many operations that do not involve state change but >need lock VM, if there are many of them, perhaps we need a more generic >way to handle this, like adding lockVM()/unlockVM()/checkVMLock() methods >and use them in these non-state-change operations. But I'm worrying the >complexity, VM full/delta sync is already very complicated, not sure if >it's a good idea to handle lock sync in them. > >Regards >Mice > >-----Original Message----- >From: Nitin Mehta [mailto:nitin.me...@citrix.com] >Sent: Saturday, March 09, 2013 12:59 AM >To: cloudstack-dev@incubator.apache.org; Alex Huang >Cc: Anthony Xu >Subject: Re: [MERGE] Support VM Snapshot > >Not to nitpick but for better understanding of what Alex's comment on the >vm lifecycle. >I went through the commit and saw the code change below. This change >successfully avoids other vm state changes when the vm is in snapshotting >operation but what about the case when the user triggers "vm snapshot" >operation when there is some other state transition is happening or say >another vm operation which doesn't involve State change but requires the >resource to be locked during the operation is executing ? > > > @Override > public boolean stateTransitTo(VMInstanceVO vm, VirtualMachine.Event >e, Long hostId) throws NoTransitionException { >+ // if there are active vm snapshots task, state change is not >allowed >+ if(_vmSnapshotMgr.hasActiveVMSnapshotTasks(vm.getId())){ >+ s_logger.error("State transit with event: " + e + " failed >due to: " + vm.getInstanceName() + " has active VM snapshots tasks"); >+ return false; >+ } > > > >Thanks, >-Nitin > > >On 01/02/13 10:23 PM, "Mice Xia" <weiran.x...@gmail.com> wrote: > >>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. >>>> > >>> >
