Re: [Kimchi-devel] [PATCH 2/3] snapshot: Change the action 'revert' to a Task

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