-----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
Mice,
Thanks!
Anthony,
Can you comment on whether VM Snapshot breaks volume snapshot?
--Alex
-----Original Message-----
From: Mice Xia [mailto:weiran.x...@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_...@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.hu...@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.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.
