incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nitin Mehta <Nitin.Me...@citrix.com>
Subject Re: [MERGE] Support VM Snapshot
Date Mon, 11 Mar 2013 03:34:50 GMT
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_xia@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.Mehta@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.xia1@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_xia@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.Huang@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.Reddy@citrix.com> wrote:
>>>>
>>>> >On 30/01/13 2:24 PM, "Mice Xia" <mice_xia@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.
>>>> >
>>>
>


Mime
View raw message