Overall, the patch is good, but I have few comments: On 15-01-2015 01:22, gouzongmei@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.