Awesome work Mice, +1 to immutable vm's, and who last called me states.
On Thu, Jan 31, 2013 at 5:23 PM, Alex Huang <alex.hu...@citrix.com> wrote: > 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. > > > > >
