3:50 p.m.
Overall, the patch is good, but I have few comments:
On 15-01-2015 01:22, gouzongmei(a)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.