incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ahmad Emneina <aemne...@gmail.com>
Subject Re: [MERGE] Support VM Snapshot
Date Fri, 01 Feb 2013 02:05:36 GMT
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.Huang@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.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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message