Francesco Romani has posted comments on this change. Change subject: Live Merge: Extend internal block volumes during merge ......................................................................
Patch Set 2: (4 comments) preliminary ACK. Seems OK, but _driveGetActualVolumeChain is also starting to look scary :\ http://gerrit.ovirt.org/#/c/31268/2/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2065: def _getLiveMergeExtendCandidates(self): Line 2066: ret = {} Line 2067: Line 2068: # The common case is that there are no active jobs. Line 2069: if len(self.conf['_blockJobs'].values()) == 0: any reason to not simply use if not self.conf['_blockJobs'].values(): ? Line 2070: return ret Line 2071: Line 2072: drives = [d for d in self.getDiskDevices() Line 2073: if isVdsmImage(d) and bool(d.blockDev)] Line 2069: if len(self.conf['_blockJobs'].values()) == 0: Line 2070: return ret Line 2071: Line 2072: drives = [d for d in self.getDiskDevices() Line 2073: if isVdsmImage(d) and bool(d.blockDev)] why explicit bool() ? I'm concerned about bugs being hidden by this. Line 2074: allChains = self._driveGetActualVolumeChain(drives).values() Line 2075: watermarks = {} Line 2076: for chain in allChains: Line 2077: for vol in chain: Line 2074: allChains = self._driveGetActualVolumeChain(drives).values() Line 2075: watermarks = {} Line 2076: for chain in allChains: Line 2077: for vol in chain: Line 2078: watermarks[vol['id']] = vol['allocation'] what about watermarks = dict((vol['id'], vol['allocation']) for chain in allChains for vol in chain) not sure it is cleaner (or actually shorter), though, so feel free to ignore. Line 2079: Line 2080: for job in self.conf['_blockJobs'].values(): Line 2081: try: Line 2082: drive = self._findDriveByUUIDs(job['disk']) Line 5488: break Line 5489: sourceAttr = ('file', 'dev')[drive.blockDev] Line 5490: path = sourceXML.getAttribute(sourceAttr) Line 5491: info = {'id': pathToVolID(drive, path), 'path': path} Line 5492: if bool(drive.blockDev): same as per line 2073 Line 5493: allocXML = findElement(diskXML, 'allocation') Line 5494: if not allocXML: Line 5495: self.log.debug("<allocation/> missing from backing " Line 5496: "chain for block device %s", drive.name) -- To view, visit http://gerrit.ovirt.org/31268 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3a9e0ebdb9c42df713c40e0fc5782945eb7228a8 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: 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
