Francesco Romani has posted comments on this change. Change subject: vm: more generic recovery check ......................................................................
Patch Set 4: (5 comments) Yes, your intuition was right. The suggestions you made will lead to nicer and cleaner code. OTOH, this patch was backport-friendly, but the proposed changes should'nt make things worse on this regard, so I'll do it. 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 Agreed, will update the test once the other comments are addressed 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. will fix 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 def Agreed, is a thing I already planned. In a future patch I'll move away also getVDSMVms. 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 Agreed, will look nicer 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 Done 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
