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 Fri, 08 Mar 2013 16:58:33 GMT
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