Change in vdsm[master]: vm: more generic recovery check
danken at redhat.com
danken at redhat.com
Mon Aug 11 15:10:10 UTC 2014
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 <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