Change in vdsm[master]: Refactored xmlrpcTests to allow for more complex tests using...

zhshzhou at linux.vnet.ibm.com zhshzhou at linux.vnet.ibm.com
Mon Aug 12 09:30:27 UTC 2013


Zhou Zheng Sheng has posted comments on this change.

Change subject: Refactored xmlrpcTests to allow for more complex tests using running VM
......................................................................


Patch Set 2: Code-Review-1

(5 comments)

Test runs successfully on my laptop.

Please consider checking the result of "self._s.destroy(self._id)". This is why I -1.

I like your idea in this patch. I think you can do better in future patch. When initramfs is missing, it is generated and destroyed per RunningVM. This takes much time. In future we can generate the initramfs in module setup [1] or class setup [2] method, so it is available for all the tests in this module or class. Another way is still generate it in RunningVM but cache the result in a class variable, then re-use the result. At last destroy it in module tearDown [1].

[1] http://nose.readthedocs.org/en/latest/writing_tests.html#test-modules
[2] http://nose.readthedocs.org/en/latest/writing_tests.html#test-classes

....................................................
File tests/functional/xmlrpcTests.py
Line 166
Line 167
Line 168
Line 169
Line 170
The empty VM test is to start a VM without kernel and initramfs, and see if libvirt and KVM is functional. In reality I have not met a situation that testStartEmptyVM() succeeds but testStartSmallVM() fails, so it's OK to remove this test and just leave testSimpleVM().


Line 109:         return (self._id, self._s.create(self._template))
Line 110: 
Line 111:     def __exit__(self, type, value, traceback):
Line 112:         if self._tempInitramfs:
Line 113:             os.unlink(self._initramfsPath)
Though it's rare, I think it's possible for os.unlike raises an exception, and this would skip the following destroy(), leaving a leftover VM. Maybe we can

try:
    if self._tempInitramfs:
        os.unlink(self._initramfsPath)
finally:
    self._s.assertVdsOK(self._s.destroy(self._id))
Line 114:         self._s.destroy(self._id)
Line 115: 
Line 116:     def _verifyBootImages(self, kernelPath, initramfsPath):
Line 117:         if not os.path.isfile(kernelPath):


Line 110: 
Line 111:     def __exit__(self, type, value, traceback):
Line 112:         if self._tempInitramfs:
Line 113:             os.unlink(self._initramfsPath)
Line 114:         self._s.destroy(self._id)
I think it's better to check the return value of destroy() as well.

 self._s.assertVdsOK(self._s.destroy(self._id))

This is because destroy() can fail when there is bug in VDSM. In the original _runVMKernelBootTemplate(), this check once helped me find a problem in vdsm/clientIF.py:removeVmFromMonitoredDomains . The bug could be triggered by destory(). There is a bugzilla for this problem [1], and Yeela fixed it. I think if we run functional tests before submitting patches, this kind of problem can be avoid early.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=990985
Line 115: 
Line 116:     def _verifyBootImages(self, kernelPath, initramfsPath):
Line 117:         if not os.path.isfile(kernelPath):
Line 118:             raise SkipTest("Can not locate kernel image for release %s" %


Line 112:         if self._tempInitramfs:
Line 113:             os.unlink(self._initramfsPath)
Line 114:         self._s.destroy(self._id)
Line 115: 
Line 116:     def _verifyBootImages(self, kernelPath, initramfsPath):
The method name is a bit misleading for me. At first I think "_verifyBootImages" should return True if it finds kernel image and initramfs are existed and readable. Actually it's the reverse. I get its real idea when I see "self._tempInitramfs = self._verifyBootImages ..." . So I think maybe "_missingBootImages" is a better name.
Line 117:         if not os.path.isfile(kernelPath):
Line 118:             raise SkipTest("Can not locate kernel image for release %s" %
Line 119:                            self._kernelVer)
Line 120:         if not readableBy(kernelPath, QEMU_PROCESS_USER):


Line 230:                 self._waitForStartup(vm)
Line 231: 
Line 232:     @skipNoKVM
Line 233:     @permutations([['hotplugNic']])
Line 234:     def testVm(self, devices=[]):
>From what I can see, "testVm"' is to test various devices by hot-plug, currently just hot-plug NIC, and more devices in future. So I think "testVmHotplugDevice" is a better name for this test.
Line 235:         customization = {'vmId': '77777777-ffff-3333-bbbb-222222222222',
Line 236:                          'vmName': 'testVm', 'devices': []}
Line 237: 
Line 238:         with RunningVm(self.s, customization) as (vm, status):


-- 
To view, visit http://gerrit.ovirt.org/17893
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I683c9e056137e7a189815bf6be2eb79ee80994cf
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpoledni at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvallare at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Peter V. Saveliev <peet at redhat.com>
Gerrit-Reviewer: Petr Ĺ ebek <psebek at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list