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