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

[Crístian Viana]

Overall, the patch is good, but I have few comments:

On 15-01-2015 01:22, gouzongmei at ourfuture.cn wrote:
> @@ -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,

I think you don't need to check "if result == 0"; if something goes 
wrong, an exception will be raised anyway. That's how we'd need to deal 
when using the C API but I guess it's not necessary with Python.

And you can also get the snapshot's name with the function 
"vir_snap.getName()", instead of reading its entire XML data and then 
parsing it for the name.

> +            result = inst.vmsnapshot_revert(u'kimchi-vm-new', snap['name'])
> +            self.assertEquals(result, [u'kimchi-vm', params_snap['name']])
> +

That new function "test_vm_snapshots" is basically what's already inside 
"test_vm_lifecycle". Those tests are just repeating themselves with the 
expcetion of the two lines I kept above, which test for the return value 
of "revert" which you changed. We can 1) add only that verification to 
the snapshot tests on "test_vm_lifecycle", or 2) move - not duplicate - 
the snapshot tests from "test_vm_lifecycle" to "test_vm_snapshots", with 
the new verification added.

We also need to have a test in "test_rest.py" which exploits the problem 
we're solving here. It's only during a REST request - not directly in 
the model - that the server calls "GET" on the resulting snapshot, which 
was causing a crash without this patch. But the tests above don't 
exploit that. The model function "vmsnapshot_revert" works with or 
without this patch (returning a different value, but it works), but the 
REST command "POST /vms/foo/snapshots/bar/revert" does not work without 
this patch and works with it, and that's what we need to test to make 
sure it won't come up as a regression.

Also, in my environment, the tests leaked the VM "kimchi-vm-new", which 
I needed to delete manually later.