Dan Kenigsberg has posted comments on this change. Change subject: vm: more generic recovery check ......................................................................
Patch Set 4: (5 comments) I am very sorry that I did not invested enough effort in my former review of this patch. It has occurred to me that I should not have accepted the usage of IndexError just to skip VMs with no devices (if this is indeed the motivation) and that the inter-module split of the isVDSMVm function should be discussed a bit more. http://gerrit.ovirt.org/#/c/31241/4/tests/vmTests.py File tests/vmTests.py: Line 1667: with FakeVM(self.conf, devices) as fake: Line 1668: self.assertRaises(ValueError, fake.buildConfDevices) Line 1669: Line 1670: Line 1671: class TestVmFunctions(TestCaseBase): It would be much preferable to test the top-level function isVDSMVm() than a very specific implementation. A test of a vmxml-built xml versus a generic one would have caught our bug much earlier. Line 1672: def testHasVDSMChannelsX86_64(self): Line 1673: vmId = CONF_TO_DOMXML_X86_64[0][0]['vmId'] Line 1674: domXml = CONF_TO_DOMXML_X86_64[0][1] % {'vmId': vmId} Line 1675: vmdom = parseString(domXml) Line 1681: vmdom = parseString(domXml) Line 1682: self.assertTrue(vm.hasVDSMChannels(vmdom)) Line 1683: Line 1684: def testHasNotVdsmChannels(self): Line 1685: vmId = CONF_TO_DOMXML_X86_64[0][0]['vmId'] # doesn't really matter the comment here is not very clear. Line 1686: domXml = DOMXML_NO_VDSM_CHANNEL % {'vmId': vmId} Line 1687: vmdom = parseString(domXml) http://gerrit.ovirt.org/#/c/31241/4/vdsm/clientIF.py File vdsm/clientIF.py: Line 477: except: Line 478: self.log.error("Vm's recovery failed", exc_info=True) Line 479: raise Line 480: Line 481: def isVDSMVm(self, vm): hasVdsmChannel is basically a helper function of isVDSMvm, but they are defined on different modules. I suppose that this was done so as not to expose _VMCHANNEL_DEVICE_NAME? How about moving this function to vm.py (in a follow-up patch)? It uses nothing of clientIF and a lot of vm.py. Then, hasVDSMChannels() can become private. Line 482: """ Line 483: Return True if vm seems as if it was created by vdsm. Line 484: """ Line 485: try: http://gerrit.ovirt.org/#/c/31241/4/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 101: _NO_CPU_PERIOD = 0 Line 102: Line 103: Line 104: def hasVDSMChannels(vmdom): Line 105: devices = vmdom.getElementsByTagName('devices')[0] the try-except IndexError in clientIF should be dropped in favor of devices = vmdom.getElementsByTagName('devices') if len(devices) == 1: devices = devices[0] else return False because if we miss <devices> or <target>, we can safely say that we do no have a VDSM channel. Line 106: for chan in devices.getElementsByTagName('channel'): Line 107: target = chan.getElementsByTagName('target')[0] Line 108: if target.getAttribute('name') == _VMCHANNEL_DEVICE_NAME: Line 109: return True Line 103: Line 104: def hasVDSMChannels(vmdom): Line 105: devices = vmdom.getElementsByTagName('devices')[0] Line 106: for chan in devices.getElementsByTagName('channel'): Line 107: target = chan.getElementsByTagName('target')[0] same goes with this [0] - if we get an empty list, we have no VDSM channel here. Line 108: if target.getAttribute('name') == _VMCHANNEL_DEVICE_NAME: Line 109: return True Line 110: Line 111: return False -- To view, visit http://gerrit.ovirt.org/31241 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic0cc57129e9c8e6545f9a947329adf1f9e82648f Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Eduardo <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
