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