Adam Litke has posted comments on this change.
Change subject: Live Merge: Get volume chain for multiple drives
......................................................................
Patch Set 8: -Verified
(8 comments)
http://gerrit.ovirt.org/#/c/31366/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 5514: return child
Line 5515: return None
Line 5516:
Line 5517: ret = {}
Line 5518: domXML = self._getUnderlyingVmInfo()
A whitespace would help readability.
Refactored.
Line 5519: for drive in drives:
Line 5520: alias = drive['alias']
Line 5521: diskXML = lookupDeviceXMLByAlias(domXML, alias)
Line 5522: if not diskXML:
Line 5516:
Line 5517: ret = {}
Line 5518: domXML = self._getUnderlyingVmInfo()
Line 5519: for drive in drives:
Line 5520: alias = drive['alias']
(Whitespace possibily)
Refactored.
Line 5521: diskXML = lookupDeviceXMLByAlias(domXML, alias)
Line 5522: if not diskXML:
Line 5523: continue
Line 5524: volChain = []
Line 5519: for drive in drives:
Line 5520: alias = drive['alias']
Line 5521: diskXML = lookupDeviceXMLByAlias(domXML, alias)
Line 5522: if not diskXML:
Line 5523: continue
What's this case? Is it problematic? Is it worth logging?
There is no valid case. If something has gone terribly wrong where our metadata is
completely wrong, then this could be None. I'll change this so lookupDeviceXMLByAlias
raises an exception and will then drop this check.
Line 5524: volChain = []
Line 5525: while True:
Line 5526: sourceXML = findElement(diskXML, 'source')
Line 5527: if not sourceXML:
Line 5521: diskXML = lookupDeviceXMLByAlias(domXML, alias)
Line 5522: if not diskXML:
Line 5523: continue
Line 5524: volChain = []
Line 5525: while True:
How hard would it be to split this while loop in an outside function
method
I've broken it out for the next submission. I'm not convinced it
looks any better but I'll leave that for others to judge.
Line 5526: sourceXML = findElement(diskXML, 'source')
Line 5527: if not sourceXML:
Line 5528: break
Line 5529: sourceAttr = ('file', 'dev')[drive.blockDev]
Line 5531: entry = VolumeChainEntry(pathToVolID(drive, path), path)
Line 5532: bsXML = findElement(diskXML, 'backingStore')
Line 5533: if not bsXML:
Line 5534: self.log.debug("<backingStore/> missing from
backing "
Line 5535: "chain for block device %s",
drive.name)
Is this always a block device?
Nope, changed message.
Line 5536: break
Line 5537: diskXML = bsXML
Line 5538: volChain.insert(0, entry)
Line 5539: if volChain:
Line 5532: bsXML = findElement(diskXML, 'backingStore')
Line 5533: if not bsXML:
Line 5534: self.log.debug("<backingStore/> missing from
backing "
Line 5535: "chain for block device %s",
drive.name)
Line 5536: break
Could this return a partial volChain? shouldn't we raise an
exception?
We use this code to probe if volumeChain into is reported by libvirt so I
don't think an exception is warranted. I might bump up the log level to warning
though. Basically, we expect all elements of the chain to have <backingStore/> or
none of them. If libvirt only supplies it for some, then it's a libvirt bug.
Line 5537: diskXML = bsXML
Line 5538: volChain.insert(0, entry)
Line 5539: if volChain:
Line 5540: ret[alias] = volChain
Line 5534: self.log.debug("<backingStore/> missing from
backing "
Line 5535: "chain for block device %s",
drive.name)
Line 5536: break
Line 5537: diskXML = bsXML
Line 5538: volChain.insert(0, entry)
(Whitespace possibily)
Refactored.
Line 5539: if volChain:
Line 5540: ret[alias] = volChain
Line 5541: return ret
Line 5542:
Line 5536: break
Line 5537: diskXML = bsXML
Line 5538: volChain.insert(0, entry)
Line 5539: if volChain:
Line 5540: ret[alias] = volChain
(Whitespace possibily)
Refactored.
Line 5541: return ret
Line 5542:
Line 5543: def _syncVolumeChain(self, drive):
Line 5544: def getVolumeInfo(device, volumeID):
--
To view, visit
http://gerrit.ovirt.org/31366
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I93d5a641b0814e3764c70bea8a6c1910a821adcc
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes