[Kimchi-devel] [PATCH] Solve the snapshot revert problem, relate to issue #526

[gouzongmei at ourfuture.cn]

From: Zongmei Gou <gouzongmei at ourfuture.cn>

This patch is partial for issue #526, in fact for another problem as Cristian mentioned:

    > There's still a known bug which happens when the user reverts a VM to a snapshot
    > when the VM's name has been changed since the snapshot was taken.In other words,
    > the following behavior will still cause a bug:
    >
        > - Create a VM named "foo".
        > - Create a snapshot of VM "foo" named "snap1".
        > - Rename VM "foo" to "bar".
        > - Revert VM "bar" to snapshot "snap1".
    >
    > The last step works correctly under the hood (i.e. the VM is reverted to that
    > snapshot) but Kimchi cannot return from its REST request because the VM
    > changed its name during the revert action.

The added test case(test_vm_snapshots) will get passed once Cristian's patch for issue #526 is applied.

Please ignore the patch sent by me last time, which contains Cristian's content.

---
 src/kimchi/control/base.py      | 14 +++++++++++---
 src/kimchi/model/vmsnapshots.py |  7 ++++++-
 tests/test_model.py             | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py
index 97e789f..484e9b9 100644
--- a/src/kimchi/control/base.py
+++ b/src/kimchi/control/base.py
@@ -57,10 +57,18 @@ class Resource(object):
         self.role_key = None
         self.admin_methods = []
 
-    def _redirect(self, ident, code=303):
-        if ident is not None and ident != self.ident:
+    def _redirect(self, action_result, code=303):
+        if isinstance(action_result, list):
+            uri_params = []
+            for arg in action_result:
+                if arg is None:
+                    arg = ''
+                uri_params.append(urllib2.quote(arg.encode('utf-8'), safe=""))
+            raise cherrypy.HTTPRedirect(self.uri_fmt % tuple(uri_params), code)
+        elif action_result is not None and action_result != self.ident:
             uri_params = list(self.model_args[:-1])
-            uri_params += [urllib2.quote(ident.encode('utf-8'), safe="")]
+            uri_params += [urllib2.quote(action_result.encode('utf-8'),
+                           safe="")]
             raise cherrypy.HTTPRedirect(self.uri_fmt % tuple(uri_params), code)
 
     def generate_action_handler(self, action_name, action_args=None):
diff --git a/src/kimchi/model/vmsnapshots.py b/src/kimchi/model/vmsnapshots.py
index 725770d..2c9345f 100644
--- a/src/kimchi/model/vmsnapshots.py
+++ b/src/kimchi/model/vmsnapshots.py
@@ -29,6 +29,7 @@ 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):
@@ -155,7 +156,11 @@ class VMSnapshotModel(object):
         try:
             vir_dom = VMModel.get_vm(vm_name, self.conn)
             vir_snap = self.get_vmsnapshot(vm_name, name)
-            vir_dom.revertToSnapshot(vir_snap, 0)
+            result = vir_dom.revertToSnapshot(vir_snap, 0)
+            if result == 0:
+                xmlObj = vir_snap.getXMLDesc(0)
+                vm_new_name = xpath_get_text(xmlObj, 'domain/name')[0]
+                return [vm_new_name, name]
         except libvirt.libvirtError, e:
             raise OperationFailed('KCHSNAP0009E', {'name': name,
                                                    'vm': vm_name,
diff --git a/tests/test_model.py b/tests/test_model.py
index c956007..d528b45 100644
--- a/tests/test_model.py
+++ b/tests/test_model.py
@@ -1074,6 +1074,44 @@ class ModelTests(unittest.TestCase):
             self.assertEquals([], inst.vm_lookup(u'пeω-∨м')['users'])
             self.assertEquals([], inst.vm_lookup(u'пeω-∨м')['groups'])
 
+    def test_vm_snapshots(self):
+        config.set("authentication", "method", "pam")
+        inst = model.Model(None,
+                           objstore_loc=self.tmp_store)
+
+        orig_params = {'name': 'test', 'memory': '1024', 'cpus': '1',
+                       'cdrom': self.kimchi_iso}
+        inst.templates_create(orig_params)
+
+        with RollbackContext() as rollback:
+            params = {'name': 'kimchi-vm', 'template': '/templates/test'}
+            inst.vms_create(params)
+            rollback.prependDefer(utils.rollback_wrapper, inst.vm_delete,
+                                  'kimchi-vm')
+
+            # create vm snapshot
+            params_snap = {'name': u'mysnap'}
+            task = inst.vmsnapshots_create(u'kimchi-vm', params_snap)
+            rollback.prependDefer(inst.vmsnapshot_delete,
+                                  u'kimchi-vm', params_snap['name'])
+            inst.task_wait(task['id'])
+            task = inst.task_lookup(task['id'])
+            self.assertEquals('finished', task['status'])
+
+            # update vm
+            params = {'name': u'kimchi-vm-new', 'cpus': 2, 'memory': 2048}
+            inst.vm_update('kimchi-vm', params)
+
+            info = inst.vm_lookup('kimchi-vm-new')
+            self.assertEquals(2048, info['memory'])
+            self.assertEquals(2, info['cpus'])
+
+            # snapshot revert
+            snap = inst.vmsnapshot_lookup(u'kimchi-vm-new',
+                                          params_snap['name'])
+            result = inst.vmsnapshot_revert(u'kimchi-vm-new', snap['name'])
+            self.assertEquals(result, [u'kimchi-vm', params_snap['name']])
+
     def test_multithreaded_connection(self):
         def worker():
             for i in xrange(100):
-- 
1.8.3.1