Change in vdsm[master]: vm: more generic recovery check
fromani at redhat.com
fromani at redhat.com
Mon Aug 11 16:03:44 UTC 2014
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 <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at gmail.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list