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. > >
