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 <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
