<div dir="ltr">Right. We should only apply this patch when the UI gets updated accordingly.<br></div><br><div class="gmail_quote">On Mon, Apr 27, 2015 at 12:37 PM Aline Manera <<a href="mailto:alinefm@linux.vnet.ibm.com">alinefm@linux.vnet.ibm.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
The UI needs to be changed accordingly, right?<br>
Otherwise, the user will get an unstable behaviour while reverting a<br>
snapshot with the current upstream UI<br>
<br>
On 20/04/2015 15:58, Crístian Deives wrote:<br>
> The action 'revert' is currently synchronous; that means that there's no<br>
> way for the user to run it in background and check its status<br>
> periodically. In some cases, reverting a snapshot can take a a few<br>
> seconds/minutes to finish, so the best option is to change it to an<br>
> asynchronous call.<br>
><br>
> Change the implementation of the action 'revert' so it returns an<br>
> asynchronous Task.<br>
><br>
> Signed-off-by: Crístian Deives <<a href="mailto:cristiandeives@gmail.com" target="_blank">cristiandeives@gmail.com</a>><br>
> ---<br>
> src/kimchi/control/vm/snapshots.py | 2 +-<br>
> src/kimchi/mockmodel.py | 11 +++++++++++<br>
> src/kimchi/model/vmsnapshots.py | 28 ++++++++++++++++++++--------<br>
> tests/test_model.py | 6 ++++--<br>
> tests/test_rest.py | 9 +++++++++<br>
> 5 files changed, 45 insertions(+), 11 deletions(-)<br>
><br>
> diff --git a/src/kimchi/control/vm/snapshots.py b/src/kimchi/control/vm/snapshots.py<br>
> index bbebc9a..4847d59 100644<br>
> --- a/src/kimchi/control/vm/snapshots.py<br>
> +++ b/src/kimchi/control/vm/snapshots.py<br>
> @@ -39,7 +39,7 @@ class VMSnapshot(Resource):<br>
> self.ident = ident<br>
> self.model_args = [self.vm, self.ident]<br>
> self.uri_fmt = '/vms/%s/snapshots/%s'<br>
> - self.revert = self.generate_action_handler('revert')<br>
> + self.revert = self.generate_action_handler_task('revert')<br>
><br>
> @property<br>
> def data(self):<br>
> diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py<br>
> index dcb8bc1..286f903 100644<br>
> --- a/src/kimchi/mockmodel.py<br>
> +++ b/src/kimchi/mockmodel.py<br>
> @@ -431,6 +431,15 @@ class MockModel(Model):<br>
> MockModel._mock_snapshots[vm_name] = snapshots<br>
><br>
> def _mock_vmsnapshot_revert(self, vm_name, name):<br>
> + params = {'vm_name': vm_name, 'name': name}<br>
> + taskid = add_task(u'/vms/%s/snapshots/%s/revert' % (vm_name, name),<br>
> + self._vmsnapshot_revert_task, self.objstore, params)<br>
> + return self.task_lookup(taskid)<br>
> +<br>
> + def _vmsnapshot_revert_task(self, cb, params):<br>
> + vm_name = params['vm_name']<br>
> + name = params['name']<br>
> +<br>
> snapshots = MockModel._mock_snapshots.get(vm_name, [])<br>
> for sn in snapshots:<br>
> if sn.current:<br>
> @@ -440,6 +449,8 @@ class MockModel(Model):<br>
> if <a href="http://sn.name" target="_blank">sn.name</a> == name:<br>
> sn.current = True<br>
><br>
> + cb('OK', True)<br>
> +<br>
><br>
> class MockStorageVolumes(object):<br>
> def __init__(self):<br>
> diff --git a/src/kimchi/model/vmsnapshots.py b/src/kimchi/model/vmsnapshots.py<br>
> index 3a92cdc..331f532 100644<br>
> --- a/src/kimchi/model/vmsnapshots.py<br>
> +++ b/src/kimchi/model/vmsnapshots.py<br>
> @@ -29,7 +29,6 @@ from kimchi.model.tasks import TaskModel<br>
> from kimchi.model.vms import DOM_STATE_MAP, VMModel<br>
> from kimchi.model.vmstorages import VMStorageModel, VMStoragesModel<br>
> from kimchi.utils import add_task<br>
> -from kimchi.xmlutils.utils import xpath_get_text<br>
><br>
><br>
> class VMSnapshotsModel(object):<br>
> @@ -120,6 +119,8 @@ class VMSnapshotsModel(object):<br>
> class VMSnapshotModel(object):<br>
> def __init__(self, **kargs):<br>
> self.conn = kargs['conn']<br>
> + self.objstore = kargs['objstore']<br>
> + self.task = TaskModel(**kargs)<br>
><br>
> def lookup(self, vm_name, name):<br>
> vir_snap = self.get_vmsnapshot(vm_name, name)<br>
> @@ -153,19 +154,30 @@ class VMSnapshotModel(object):<br>
> 'err': e.message})<br>
><br>
> def revert(self, vm_name, name):<br>
> + vir_dom = VMModel.get_vm(vm_name, self.conn)<br>
> + vir_snap = self.get_vmsnapshot(vm_name, name)<br>
> +<br>
> + task_params = {'name': name, 'vm_name': vm_name,<br>
> + 'dom': vir_dom, 'snap': vir_snap}<br>
> + taskid = add_task(u'/vms/%s/snapshots/%s/revert' % (vm_name, name),<br>
> + self._revert_task, self.objstore, task_params)<br>
> + return self.task.lookup(taskid)<br>
> +<br>
> + def _revert_task(self, cb, params):<br>
> + vir_dom = params['dom']<br>
> + vir_snap = params['snap']<br>
> +<br>
> try:<br>
> - vir_dom = VMModel.get_vm(vm_name, self.conn)<br>
> - vir_snap = self.get_vmsnapshot(vm_name, name)<br>
> + cb('reverting to snapshot')<br>
> vir_dom.revertToSnapshot(vir_snap, 0)<br>
> -<br>
> - # get vm name recorded in the snapshot and return new uri params<br>
> - vm_new_name = xpath_get_text(vir_snap.getXMLDesc(0),<br>
> - 'domain/name')[0]<br>
> - return [vm_new_name, name]<br>
> except libvirt.libvirtError, e:<br>
> + name = params['name']<br>
> + vm_name = params['vm_name']<br>
> +<br>
> raise OperationFailed('KCHSNAP0009E', {'name': name,<br>
> 'vm': vm_name,<br>
> 'err': e.message})<br>
> + cb('OK', True)<br>
><br>
> def get_vmsnapshot(self, vm_name, name):<br>
> vir_dom = VMModel.get_vm(vm_name, self.conn)<br>
> diff --git a/tests/test_model.py b/tests/test_model.py<br>
> index c25929f..b53822a 100644<br>
> --- a/tests/test_model.py<br>
> +++ b/tests/test_model.py<br>
> @@ -198,8 +198,10 @@ class ModelTests(unittest.TestCase):<br>
> snap = inst.vmsnapshot_lookup(u'kimchi-vm-new', params['name'])<br>
><br>
> # snapshot revert to the first created vm<br>
> - result = inst.vmsnapshot_revert(u'kimchi-vm-new', params['name'])<br>
> - self.assertEquals(result, [u'kimchi-vm', snap['name']])<br>
> + task = inst.vmsnapshot_revert(u'kimchi-vm-new', params['name'])<br>
> + inst.task_wait(task['id'])<br>
> + task = inst.task_lookup(task['id'])<br>
> + self.assertEquals('finished', task['status'])<br>
><br>
> vm = inst.vm_lookup(u'kimchi-vm')<br>
> self.assertEquals(vm['state'], snap['state'])<br>
> diff --git a/tests/test_rest.py b/tests/test_rest.py<br>
> index c1b81e8..3b592d6 100644<br>
> --- a/tests/test_rest.py<br>
> +++ b/tests/test_rest.py<br>
> @@ -382,6 +382,15 @@ class RestTests(unittest.TestCase):<br>
> # Revert to snapshot<br>
> resp = self.request('/vms/test-vm/snapshots/%s/revert' %<br>
> params['name'], '{}', 'POST')<br>
> + self.assertEquals(202, resp.status)<br>
> + task = json.loads(resp.read())<br>
> + wait_task(self._task_lookup, task['id'])<br>
> + resp = self.request('/tasks/%s' % task['id'], '{}', 'GET')<br>
> + task = json.loads(resp.read())<br>
> + self.assertEquals('finished', task['status'])<br>
> +<br>
> + resp = self.request('/vms/test-vm/snapshots/%s' % params['name'],<br>
> + '{}', 'GET')<br>
> self.assertEquals(200, resp.status)<br>
> snap = json.loads(resp.read())<br>
> resp = self.request('/vms/test-vm', '{}', 'GET')<br>
<br>
</blockquote></div>