Mailing-List: contact cloudstack-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: cloudstack-dev@incubator.apache.org
Received-SPF: pass (nike.apache.org: domain of Xuefei.Xu@citrix.com designates
 66.165.176.89 as permitted sender)
From: Anthony Xu <Xuefei.Xu@citrix.com>
To: Alex Huang <Alex.Huang@citrix.com>, Mice Xia <weiran.xia1@gmail.com>,
	"cloudstack-dev@incubator.apache.org" <cloudstack-dev@incubator.apache.org>
Date: Fri, 1 Feb 2013 12:05:55 -0800
Subject: RE: [MERGE] Support VM Snapshot
Thread-Topic: [MERGE] Support VM Snapshot
Thread-Index: Ac4AnK0iCIomWz9ERquSXvhxbIJb6gAELESQAAEFtWA=
Message-ID: 
 <B1DF26ECC0458748AC97CECE2DA98D41012FA47B62B6@SJCPMAILBOX01.citrite.net>
References: <CD2F1C53.2EF0B%murali.reddy@citrix.com>
	<CD2EFE29.42AE1%chiradeep.vittal@citrix.com>
	<B1DF26ECC0458748AC97CECE2DA98D41012FA4A6FAC1@SJCPMAILBOX01.citrite.net>
	<2C97F788CCC013428671BC3A5FC2F64D15AAB1@c-mail.cloud-valley.com.cn>
 <CALT7repypeekCsHxfvCeo2-WTSMf+7zb-QAwdfSYAPGEyHTt0Q@mail.gmail.com>
 <B1DF26ECC0458748AC97CECE2DA98D41012FA4A6FB8A@SJCPMAILBOX01.citrite.net>
In-Reply-To: 
 <B1DF26ECC0458748AC97CECE2DA98D41012FA4A6FB8A@SJCPMAILBOX01.citrite.net>
Accept-Language: en-US
Content-Language: en-US
acceptlanguage: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

CS uses XenServer delta snapshot, snapshot manager records a VHD chain in s=
napshot DB for each volume. VM snapshot creation/revert also operate on vol=
ume snapshot, if snapshot manager doesn't know the VM snapshot , volume sna=
pshot might be broken.


You can try following test,

1. create a VM.
2. create empty file test1 inside this VM.
3. create a volume snapshot(s1)
4. create empty file test2 inside this VM
5. create a VM snapshot (vm1)
6. create empty file test3 inside this VM
7. create a volume snapshot (s2)
8. create a volume from snapshot (s2)
9. attach this volume to a VM
10. if one of test1, test2, test3 is missing in this volume, might mean vol=
ume snapshot is broken.


It might be difficult to support both VM snapshot and volume snapshot in th=
e same time for hypervisor which supports delta snapshot.
Maybe we need to provide a zone level configuration for it, only one is sup=
ported in a zone, volume snapshot or vm snapshot.


Anthony


> -----Original Message-----
> From: Alex Huang
> Sent: Friday, February 01, 2013 10:54 AM
> To: Mice Xia; cloudstack-dev@incubator.apache.org
> Cc: Anthony Xu
> Subject: RE: [MERGE] Support VM Snapshot
>=20
> Mice,
>=20
> Thanks!
>=20
> Anthony,
>=20
> Can you comment on whether VM Snapshot breaks volume snapshot?
>=20
> --Alex
>=20
> > -----Original Message-----
> > From: Mice Xia [mailto:weiran.xia1@gmail.com]
> > Sent: Friday, February 01, 2013 8:53 AM
> > To: cloudstack-dev@incubator.apache.org; Alex Huang
> > Subject: Re: [MERGE] Support VM Snapshot
> >
> > 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.
> > >> >
> > >