[PATCH] Re: Solve the snapshot revert problem, relate to issue #526
From: Zongmei Gou <gouzongmei@ourfuture.cn> Hi, Cristian Thanks for your time on this, I've modified my patch according to your opinion, some problems may still exist. 1. The 'revert' function need return vm name recorded in the snapshot xml, not the snapshot name, so the "vir_snap.getName()" won't help. 2. Test case for vmsnapshot revert has been added to 'test_vm_lifecycle', and no duplicate cases exist now. 3. AFAIK, libvirt's Test driver does not support the function "virDomainListAllSnapshots", which cause 'test_edit_vm' failed in test_rest.py(once test our patch together), so if I catch this error(like 'delete' function on line 800 in vms.py), this case will pass. 4. For "test_rest", the snapshot revert function in mockmodel.py is different with the real model, which causes the response status different. So I just kept it, and all related test cases passed by now in my environment. Any suggestion about this issue is welcome, especially for 4, because I'm still a little confused with it. Thanks --- src/kimchi/control/base.py | 14 +++++++++++--- src/kimchi/model/vmsnapshots.py | 6 ++++++ tests/test_model.py | 12 ++++++++++-- 3 files changed, 27 insertions(+), 5 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..0c5b601 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): @@ -156,6 +157,11 @@ class VMSnapshotModel(object): vir_dom = VMModel.get_vm(vm_name, self.conn) vir_snap = self.get_vmsnapshot(vm_name, name) 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: raise OperationFailed('KCHSNAP0009E', {'name': name, 'vm': vm_name, diff --git a/tests/test_model.py b/tests/test_model.py index c956007..b9d71cf 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -176,8 +176,16 @@ class ModelTests(unittest.TestCase): current_snap = inst.currentvmsnapshot_lookup(u'kimchi-vm') self.assertEquals(snap, current_snap) - snap = inst.vmsnapshot_lookup(u'kimchi-vm', params['name']) - inst.vmsnapshot_revert(u'kimchi-vm', params['name']) + # update vm name + inst.vm_update('kimchi-vm', {'name': u'kimchi-vm-new'}) + + # Look up the first created snapshot from the renamed vm + 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']]) + vm = inst.vm_lookup(u'kimchi-vm') self.assertEquals(vm['state'], snap['state']) -- 1.8.3.1
Reviewed-by: Crístian Viana <vianac@linux.vnet.ibm.com> A few comments below: On 20-01-2015 10:10, gouzongmei@ourfuture.cn wrote:
From: Zongmei Gou <gouzongmei@ourfuture.cn>
Hi, Cristian
Thanks for your time on this, I've modified my patch according to your opinion, some problems may still exist.
1. The 'revert' function need return vm name recorded in the snapshot xml, not the snapshot name, so the "vir_snap.getName()" won't help.
You're right, we need the VM name there, not the snapshot name. So I guess you could call "vir_dom.name()" instead of "vir_snap.getXMLDesc() + xpath_get_text". I'm expecting that libvirt will return the updated value when it changes. But you don't need to send another revision of this patch just because of that, it already works as it is.
4. For "test_rest", the snapshot revert function in mockmodel.py is different with the real model, which causes the response status different. So I just kept it, and all related test cases passed by now in my environment.
Now I see that we cannot create that specific test scenario in our mockmodel, given its current implementation. Our MockSnapshot is too simple, it isn't aware of the original VM's name (and many more things), so unless we change our class MockSnapshot - which I don't think it's worth it now - we cannot reproduce that issue. So we can keep it as it is now, as you proposed.
On 21/01/2015 19:15, Crístian Viana wrote:
Reviewed-by: Crístian Viana <vianac@linux.vnet.ibm.com>
A few comments below:
On 20-01-2015 10:10, gouzongmei@ourfuture.cn wrote:
From: Zongmei Gou <gouzongmei@ourfuture.cn>
Hi, Cristian
Thanks for your time on this, I've modified my patch according to your opinion, some problems may still exist.
1. The 'revert' function need return vm name recorded in the snapshot xml, not the snapshot name, so the "vir_snap.getName()" won't help.
You're right, we need the VM name there, not the snapshot name. So I guess you could call "vir_dom.name()" instead of "vir_snap.getXMLDesc() + xpath_get_text". I'm expecting that libvirt will return the updated value when it changes. But you don't need to send another revision of this patch just because of that, it already works as it is.
4. For "test_rest", the snapshot revert function in mockmodel.py is different with the real model, which causes the response status different. So I just kept it, and all related test cases passed by now in my environment.
Now I see that we cannot create that specific test scenario in our mockmodel, given its current implementation. Our MockSnapshot is too simple, it isn't aware of the original VM's name (and many more things), so unless we change our class MockSnapshot - which I don't think it's worth it now - we cannot reproduce that issue. So we can keep it as it is now, as you proposed.
What are the consequences on it when running Kimchi on test mode?
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
On 22-01-2015 14:45, Aline Manera wrote:
Now I see that we cannot create that specific test scenario in our mockmodel, given its current implementation. Our MockSnapshot is too simple, it isn't aware of the original VM's name (and many more things), so unless we change our class MockSnapshot - which I don't think it's worth it now - we cannot reproduce that issue. So we can keep it as it is now, as you proposed.
What are the consequences on it when running Kimchi on test mode?
Under the mockmodel, we're not able to reproduce the same scenario the way it happens in the real model (i.e. on libvirt with QEMU), thus the tests don't test the real behaviour. The class MockSnapshot doesn't keep track of the original VM's name, so we cannot reproduce a situation where reverting to a snapshot changes the VM name, which is what this patch fixes. So the consequence is that the failing scenario cannot be tested under our test mode, only if we reproduce that situation manuall on a real model. Or unless we update the class MockSnapshot to be as close as a real snapshot.
participants (3)
-
Aline Manera -
Crístian Viana -
gouzongmei＠ourfuture.cn