Vinzenz Feenstra has uploaded a new change for review.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
vdsm: Refactoring of retrieving device info from xml
Reworked a bit the retrieval of device info from the libvirt domain xml. Now VDSM won't parse the code in lastXmlDesc every time and the retrieval of elements from the domain xml has been a bit abstracted. Additionally the retrieval of an alias has been moved into a separate function call to make the readability a bit better.
Change-Id: I7e106b2f2d3f4160d4e882f1a2880cb1b52fbb22 Signed-off-by: Vinzenz Feenstra vfeenstr@redhat.com --- M vdsm/vm.py 1 file changed, 63 insertions(+), 76 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/94/17694/1
diff --git a/vdsm/vm.py b/vdsm/vm.py index dc52909..e51050e 100644 --- a/vdsm/vm.py +++ b/vdsm/vm.py @@ -1698,6 +1698,7 @@ self._guestSocketFile = self._makeChannelPath(_VMCHANNEL_DEVICE_NAME) self._qemuguestSocketFile = self._makeChannelPath(_QEMU_GA_DEVICE_NAME) self._lastXMLDesc = '<domain><uuid>%s</uuid></domain>' % self.id + self._lastParsedXmlDesc = _domParseStr(self._lastXMLDesc) self._devXmlHash = '0' self._released = False self._releaseLock = threading.Lock() @@ -2722,24 +2723,30 @@ self._guestCpuRunning = (self._dom.info()[0] == libvirt.VIR_DOMAIN_RUNNING)
+ def _getDevicesXml(self, parsedXml=None): + parsedXml = parsedXml or self._lastParsedXmlDesc + return parsedXml.childNodes[0].getElementsByTagName('devices')[0] + def _getUnderlyingVmDevicesInfo(self): """ Obtain underlying vm's devices info from libvirt. """ - self._getUnderlyingNetworkInterfaceInfo() - self._getUnderlyingDriveInfo() - self._getUnderlyingDisplayPort() - self._getUnderlyingSoundDeviceInfo() - self._getUnderlyingVideoDeviceInfo() - self._getUnderlyingControllerDeviceInfo() - self._getUnderlyingBalloonDeviceInfo() - self._getUnderlyingWatchdogDeviceInfo() - self._getUnderlyingSmartcardDeviceInfo() - self._getUnderlyingConsoleDeviceInfo() + devicesXml = self._getDevicesXml(parsedXml=self._lastParsedXmlDesc) + self._getUnderlyingNetworkInterfaceInfo(devicesXml=devicesXml) + self._getUnderlyingDriveInfo(devicesXml=devicesXml) + self._getUnderlyingDisplayPort(xml=self._lastParsedXmlDesc) + self._getUnderlyingSoundDeviceInfo(devicesXml=devicesXml) + self._getUnderlyingVideoDeviceInfo(devicesXml=devicesXml) + self._getUnderlyingControllerDeviceInfo(devicesXml=devicesXml) + self._getUnderlyingBalloonDeviceInfo(devicesXml=devicesXml) + self._getUnderlyingWatchdogDeviceInfo(devicesXml=devicesXml) + self._getUnderlyingSmartcardDeviceInfo(devicesXml=devicesXml) + self._getUnderlyingConsoleDeviceInfo(devicesXml=devicesXml) # Obtain info of all unknown devices. Must be last! - self._getUnderlyingUnknownDeviceInfo() + self._getUnderlyingUnknownDeviceInfo(devicesXml=devicesXml) + self._updateAgentChannels(devicesXml=devicesXml)
- def _updateAgentChannels(self): + def _updateAgentChannels(self, devicesXml): """ We moved the naming of guest agent channel sockets. To keep backwards compatability we need to make symlinks from the old channel sockets, to @@ -2747,9 +2754,7 @@ This is necessary to prevent incoming migrations, restoring of VMs and the upgrade of VDSM with running VMs to fail on this. """ - agentChannelXml = _domParseStr(self._lastXMLDesc).childNodes[0]. \ - getElementsByTagName('devices')[0]. \ - getElementsByTagName('channel') + agentChannelXml = devicesXml.getElementsByTagName('channel') for channel in agentChannelXml: try: name = channel.getElementsByTagName('target')[0].\ @@ -2781,7 +2786,6 @@
self._getUnderlyingVmInfo() self._getUnderlyingVmDevicesInfo() - self._updateAgentChannels()
#Currently there is no protection agains mirroring a network twice, for nic in self._devices[NIC_DEVICES]: @@ -2937,9 +2941,8 @@ or revert to snapshot. """ parsedSrcDomXML = _domParseStr(srcDomXML) - - allDiskDeviceXmlElements = parsedSrcDomXML.childNodes[0]. \ - getElementsByTagName('devices')[0].getElementsByTagName('disk') + devicesXml = self._getDevicesXml(parsedXml=parsedSrcDomXML) + allDiskDeviceXmlElements = devicesXml.getElementsByTagName('disk')
snappableDiskDeviceXmlElements = \ _filterSnappableDiskDevices(allDiskDeviceXmlElements) @@ -3008,7 +3011,8 @@ with self._confLock: self.conf['devices'].append(nicParams) self.saveState() - self._getUnderlyingNetworkInterfaceInfo() + self._getUnderlyingNetworkInterfaceInfo( + devicesXml=self._getDevicesXml()) hooks.after_nic_hotplug(nicXml, self.conf, params=customProps)
@@ -3264,7 +3268,7 @@ with self._confLock: self.conf['devices'].append(diskParams) self.saveState() - self._getUnderlyingDriveInfo() + self._getUnderlyingDriveInfo(devicesXml=self._getDevicesXml()) hooks.after_disk_hotplug(driveXml, self.conf, params=customProps)
@@ -4181,8 +4185,8 @@
def _getUnderlyingVmInfo(self): self._lastXMLDesc = self._dom.XMLDesc(0) - devxml = _domParseStr(self._lastXMLDesc).childNodes[0]. \ - getElementsByTagName('devices')[0] + self._lastParsedXmlDesc = _domParseStr(self._lastXMLDesc) + devxml = self._getDevicesXml() self._devXmlHash = str(hash(devxml.toxml()))
return self._lastXMLDesc @@ -4331,6 +4335,9 @@ self.saveState() return {'status': doneCode}
+ def _getUnderlyingDeviceAliasName(self, devXml): + return devXml.getElementsByTagName('alias')[0].getAttribute('name') + def _getUnderlyingDeviceAddress(self, devXml): """ Obtain device's address from libvirt @@ -4347,7 +4354,7 @@
return address
- def _getUnderlyingUnknownDeviceInfo(self): + def _getUnderlyingUnknownDeviceInfo(self, devicesXml): """ Obtain unknown devices info from libvirt.
@@ -4360,16 +4367,13 @@ return True return False
- devsxml = _domParseStr(self._lastXMLDesc).childNodes[0]. \ - getElementsByTagName('devices')[0] - - for x in devsxml.childNodes: + for x in devicesXml.childNodes: # Ignore empty nodes and devices without address if (x.nodeName == '#text' or not x.getElementsByTagName('address')): continue
- alias = x.getElementsByTagName('alias')[0].getAttribute('name') + alias = self._getUnderlyingDeviceAliasName(x) if not isKnownDevice(alias): address = self._getUnderlyingDeviceAddress(x) # I general case we assume that device has attribute 'type', @@ -4381,18 +4385,16 @@ 'address': address} self.conf['devices'].append(newDev)
- def _getUnderlyingControllerDeviceInfo(self): + def _getUnderlyingControllerDeviceInfo(self, devicesXml): """ Obtain controller devices info from libvirt. """ - ctrlsxml = _domParseStr(self._lastXMLDesc).childNodes[0]. \ - getElementsByTagName('devices')[0]. \ - getElementsByTagName('controller') + ctrlsxml = devicesXml.getElementsByTagName('controller') for x in ctrlsxml: # Ignore controller devices without address if not x.getElementsByTagName('address'): continue - alias = x.getElementsByTagName('alias')[0].getAttribute('name') + alias = self._getUnderlyingDeviceAliasName(x) device = x.getAttribute('type') # Get model and index. Relevant for USB controllers. model = x.getAttribute('model') @@ -4428,20 +4430,18 @@ 'address': address, 'alias': alias})
- def _getUnderlyingBalloonDeviceInfo(self): + def _getUnderlyingBalloonDeviceInfo(self, devicesXml): """ Obtain balloon device info from libvirt. """ - balloonxml = _domParseStr(self._lastXMLDesc).childNodes[0]. \ - getElementsByTagName('devices')[0]. \ - getElementsByTagName('memballoon') + balloonxml = devicesXml.getElementsByTagName('memballoon') for x in balloonxml: # Ignore balloon devices without address. if not x.getElementsByTagName('address'): address = None else: address = self._getUnderlyingDeviceAddress(x) - alias = x.getElementsByTagName('alias')[0].getAttribute('name') + alias = self._getUnderlyingDeviceAliasName(x)
for dev in self._devices[BALLOON_DEVICES]: if address and not hasattr(dev, 'address'): @@ -4456,16 +4456,14 @@ if not dev.get('alias'): dev['alias'] = alias
- def _getUnderlyingConsoleDeviceInfo(self): + def _getUnderlyingConsoleDeviceInfo(self, devicesXml): """ Obtain the alias for the console device from libvirt """ - consolexml = _domParseStr(self._lastXMLDesc).childNodes[0].\ - getElementsByTagName('devices')[0].\ - getElementsByTagName('console') + consolexml = devicesXml.getElementsByTagName('console') for x in consolexml: # All we care about is the alias - alias = x.getElementsByTagName('alias')[0].getAttribute('name') + alias = self._getUnderlyingDeviceAliasName(x) for dev in self._devices[CONSOLE_DEVICES]: if not hasattr(dev, 'alias'): dev.alias = alias @@ -4475,19 +4473,17 @@ not dev.get('alias'): dev['alias'] = alias
- def _getUnderlyingSmartcardDeviceInfo(self): + def _getUnderlyingSmartcardDeviceInfo(self, devicesXml): """ Obtain smartcard device info from libvirt. """ - smartcardxml = _domParseStr(self._lastXMLDesc).childNodes[0].\ - getElementsByTagName('devices')[0].\ - getElementsByTagName('smartcard') + smartcardxml = devicesXml.getElementsByTagName('smartcard') for x in smartcardxml: if not x.getElementsByTagName('address'): continue
address = self._getUnderlyingDeviceAddress(x) - alias = x.getElementsByTagName('alias')[0].getAttribute('name') + alias = self._getUnderlyingDeviceAliasName(x)
for dev in self._devices[SMARTCARD_DEVICES]: if not hasattr(dev, 'address'): @@ -4500,19 +4496,17 @@ dev['address'] = address dev['alias'] = alias
- def _getUnderlyingWatchdogDeviceInfo(self): + def _getUnderlyingWatchdogDeviceInfo(self, devicesXml): """ Obtain watchdog device info from libvirt. """ - watchdogxml = _domParseStr(self._lastXMLDesc).childNodes[0]. \ - getElementsByTagName('devices')[0]. \ - getElementsByTagName('watchdog') + watchdogxml = devicesXml.getElementsByTagName('watchdog') for x in watchdogxml:
# PCI watchdog has "address" different from ISA watchdog if x.getElementsByTagName('address'): address = self._getUnderlyingDeviceAddress(x) - alias = x.getElementsByTagName('alias')[0].getAttribute('name') + alias = self._getUnderlyingDeviceAliasName(x)
for wd in self._devices[WATCHDOG_DEVICES]: if not hasattr(wd, 'address') or not hasattr(wd, 'alias'): @@ -4525,14 +4519,13 @@ dev['address'] = address dev['alias'] = alias
- def _getUnderlyingVideoDeviceInfo(self): + def _getUnderlyingVideoDeviceInfo(self, devicesXml): """ Obtain video devices info from libvirt. """ - videosxml = _domParseStr(self._lastXMLDesc).childNodes[0]. \ - getElementsByTagName('devices')[0].getElementsByTagName('video') + videosxml = devicesXml.getElementsByTagName('video') for x in videosxml: - alias = x.getElementsByTagName('alias')[0].getAttribute('name') + alias = self._getUnderlyingDeviceAliasName(x) # Get video card address address = self._getUnderlyingDeviceAddress(x)
@@ -4553,14 +4546,13 @@ dev['alias'] = alias break
- def _getUnderlyingSoundDeviceInfo(self): + def _getUnderlyingSoundDeviceInfo(self, devicesXml): """ Obtain sound devices info from libvirt. """ - soundsxml = _domParseStr(self._lastXMLDesc).childNodes[0]. \ - getElementsByTagName('devices')[0].getElementsByTagName('sound') + soundsxml = devicesXml.getElementsByTagName('sound') for x in soundsxml: - alias = x.getElementsByTagName('alias')[0].getAttribute('name') + alias = self._getUnderlyingDeviceAliasName(x) # Get sound card address address = self._getUnderlyingDeviceAddress(x)
@@ -4581,12 +4573,11 @@ dev['alias'] = alias break
- def _getUnderlyingDriveInfo(self): + def _getUnderlyingDriveInfo(self, devicesXml): """ Obtain block devices info from libvirt. """ - disksxml = _domParseStr(self._lastXMLDesc).childNodes[0]. \ - getElementsByTagName('devices')[0].getElementsByTagName('disk') + disksxml = devicesXml.getElementsByTagName('disk') # FIXME! We need to gather as much info as possible from the libvirt. # In the future we can return this real data to management instead of # vm's conf @@ -4600,7 +4591,7 @@
target = x.getElementsByTagName('target') name = target[0].getAttribute('dev') if target else '' - alias = x.getElementsByTagName('alias')[0].getAttribute('name') + alias = self._getUnderlyingDeviceAliasName(x) readonly = bool(x.getElementsByTagName('readonly')) boot = x.getElementsByTagName('boot') bootOrder = boot[0].getAttribute('order') if boot else '' @@ -4646,12 +4637,11 @@ diskDev['bootOrder'] = bootOrder self.conf['devices'].append(diskDev)
- def _getUnderlyingDisplayPort(self): + def _getUnderlyingDisplayPort(self, xml): """ Obtain display port info from libvirt. """ - graphics = _domParseStr(self._lastXMLDesc).childNodes[0]. \ - getElementsByTagName('graphics')[0] + graphics = xml.childNodes[0].getElementsByTagName('graphics')[0] port = graphics.getAttribute('port') if port: self.conf['displayPort'] = port @@ -4659,18 +4649,16 @@ if port: self.conf['displaySecurePort'] = port
- def _getUnderlyingNetworkInterfaceInfo(self): + def _getUnderlyingNetworkInterfaceInfo(self, devicesXml): """ Obtain network interface info from libvirt. """ # TODO use xpath instead of parseString (here and elsewhere) - ifsxml = _domParseStr(self._lastXMLDesc).childNodes[0]. \ - getElementsByTagName('devices')[0]. \ - getElementsByTagName('interface') + ifsxml = devicesXml.getElementsByTagName('interface') for x in ifsxml: devType = x.getAttribute('type') mac = x.getElementsByTagName('mac')[0].getAttribute('address') - alias = x.getElementsByTagName('alias')[0].getAttribute('name') + alias = self._getUnderlyingDeviceAliasName(x) if devType == 'hostdev': name = alias model = 'passthrough' @@ -4802,8 +4790,7 @@ "during migration at destination host" % devType)
- devices = _domParseStr(xml).childNodes[0]. \ - getElementsByTagName('devices')[0] + devices = self._getDevicesXml(parsedXml=_domParseStr(xml))
for deviceXML in devices.childNodes: if deviceXML.nodeType != Node.ELEMENT_NODE:
Vinzenz Feenstra has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 1: Verified+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 1: Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3725/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3641/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2834/ : FAILURE
Vinzenz Feenstra has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 2: Verified+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 2:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3731/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3647/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2840/ : SUCCESS
Peter V. Saveliev has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 2: Code-Review+1
Antoni Segura Puimedon has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 2:
(1 comment)
A very minor comment.
As a side note, I feel that there is potential for doing all this xml handling happen more implicitly without passing the stuff around.
.................................................... File vdsm/vm.py Line 2730: def _getUnderlyingVmDevicesInfo(self): Line 2731: """ Line 2732: Obtain underlying vm's devices info from libvirt. Line 2733: """ Line 2734: devicesXml = self._getDevicesXml(parsedXml=self._lastParsedXmlDesc) wouldn't it be the same without passing it? Line 2735: self._getUnderlyingNetworkInterfaceInfo(devicesXml=devicesXml) Line 2736: self._getUnderlyingDriveInfo(devicesXml=devicesXml) Line 2737: self._getUnderlyingDisplayPort(xml=self._lastParsedXmlDesc) Line 2738: self._getUnderlyingSoundDeviceInfo(devicesXml=devicesXml)
Vinzenz Feenstra has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 2:
(1 comment)
There's a reason for having the parameter. You can pass an arbitrary passed XML. Before it always used the stored one.
If we want to retrieve the information from a different XML (e.g. conf['_srcDomXML'] during an incoming migratoin we'd have otherwise to set the value internally to lastParsedXMlDesc which I don't think is a good idea.
Besides that is my plan to move the getUnderlying*DeviceInfo to the dedicated device classes
.................................................... File vdsm/vm.py Line 2730: def _getUnderlyingVmDevicesInfo(self): Line 2731: """ Line 2732: Obtain underlying vm's devices info from libvirt. Line 2733: """ Line 2734: devicesXml = self._getDevicesXml(parsedXml=self._lastParsedXmlDesc) In this case yes, I will fix this Line 2735: self._getUnderlyingNetworkInterfaceInfo(devicesXml=devicesXml) Line 2736: self._getUnderlyingDriveInfo(devicesXml=devicesXml) Line 2737: self._getUnderlyingDisplayPort(xml=self._lastParsedXmlDesc) Line 2738: self._getUnderlyingSoundDeviceInfo(devicesXml=devicesXml)
oVirt Jenkins CI Server has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 3: Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3758/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3674/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2867/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 4:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3761/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3677/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2870/ : SUCCESS
Michal Skrivanek has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 4:
(2 comments)
.................................................... File vdsm/vm.py Line 1697: self.conf['vmName'] = 'n%s' % self.id Line 1698: self._guestSocketFile = self._makeChannelPath(_VMCHANNEL_DEVICE_NAME) Line 1699: self._qemuguestSocketFile = self._makeChannelPath(_QEMU_GA_DEVICE_NAME) Line 1700: self._lastXMLDesc = '<domain><uuid>%s</uuid></domain>' % self.id Line 1701: self._lastParsedXmlDesc = _domParseStr(self._lastXMLDesc) can we maybe keep it together? or do we still need the last non-parsed xml? Line 1702: self._devXmlHash = '0' Line 1703: self._released = False Line 1704: self._releaseLock = threading.Lock() Line 1705: self.saveState()
Line 2742: self._getUnderlyingBalloonDeviceInfo(devicesXml=devicesXml) Line 2743: self._getUnderlyingWatchdogDeviceInfo(devicesXml=devicesXml) Line 2744: self._getUnderlyingSmartcardDeviceInfo(devicesXml=devicesXml) Line 2745: self._getUnderlyingConsoleDeviceInfo(devicesXml=devicesXml) Line 2746: # Obtain info of all unknown devices. Must be last! is it?;-) Line 2747: self._getUnderlyingUnknownDeviceInfo(devicesXml=devicesXml) Line 2748: self._updateAgentChannels(devicesXml=devicesXml) Line 2749: Line 2750: def _updateAgentChannels(self, devicesXml):
Peter V. Saveliev has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 4: Code-Review+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 5:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3792/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3708/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2901/ : SUCCESS
Vinzenz Feenstra has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 6: Verified+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 6:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4400/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3503/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4319/ : SUCCESS
Vinzenz Feenstra has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 7: Verified+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 7:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3820/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4705/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4629/ : SUCCESS
Itamar Heim has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 7:
ping?
Francesco Romani has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 7: Code-Review+1
I like the idea.
Francesco Romani has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 7:
(1 comment)
http://gerrit.ovirt.org/#/c/17694/7/vdsm/vm.py File vdsm/vm.py:
Line 2794: self._getUnderlyingBalloonDeviceInfo(devicesXml=devicesXml) Line 2795: self._getUnderlyingWatchdogDeviceInfo(devicesXml=devicesXml) Line 2796: self._getUnderlyingSmartcardDeviceInfo(devicesXml=devicesXml) Line 2797: self._getUnderlyingConsoleDeviceInfo(devicesXml=devicesXml) Line 2798: self._updateAgentChannels(devicesXml=devicesXml) Probably better to save this for another patch. Line 2799: # Obtain info of all unknown devices. Must be last! Line 2800: self._getUnderlyingUnknownDeviceInfo(devicesXml=devicesXml) Line 2801: Line 2802: def _updateAgentChannels(self, devicesXml):
Dan Kenigsberg has posted comments on this change.
Change subject: vdsm: Refactoring of retrieving device info from xml ......................................................................
Patch Set 7: Code-Review-1
(1 comment)
http://gerrit.ovirt.org/#/c/17694/7//COMMIT_MSG Commit Message:
Line 3: AuthorDate: 2013-08-05 10:36:28 +0200 Line 4: Commit: Gerrit Code Review gerrit2@gerrit.ovirt.org Line 5: CommitDate: 2013-10-01 03:56:14 -0400 Line 6: Line 7: vdsm: Refactoring of retrieving device info from xml Could you explain the benefit of the new code? it is not self-evident (at least not to me). I would not mind adding as self._lastXMLDescParsed for efficiency, and I would not mind creating a class wrapping the two together, something like
class ParsedDomXML(object): def __init__(self, text): self.text = text self.dom = _domParseStr(text) self.devs = self.dom.childNodes[0].getElementsByTagName('devices')[0] Line 8: Line 9: Reworked a bit the retrieval of device info from the libvirt domain xml. Line 10: Now VDSM won't parse the code in lastXmlDesc every time and the retrieval Line 11: of elements from the domain xml has been a bit abstracted.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 8:
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6730/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7520/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7630/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 9:
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6731/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7521/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7631/ : FAILURE
Francesco Romani has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 10:
(2 comments)
just a couple of nits. I'm really looking forward to have this merged!
http://gerrit.ovirt.org/#/c/17694/10/vdsm/vm.py File vdsm/vm.py:
Line 464: Line 465: def set(self, xml): Line 466: self._xml = xml Line 467: self._dom = _domParseStr(xml) Line 468: devices = self.firstElementByTagName('devices') Unneeded temporary? Line 469: self._devices = devices Line 470: Line 471: def xml(self): Line 472: return self._xml
Line 476: Line 477: def firstElementByTagName(self, tagName): Line 478: return self._dom.childNodes[0].getElementsByTagName(tagName)[0] Line 479: Line 480: def devs(self): nit: 'devices' is better IMO. Also wondering if we make a property of this one, 'dom' and 'xml'. This will lead to clearer code and save us a temporary (line 3281) and a few parens here and there. Line 481: return self._devices Line 482: Line 483: def getDeviceElements(self, tagName): Line 484: return self._devices.getElementsByTagName(tagName)
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 10:
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6732/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7522/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7632/ : FAILURE
Martin Sivák has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 10:
(3 comments)
http://gerrit.ovirt.org/#/c/17694/10/vdsm/vm.py File vdsm/vm.py:
Line 461: class ParsedDomXML(object): Line 462: def __init__(self, xml): Line 463: self.set(xml) Line 464: Line 465: def set(self, xml): I would probably make the class "read-only" by moving this to the constructor and requiring the user to create a new instance of the class every time a new XML is needed. Line 466: self._xml = xml Line 467: self._dom = _domParseStr(xml) Line 468: devices = self.firstElementByTagName('devices') Line 469: self._devices = devices
Line 476: Line 477: def firstElementByTagName(self, tagName): Line 478: return self._dom.childNodes[0].getElementsByTagName(tagName)[0] Line 479: Line 480: def devs(self):
nit: 'devices' is better IMO.
I agree. Properties would make it nicer. Line 481: return self._devices Line 482: Line 483: def getDeviceElements(self, tagName): Line 484: return self._devices.getElementsByTagName(tagName)
Line 4631: return pid Line 4632: Line 4633: def _getUnderlyingVmInfo(self): Line 4634: self._lastXMLDesc.set(self._dom.XMLDesc(0)) Line 4635: self._devXmlHash = str(hash(self._lastXMLDesc.devs().toxml())) You can add the hash to the ParsedXMLDom class as a property and compute it when it is accessed for the first time. Line 4636: Line 4637: def _ejectFloppy(self): Line 4638: if 'volatileFloppy' in self.conf: Line 4639: utils.rmFile(self.conf['floppy'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 11:
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6741/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7531/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7641/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 12:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6743/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7533/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7643/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 13:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6748/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7538/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7648/ : SUCCESS
Francesco Romani has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 13: Code-Review+1
Martin Sivák has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 13:
(2 comments)
http://gerrit.ovirt.org/#/c/17694/13/vdsm/vm.py File vdsm/vm.py:
Line 485: return self._devices.getElementsByTagName(tagName) Line 486: Line 487: @property Line 488: def hash(self): Line 489: if self.devices and not self._hash: Why not "self._hash is not None"? Line 490: self._hash = str(hash(self._devices.toxml())) Line 491: return self._hash or '0' Line 492: Line 493:
Line 487: @property Line 488: def hash(self): Line 489: if self.devices and not self._hash: Line 490: self._hash = str(hash(self._devices.toxml())) Line 491: return self._hash or '0' Why the or in return? Just compute the hash and save it. Line 492: Line 493: Line 494: def eventToString(event): Line 495: return _EVENT_STRINGS[event]
Nir Soffer has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 13: Code-Review-1
(8 comments)
This is a very nice patch, but it needs some more work.
http://gerrit.ovirt.org/#/c/17694/13/vdsm/vm.py File vdsm/vm.py:
Line 457: "Shutdown", Line 458: "PM-Suspended") Line 459: Line 460: Line 461: class ParsedDomXML(object): While this name is clear, a name like LibvirtDomain or DomainDescriptor may be nicer. Line 462: def __init__(self, xml): Line 463: self._hash = None Line 464: self._xml = xml Line 465: self._dom = _domParseStr(xml)
Line 462: def __init__(self, xml): Line 463: self._hash = None Line 464: self._xml = xml Line 465: self._dom = _domParseStr(xml) Line 466: self._devices = self.firstElementByTagName('devices') Since we assume that self._xml, self._dom, and self._devices never change, we can simply create self._hash here and return it in hash(). Since hashing is cheap, this should be good enough. Line 467: Line 468: @property Line 469: def xml(self): Line 470: return self._xml
Line 484: def getDeviceElements(self, tagName): Line 485: return self._devices.getElementsByTagName(tagName) Line 486: Line 487: @property Line 488: def hash(self): This should be named devicesHash - this is not a hash of the xml. Can we use the much cheaper hash(self._xml) Line 489: if self.devices and not self._hash: Line 490: self._hash = str(hash(self._devices.toxml())) Line 491: return self._hash or '0' Line 492:
Line 486: Line 487: @property Line 488: def hash(self): Line 489: if self.devices and not self._hash: Line 490: self._hash = str(hash(self._devices.toxml())) For another patch: I'm not sure how we use this hash - but it is not good for detecting changes in the xml. For this we should use md5 or sha1. Line 491: return self._hash or '0' Line 492: Line 493: Line 494: def eventToString(event):
Line 2033: if 'vmName' not in self.conf: Line 2034: self.conf['vmName'] = 'n%s' % self.id Line 2035: self._guestSocketFile = self._makeChannelPath(_VMCHANNEL_DEVICE_NAME) Line 2036: self._qemuguestSocketFile = self._makeChannelPath(_QEMU_GA_DEVICE_NAME) Line 2037: self._lastXMLDesc = ParsedDomXML( For next patch - rename lastXMLDesc to something nicer? Line 2038: '<domain><uuid>%s</uuid></domain>' % self.id) Line 2039: self._released = False Line 2040: self._releaseLock = threading.Lock() Line 2041: self.saveState()
Line 3292: Line 3293: for snappableDiskDeviceXmlElement in snappableDiskDeviceXmlElements: Line 3294: self._changeDisk(snappableDiskDeviceXmlElement) Line 3295: Line 3296: return parsedSrcDomXML.dom.toxml() This usage is bad - we take a read-only object (ParsedDomXML), modify it's dom behind it's back, and then accessing the dom to create a new xml.
I think we should leave the old code as is, and remove the dom property, used only by this function. Line 3297: Line 3298: def _changeDisk(self, diskDeviceXmlElement): Line 3299: diskType = diskDeviceXmlElement.getAttribute('type') Line 3300:
Line 4634: pass Line 4635: return pid Line 4636: Line 4637: def _getUnderlyingVmInfo(self): Line 4638: self._lastXMLDesc = ParsedDomXML(self._dom.XMLDesc(0)) This function used to return self._lastXMLDesc?!
If this should not have a return value, add another patch removing the return value and renaming this to something like initUnderlyingVmInfo.
Minor thing: it is not clear what is self._dom.XMLDesc(0). An explaining variable may help here:
explaining_name = self._dom.XMLDesc(0) self._lastXMLDesc = ParsedDomXML(explaining_name) Line 4639: Line 4640: def _ejectFloppy(self): Line 4641: if 'volatileFloppy' in self.conf: Line 4642: utils.rmFile(self.conf['floppy'])
Line 5108: def _getUnderlyingDisplayPort(self): Line 5109: """ Line 5110: Obtain display port info from libvirt. Line 5111: """ Line 5112: graphics = self._lastXMLDesc.firstElementByTagName('graphics') This can return None - will raise AttributeError on next line. Line 5113: port = graphics.getAttribute('port') Line 5114: if port: Line 5115: self.conf['displayPort'] = port Line 5116: port = graphics.getAttribute('tlsPort')
Nir Soffer has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 13:
(1 comment)
http://gerrit.ovirt.org/#/c/17694/13/vdsm/vm.py File vdsm/vm.py:
Line 3105: self._getUnderlyingWatchdogDeviceInfo() Line 3106: self._getUnderlyingSmartcardDeviceInfo() Line 3107: self._getUnderlyingConsoleDeviceInfo() Line 3108: # Obtain info of all unknown devices. Must be last! Line 3109: self._getUnderlyingUnknownDeviceInfo() For another patch: get rid of the "Underlying" in these names. Line 3110: Line 3111: def _updateAgentChannels(self): Line 3112: """ Line 3113: We moved the naming of guest agent channel sockets. To keep backwards
Michal Skrivanek has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 13:
(2 comments)
http://gerrit.ovirt.org/#/c/17694/13/vdsm/vm.py File vdsm/vm.py:
Line 3105: self._getUnderlyingWatchdogDeviceInfo() Line 3106: self._getUnderlyingSmartcardDeviceInfo() Line 3107: self._getUnderlyingConsoleDeviceInfo() Line 3108: # Obtain info of all unknown devices. Must be last! Line 3109: self._getUnderlyingUnknownDeviceInfo()
For another patch: get rid of the "Underlying" in these names.
I tend to like the name. It gives you a clear distinction from vdsm's structures Line 3110: Line 3111: def _updateAgentChannels(self): Line 3112: """ Line 3113: We moved the naming of guest agent channel sockets. To keep backwards
Line 5120: def _getUnderlyingNetworkInterfaceInfo(self): Line 5121: """ Line 5122: Obtain network interface info from libvirt. Line 5123: """ Line 5124: # TODO use xpath instead of parseString (here and elsewhere) This is not valid here anymore, is it? Line 5125: for x in self._lastXMLDesc.getDeviceElements('interface'): Line 5126: devType = x.getAttribute('type') Line 5127: mac = x.getElementsByTagName('mac')[0].getAttribute('address') Line 5128: alias = x.getElementsByTagName('alias')[0].getAttribute('name')
Vinzenz Feenstra has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 13:
(8 comments)
http://gerrit.ovirt.org/#/c/17694/13/vdsm/vm.py File vdsm/vm.py:
Line 484: def getDeviceElements(self, tagName): Line 485: return self._devices.getElementsByTagName(tagName) Line 486: Line 487: @property Line 488: def hash(self):
This should be named devicesHash - this is not a hash of the xml. Can we u
correct, it has to be renamed Line 489: if self.devices and not self._hash: Line 490: self._hash = str(hash(self._devices.toxml())) Line 491: return self._hash or '0' Line 492:
Line 487: @property Line 488: def hash(self): Line 489: if self.devices and not self._hash: Line 490: self._hash = str(hash(self._devices.toxml())) Line 491: return self._hash or '0'
Why the or in return? Just compute the hash and save it.
You might not have devices, and this keeps the original behaviour that it is initially returning '0' Line 492: Line 493: Line 494: def eventToString(event): Line 495: return _EVENT_STRINGS[event]
Line 2033: if 'vmName' not in self.conf: Line 2034: self.conf['vmName'] = 'n%s' % self.id Line 2035: self._guestSocketFile = self._makeChannelPath(_VMCHANNEL_DEVICE_NAME) Line 2036: self._qemuguestSocketFile = self._makeChannelPath(_QEMU_GA_DEVICE_NAME) Line 2037: self._lastXMLDesc = ParsedDomXML(
For next patch - rename lastXMLDesc to something nicer?
Yeah, I hate it as well ;-) Line 2038: '<domain><uuid>%s</uuid></domain>' % self.id) Line 2039: self._released = False Line 2040: self._releaseLock = threading.Lock() Line 2041: self.saveState()
Line 3105: self._getUnderlyingWatchdogDeviceInfo() Line 3106: self._getUnderlyingSmartcardDeviceInfo() Line 3107: self._getUnderlyingConsoleDeviceInfo() Line 3108: # Obtain info of all unknown devices. Must be last! Line 3109: self._getUnderlyingUnknownDeviceInfo()
For another patch: get rid of the "Underlying" in these names.
In the long run we want to get rid of them inside of the VM class and refactor them out of there, then we'd get rid of this name. For now I tend to agree with Michal Line 3110: Line 3111: def _updateAgentChannels(self): Line 3112: """ Line 3113: We moved the naming of guest agent channel sockets. To keep backwards
Line 3292: Line 3293: for snappableDiskDeviceXmlElement in snappableDiskDeviceXmlElements: Line 3294: self._changeDisk(snappableDiskDeviceXmlElement) Line 3295: Line 3296: return parsedSrcDomXML.dom.toxml()
This usage is bad - we take a read-only object (ParsedDomXML), modify it's
True, I just tried to use the convenience functions of it. It's probably not really worth it Line 3297: Line 3298: def _changeDisk(self, diskDeviceXmlElement): Line 3299: diskType = diskDeviceXmlElement.getAttribute('type') Line 3300:
Line 4634: pass Line 4635: return pid Line 4636: Line 4637: def _getUnderlyingVmInfo(self): Line 4638: self._lastXMLDesc = ParsedDomXML(self._dom.XMLDesc(0))
This function used to return self._lastXMLDesc?!
The return value was never used anywhere, that's why I dropped it. It's not really obvious though.
I am not sure why I would add an explaining name for this and we use this everywhere else without it. XMLDesc is an API of libvirt. We don't really document that usage anywhere else either, the only part which is not quite clear to someone might be the 0 parameter. Line 4639: Line 4640: def _ejectFloppy(self): Line 4641: if 'volatileFloppy' in self.conf: Line 4642: utils.rmFile(self.conf['floppy'])
Line 5108: def _getUnderlyingDisplayPort(self): Line 5109: """ Line 5110: Obtain display port info from libvirt. Line 5111: """ Line 5112: graphics = self._lastXMLDesc.firstElementByTagName('graphics')
This can return None - will raise AttributeError on next line.
Well and before it was an out of index error in that case. Not sure how relevant that really is Line 5113: port = graphics.getAttribute('port') Line 5114: if port: Line 5115: self.conf['displayPort'] = port Line 5116: port = graphics.getAttribute('tlsPort')
Line 5120: def _getUnderlyingNetworkInterfaceInfo(self): Line 5121: """ Line 5122: Obtain network interface info from libvirt. Line 5123: """ Line 5124: # TODO use xpath instead of parseString (here and elsewhere)
This is not valid here anymore, is it?
Probably not Line 5125: for x in self._lastXMLDesc.getDeviceElements('interface'): Line 5126: devType = x.getAttribute('type') Line 5127: mac = x.getElementsByTagName('mac')[0].getAttribute('address') Line 5128: alias = x.getElementsByTagName('alias')[0].getAttribute('name')
Nir Soffer has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 13:
(1 comment)
http://gerrit.ovirt.org/#/c/17694/13/vdsm/vm.py File vdsm/vm.py:
Line 5108: def _getUnderlyingDisplayPort(self): Line 5109: """ Line 5110: Obtain display port info from libvirt. Line 5111: """ Line 5112: graphics = self._lastXMLDesc.firstElementByTagName('graphics')
Well and before it was an out of index error in that case. Not sure how rel
Right, so this can wait for another patch. Line 5113: port = graphics.getAttribute('port') Line 5114: if port: Line 5115: self.conf['displayPort'] = port Line 5116: port = graphics.getAttribute('tlsPort')
Nir Soffer has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 13:
(1 comment)
http://gerrit.ovirt.org/#/c/17694/13/vdsm/vm.py File vdsm/vm.py:
Line 4634: pass Line 4635: return pid Line 4636: Line 4637: def _getUnderlyingVmInfo(self): Line 4638: self._lastXMLDesc = ParsedDomXML(self._dom.XMLDesc(0))
The return value was never used anywhere, that's why I dropped it. It's not
Obviously libvirt API is not clear, and we would like to have more clear code, so it is easy to work with. Of course this should be handled in another patch. Line 4639: Line 4640: def _ejectFloppy(self): Line 4641: if 'volatileFloppy' in self.conf: Line 4642: utils.rmFile(self.conf['floppy'])
Francesco Romani has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 14:
Patch set 14: just published a patch of mine on top of this one :\
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 14:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/8108/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/8221/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/7318/ : SUCCESS
Nir Soffer has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 14: Code-Review-1
Please address my comments from patch set 13: http://gerrit.ovirt.org/#/c/17694/13/vdsm/vm.py
Itamar Heim has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 14:
ping
Vinzenz Feenstra has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 13:
(3 comments)
http://gerrit.ovirt.org/#/c/17694/13/vdsm/vm.py File vdsm/vm.py:
Line 484: def getDeviceElements(self, tagName): Line 485: return self._devices.getElementsByTagName(tagName) Line 486: Line 487: @property Line 488: def hash(self):
correct, it has to be renamed
Done Line 489: if self.devices and not self._hash: Line 490: self._hash = str(hash(self._devices.toxml())) Line 491: return self._hash or '0' Line 492:
Line 3292: Line 3293: for snappableDiskDeviceXmlElement in snappableDiskDeviceXmlElements: Line 3294: self._changeDisk(snappableDiskDeviceXmlElement) Line 3295: Line 3296: return parsedSrcDomXML.dom.toxml()
True, I just tried to use the convenience functions of it. It's probably no
Done Line 3297: Line 3298: def _changeDisk(self, diskDeviceXmlElement): Line 3299: diskType = diskDeviceXmlElement.getAttribute('type') Line 3300:
Line 4634: pass Line 4635: return pid Line 4636: Line 4637: def _getUnderlyingVmInfo(self): Line 4638: self._lastXMLDesc = ParsedDomXML(self._dom.XMLDesc(0))
This function used to return self._lastXMLDesc?!
Done Line 4639: Line 4640: def _ejectFloppy(self): Line 4641: if 'volatileFloppy' in self.conf: Line 4642: utils.rmFile(self.conf['floppy'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 15: Code-Review-1 Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9666/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10451/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1018/ : There was an infra issue, please contact infra@ovirt.org
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10608/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 16: Code-Review-1 Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9667/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10452/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1019/ : There was an infra issue, please contact infra@ovirt.org
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10609/ : FAILURE
Nir Soffer has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 18:
(2 comments)
Nice, needs little cleanup.
http://gerrit.ovirt.org/#/c/17694/18/vdsm/virt/vm.py File vdsm/virt/vm.py:
Line 1855: self.conf['vmName'] = 'n%s' % self.id Line 1856: self._guestSocketFile = self._makeChannelPath(_VMCHANNEL_DEVICE_NAME) Line 1857: self._qemuguestSocketFile = self._makeChannelPath(_QEMU_GA_DEVICE_NAME) Line 1858: self._lastXMLDesc = DomainDescriptor( Line 1859: '<domain><uuid>%s</uuid></domain>' % self.id) Lets move this hack into domaindescrioptor module and call it something like emptyDescriptor or something like that.
Will clean the code here:
self._lastXMLDesc = DomainDescriptor.fromId(self.id) Line 1860: self._released = False Line 1861: self._releaseLock = threading.Lock() Line 1862: self.saveState() Line 1863: self._watchdogEvent = {}
Line 4659: except Exception: Line 4660: pass Line 4661: return pid Line 4662: Line 4663: def _getUnderlyingVmInfo(self): This does not get anything.
Lets rename to setupUnderlyingVmInfo, or maye more specific like parseDomainXML Line 4664: domainXML = self._dom.XMLDesc(0) Line 4665: self._lastXMLDesc = DomainDescriptor(domainXML) Line 4666: Line 4667: def _ejectFloppy(self):
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 17: Code-Review-1 Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9668/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10453/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1020/ : There was an infra issue, please contact infra@ovirt.org
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10610/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 18:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9670/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10455/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1021/ : There was an infra issue, please contact infra@ovirt.org
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10612/ : SUCCESS
Francesco Romani has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 18:
(2 comments)
I'm fine with the domain_descriptor.py; but just a wild idea, maybe merge into the new xmldom module? http://gerrit.ovirt.org/#/c/26855/2
http://gerrit.ovirt.org/#/c/17694/18/vdsm/virt/vm.py File vdsm/virt/vm.py:
Line 1855: self.conf['vmName'] = 'n%s' % self.id Line 1856: self._guestSocketFile = self._makeChannelPath(_VMCHANNEL_DEVICE_NAME) Line 1857: self._qemuguestSocketFile = self._makeChannelPath(_QEMU_GA_DEVICE_NAME) Line 1858: self._lastXMLDesc = DomainDescriptor( Line 1859: '<domain><uuid>%s</uuid></domain>' % self.id)
Lets move this hack into domaindescrioptor module and call it something lik
+1 Line 1860: self._released = False Line 1861: self._releaseLock = threading.Lock() Line 1862: self.saveState() Line 1863: self._watchdogEvent = {}
Line 4661: return pid Line 4662: Line 4663: def _getUnderlyingVmInfo(self): Line 4664: domainXML = self._dom.XMLDesc(0) Line 4665: self._lastXMLDesc = DomainDescriptor(domainXML) Now we need the return value: see _lookupDeviceXMLByAlias Line 4666: Line 4667: def _ejectFloppy(self): Line 4668: if 'volatileFloppy' in self.conf: Line 4669: utils.rmFile(self.conf['floppy'])
Vinzenz Feenstra has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 18:
@return value No, not any more, I just updated it
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 19: Code-Review-1 Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9678/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10463/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1028/ : There was an infra issue, please contact infra@ovirt.org
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10620/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 20: Code-Review-1 Verified-1
Build Unstable
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9691/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10476/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1033/ : The patch does not pass the virt functional tests
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10633/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 21: Code-Review-1 Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9692/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10477/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1034/ : The patch does not pass the virt functional tests
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/24/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc19_created/57/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/34/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10634/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 22:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9693/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10478/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1035/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/25/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc19_created/58/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/35/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10635/ : SUCCESS
Nir Soffer has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 22:
(2 comments)
Nice but see comments in http://gerrit.ovirt.org/#/c/17694/22/vdsm/virt/domain_descriptor.py
http://gerrit.ovirt.org/#/c/17694/22/vdsm/virt/domain_descriptor.py File vdsm/virt/domain_descriptor.py:
Line 19: # Line 20: import xml.dom.minidom Line 21: Line 22: Line 23: class DomainDescriptor(object): Blank line bellow the class is common and make it even nicer. Line 24: def __init__(self, xmlStr): Line 25: self._xml = xmlStr Line 26: self._dom = xml.dom.minidom.parseString(xmlStr) Line 27: self._devices = self.firstElementByTagName('devices')
Line 29: self._devicesHash = str(hash(self._devices)) if self._devices else '0' Line 30: Line 31: @staticmethod Line 32: def fromId(uuid): Line 33: return DomainDescriptor('<domain><uuid>%s</uuid></domain>' % uuid) Should be class method:
@classmethod def fromId(cls, uuid): retrn cls("...")
So if you subclass this you will create your subclass, not the superclass. Line 34: Line 35: @property Line 36: def xml(self): Line 37: return self._xml
Vinzenz Feenstra has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 22:
(2 comments)
http://gerrit.ovirt.org/#/c/17694/22/vdsm/virt/domain_descriptor.py File vdsm/virt/domain_descriptor.py:
Line 19: # Line 20: import xml.dom.minidom Line 21: Line 22: Line 23: class DomainDescriptor(object):
Blank line bellow the class is common and make it even nicer.
Done Line 24: def __init__(self, xmlStr): Line 25: self._xml = xmlStr Line 26: self._dom = xml.dom.minidom.parseString(xmlStr) Line 27: self._devices = self.firstElementByTagName('devices')
Line 29: self._devicesHash = str(hash(self._devices)) if self._devices else '0' Line 30: Line 31: @staticmethod Line 32: def fromId(uuid): Line 33: return DomainDescriptor('<domain><uuid>%s</uuid></domain>' % uuid)
Should be class method:
Done Line 34: Line 35: @property Line 36: def xml(self): Line 37: return self._xml
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 22:
(1 comment)
http://gerrit.ovirt.org/#/c/17694/22/vdsm/virt/domain_descriptor.py File vdsm/virt/domain_descriptor.py:
Line 29: self._devicesHash = str(hash(self._devices)) if self._devices else '0' Line 30: Line 31: @staticmethod Line 32: def fromId(uuid): Line 33: return DomainDescriptor('<domain><uuid>%s</uuid></domain>' % uuid)
Should be class method:
@Nir are you sure you're not confusing this with @classmethod? https://docs.python.org/2/library/functions.html#staticmethod vs https://docs.python.org/2/library/functions.html#classmethod Line 34: Line 35: @property Line 36: def xml(self): Line 37: return self._xml
Nir Soffer has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 22:
(1 comment)
http://gerrit.ovirt.org/#/c/17694/22/vdsm/virt/domain_descriptor.py File vdsm/virt/domain_descriptor.py:
Line 29: self._devicesHash = str(hash(self._devices)) if self._devices else '0' Line 30: Line 31: @staticmethod Line 32: def fromId(uuid): Line 33: return DomainDescriptor('<domain><uuid>%s</uuid></domain>' % uuid)
@Nir are you sure you're not confusing this with @classmethod? https://docs
I do not understand your question - Vinzenz is using a @staticmethod and I suggested a @classmethod - where is the confusion? Line 34: Line 35: @property Line 36: def xml(self): Line 37: return self._xml
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 22:
(1 comment)
http://gerrit.ovirt.org/#/c/17694/22/vdsm/virt/domain_descriptor.py File vdsm/virt/domain_descriptor.py:
Line 29: self._devicesHash = str(hash(self._devices)) if self._devices else '0' Line 30: Line 31: @staticmethod Line 32: def fromId(uuid): Line 33: return DomainDescriptor('<domain><uuid>%s</uuid></domain>' % uuid)
I do not understand your question - Vinzenz is using a @staticmethod and I
I now see I caused the confusion by misreading it. My apologies. Line 34: Line 35: @property Line 36: def xml(self): Line 37: return self._xml
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 23:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9745/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10530/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1068/ : There was an infra issue, please contact infra@ovirt.org
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/32/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc19_created/65/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/44/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10687/ : SUCCESS
Nir Soffer has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 23: Code-Review+1
Nice
Francesco Romani has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 23: Code-Review+1
let's have this in! It owuld be awesome to have also some numbers about the performance gains of this patch
Francesco Romani has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 24:
rebased, due to vmxml.py changes introduced since the revision before.
Francesco Romani has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 24:
(2 comments)
some possible improvements I'd like to implement, probably better in follow up patches once this is in.
http://gerrit.ovirt.org/#/c/17694/24/vdsm/virt/domain_descriptor.py File vdsm/virt/domain_descriptor.py:
Line 40: @property Line 41: def dom(self): Line 42: return self._dom Line 43: Line 44: def firstElementByTagName(self, tagName): according to git grep, this can be made private Line 45: elements = self._dom.childNodes[0].getElementsByTagName(tagName) Line 46: return elements[0] if elements else None Line 47: Line 48: @property
Line 49: def devices(self): Line 50: return self._devices Line 51: Line 52: def getDeviceElements(self, tagName): Line 53: return self._devices.getElementsByTagName(tagName) Since we need another round of review due to the big rebase being made, I'd like to propose a new API here. My rationale is 1 since this is new module, have an API more_pep8_friendly and goAwayFromCamelCase 2 add more convenience methods, possibly merging helpers currently in vmxml.py 3 abstract from the minidom API, in not-so-distant future we want to move to cElementTree for performance and nicer API.
What about replacing the two above with
@property def all_devices(self): return self._devices
def devices(self, type): # klass? dev_type? kind? return self._devices.getElementsByTagName(type)
- of course this will not address #3 above Line 54: Line 55: @property Line 56: def devicesHash(self):
Francesco Romani has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 25:
fixes tests broken by rebase. Will do any other change but rebase fixes/address reviewer's comments in followup patches. let's unfreeze this change and have it in!
Nir Soffer has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 25:
(2 comments)
Partial review
http://gerrit.ovirt.org/#/c/17694/25/vdsm/virt/vm.py File vdsm/virt/vm.py:
Line 4489: return pid Line 4490: Line 4491: def _updateDomainDescriptor(self): Line 4492: domainXML = self._dom.XMLDesc(0) Line 4493: self._lastXMLDesc = DomainDescriptor(domainXML) Lets clean up the ugly _lastXMLDesc - how about self._domain?
For example:
for device in self._domain.devices: ...
Or:
self._domain.xml Line 4494: Line 4495: def _ejectFloppy(self): Line 4496: if 'volatileFloppy' in self.conf: Line 4497: utils.rmFile(self.conf['floppy'])
Line 5045: def _getUnderlyingNetworkInterfaceInfo(self): Line 5046: """ Line 5047: Obtain network interface info from libvirt. Line 5048: """ Line 5049: # TODO use xpath instead of parseString (here and elsewhere) This comment can be remove now. Line 5050: for x in self._lastXMLDesc.getDeviceElements('interface'): Line 5051: devType = x.getAttribute('type') Line 5052: mac = x.getElementsByTagName('mac')[0].getAttribute('address') Line 5053: alias = x.getElementsByTagName('alias')[0].getAttribute('name')
Nir Soffer has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 25:
(1 comment)
http://gerrit.ovirt.org/#/c/17694/25/vdsm/virt/vm.py File vdsm/virt/vm.py:
Line 4489: return pid Line 4490: Line 4491: def _updateDomainDescriptor(self): Line 4492: domainXML = self._dom.XMLDesc(0) Line 4493: self._lastXMLDesc = DomainDescriptor(domainXML)
Lets clean up the ugly _lastXMLDesc - how about self._domain?
We can also do it in another patch - but this patch changes every line that touch self._lastXMLDesc since we change the type. Changing the name now will save additional verification and will save reviewers time. Line 4494: Line 4495: def _ejectFloppy(self): Line 4496: if 'volatileFloppy' in self.conf: Line 4497: utils.rmFile(self.conf['floppy'])
Francesco Romani has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 25:
(1 comment)
http://gerrit.ovirt.org/#/c/17694/25/vdsm/virt/vm.py File vdsm/virt/vm.py:
Line 4489: return pid Line 4490: Line 4491: def _updateDomainDescriptor(self): Line 4492: domainXML = self._dom.XMLDesc(0) Line 4493: self._lastXMLDesc = DomainDescriptor(domainXML)
We can also do it in another patch - but this patch changes every line that
'domain' is nicer and shorter, and as you said saves developer time. Will change.
Should I also squash http://gerrit.ovirt.org/#/c/33672/1 in this? Only name are changed, non changes in functioning. Line 4494: Line 4495: def _ejectFloppy(self): Line 4496: if 'volatileFloppy' in self.conf: Line 4497: utils.rmFile(self.conf['floppy'])
Nir Soffer has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 25:
(1 comment)
http://gerrit.ovirt.org/#/c/17694/25/vdsm/virt/vm.py File vdsm/virt/vm.py:
Line 4489: return pid Line 4490: Line 4491: def _updateDomainDescriptor(self): Line 4492: domainXML = self._dom.XMLDesc(0) Line 4493: self._lastXMLDesc = DomainDescriptor(domainXML)
'domain' is nicer and shorter, and as you said saves developer time. Will c
336721 is not related to this change, please don't squash it. Line 4494: Line 4495: def _ejectFloppy(self): Line 4496: if 'volatileFloppy' in self.conf: Line 4497: utils.rmFile(self.conf['floppy'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 24:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/414... : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/431/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11766/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12710/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1709/ : There was an infra issue, please contact infra@ovirt.org
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12555/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 25:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/416... : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/433/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11769/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12713/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1710/ : There was an infra issue, please contact infra@ovirt.org
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12558/ : SUCCESS
Dan Kenigsberg has posted comments on this change.
Change subject: Introduction for caching the parsed domain XML ......................................................................
Patch Set 25:
(1 comment)
http://gerrit.ovirt.org/#/c/17694/25/vdsm/virt/domain_descriptor.py File vdsm/virt/domain_descriptor.py:
Line 40: @property Line 41: def dom(self): Line 42: return self._dom Line 43: Line 44: def firstElementByTagName(self, tagName): this should be private (unless expected to be used out of __init__ Line 45: elements = self._dom.childNodes[0].getElementsByTagName(tagName) Line 46: return elements[0] if elements else None Line 47: Line 48: @property
oVirt Jenkins CI Server has posted comments on this change.
Change subject: virt: introduce caching the parsed domain XML ......................................................................
Patch Set 26:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12811/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11863/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1746/ : There was an infra issue, please contact infra@ovirt.org
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/451/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/434... : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12654/ : SUCCESS
Nir Soffer has posted comments on this change.
Change subject: virt: introduce caching the parsed domain XML ......................................................................
Patch Set 26:
(2 comments)
http://gerrit.ovirt.org/#/c/17694/26/vdsm/virt/domain_descriptor.py File vdsm/virt/domain_descriptor.py:
Line 40: @property Line 41: def dom(self): Line 42: return self._dom Line 43: Line 44: def firstElementByTagName(self, tagName): This should be private as Dan suggested in the previous version. Line 45: elements = self._dom.childNodes[0].getElementsByTagName(tagName) Line 46: return elements[0] if elements else None Line 47: Line 48: @property
http://gerrit.ovirt.org/#/c/17694/26/vdsm/virt/vm.py File vdsm/virt/vm.py:
Line 5045: def _getUnderlyingNetworkInterfaceInfo(self): Line 5046: """ Line 5047: Obtain network interface info from libvirt. Line 5048: """ Line 5049: # TODO use xpath instead of parseString (here and elsewhere) This comment is unneeded now. Line 5050: for x in self._domain.getDeviceElements('interface'): Line 5051: devType = x.getAttribute('type') Line 5052: mac = x.getElementsByTagName('mac')[0].getAttribute('address') Line 5053: alias = x.getElementsByTagName('alias')[0].getAttribute('name')
Francesco Romani has posted comments on this change.
Change subject: virt: introduce caching the parsed domain XML ......................................................................
Patch Set 26:
(2 comments)
http://gerrit.ovirt.org/#/c/17694/26/vdsm/virt/domain_descriptor.py File vdsm/virt/domain_descriptor.py:
Line 40: @property Line 41: def dom(self): Line 42: return self._dom Line 43: Line 44: def firstElementByTagName(self, tagName):
This should be private as Dan suggested in the previous version.
Done Line 45: elements = self._dom.childNodes[0].getElementsByTagName(tagName) Line 46: return elements[0] if elements else None Line 47: Line 48: @property
http://gerrit.ovirt.org/#/c/17694/26/vdsm/virt/vm.py File vdsm/virt/vm.py:
Line 5045: def _getUnderlyingNetworkInterfaceInfo(self): Line 5046: """ Line 5047: Obtain network interface info from libvirt. Line 5048: """ Line 5049: # TODO use xpath instead of parseString (here and elsewhere)
This comment is unneeded now.
Done Line 5050: for x in self._domain.getDeviceElements('interface'): Line 5051: devType = x.getAttribute('type') Line 5052: mac = x.getElementsByTagName('mac')[0].getAttribute('address') Line 5053: alias = x.getElementsByTagName('alias')[0].getAttribute('name')
oVirt Jenkins CI Server has posted comments on this change.
Change subject: virt: introduce caching the parsed domain XML ......................................................................
Patch Set 27:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12836/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11888/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1749/ : There was an infra issue, please contact infra@ovirt.org
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/457/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/440... : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12679/ : SUCCESS
Francesco Romani has posted comments on this change.
Change subject: virt: introduce caching the parsed domain XML ......................................................................
Patch Set 27: Verified+1
I tested all the four vm creation flows without noticing any issue. Verified migration to/from an unpatched VDSM.
I'm up for additional verification, just drop me a note with a different scenario.
Francesco Romani has posted comments on this change.
Change subject: virt: introduce caching the parsed domain XML ......................................................................
Patch Set 27:
rebased because of local conflicts (cherry-pick didn't applied cleanly).
Dan Kenigsberg has posted comments on this change.
Change subject: virt: introduce caching the parsed domain XML ......................................................................
Patch Set 27: Code-Review+2
Nir's comments and mine are answered. Thanks.
Dan Kenigsberg has submitted this change and it was merged.
Change subject: virt: introduce caching the parsed domain XML ......................................................................
virt: introduce caching the parsed domain XML
This patch replaces the repeatively parsed domain XML with a pre-parsed and cached implementation. Additionally it comes with convenience functions for accessing the devices sub-tree
Change-Id: I7e106b2f2d3f4160d4e882f1a2880cb1b52fbb22 Signed-off-by: Vinzenz Feenstra vfeenstr@redhat.com Reviewed-on: http://gerrit.ovirt.org/17694 Tested-by: Francesco Romani fromani@redhat.com Reviewed-by: Dan Kenigsberg danken@redhat.com --- M debian/vdsm.install M tests/vmTests.py M vdsm.spec.in M vdsm/virt/Makefile.am A vdsm/virt/domain_descriptor.py M vdsm/virt/vm.py 6 files changed, 90 insertions(+), 64 deletions(-)
Approvals: Dan Kenigsberg: Looks good to me, approved Francesco Romani: Verified
oVirt Jenkins CI Server has posted comments on this change.
Change subject: virt: introduce caching the parsed domain XML ......................................................................
Patch Set 28:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/4114/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc21-x86_64_merged/104/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el6-x86_64_merged/129/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged_test_debug/322/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5954/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el7-x86_64_merged/126/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc20-x86_64_merged/122/ : SUCCESS
vdsm-patches@lists.fedorahosted.org
-
asegurap＠redhat.com -
Dan Kenigsberg -
ewoud＠kohlvanwijngaarden.nl -
fromani＠redhat.com -
iheim＠redhat.com -
Jenkins CI RO -
Martin Sivak -
michal.skrivanek＠redhat.com -
Nir Soffer -
peet＠redhat.com -
Vinzenz Feenstra