Nir Soffer has posted comments on this change. Change subject: Live Merge: Restore watermark tracking ......................................................................
Patch Set 4: (13 comments) http://gerrit.ovirt.org/#/c/36924/4/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 722: 'writeLatency': str(compute_latency('wr')), Line 723: 'flushLatency': str(compute_latency('flush'))} Line 724: Line 725: Line 726: def _getAllVMsBulkStats(conn, vmList, statsTypes=0, statsFlags=0): This returns stats for *some* vms, not all. Sorry for asking for the bad name :-) Line 727: domList = [x._dom._dom for x in vmList] Line 728: if statsTypes == 0: Line 729: statsTypes = (libvirt.VIR_DOMAIN_STATS_STATE | Line 730: libvirt.VIR_DOMAIN_STATS_CPU_TOTAL | Line 723: 'flushLatency': str(compute_latency('flush'))} Line 724: Line 725: Line 726: def _getAllVMsBulkStats(conn, vmList, statsTypes=0, statsFlags=0): Line 727: domList = [x._dom._dom for x in vmList] "x" would be more clear as "vm". Line 728: if statsTypes == 0: Line 729: statsTypes = (libvirt.VIR_DOMAIN_STATS_STATE | Line 730: libvirt.VIR_DOMAIN_STATS_CPU_TOTAL | Line 731: libvirt.VIR_DOMAIN_STATS_BALLOON | Line 732: libvirt.VIR_DOMAIN_STATS_VCPU | Line 733: libvirt.VIR_DOMAIN_STATS_INTERFACE | Line 734: libvirt.VIR_DOMAIN_STATS_BLOCK) Line 735: statsList = conn.domainListGetStats(domList, statsTypes, statsFlags) Line 736: return dict([(x[0].UUIDString(), x[1]) for x in statsList]) The x variable is not very helpful, specially when you don't know what are x[0] and x[1]. Better unpack the tuple/list. And we can avoid the temporary list using generator expression, which is also little more clean: return dict((dom.UUIDString(), domStats) for dom, domStats in statsList) Line 737: Line 738: Line 739: class TimeoutError(libvirt.libvirtError): Line 740: pass Line 1429: self.conf['timeOffset'] = newTimeOffset Line 1430: Line 1431: def _getBulkStats(self, statsTypes, statsFlags): Line 1432: return _getAllVMsBulkStats(self._connection, [self], statsTypes, Line 1433: statsFlags)[self.id] This is little too hard to read as one line. It would be nicer as: vmsStats = _getVMsBulkStats(self._connection, [self], statsTypes, statsFlags) return vmsStats[self.id] Line 1434: Line 1435: def _getWriteWatermarks(self): Line 1436: volAllocMap = {} Line 1437: bulkStats = self._getBulkStats(libvirt.VIR_DOMAIN_STATS_BLOCK, Line 1431: def _getBulkStats(self, statsTypes, statsFlags): Line 1432: return _getAllVMsBulkStats(self._connection, [self], statsTypes, Line 1433: statsFlags)[self.id] Line 1434: Line 1435: def _getWriteWatermarks(self): Should be documented - what stats do we get, for which volumes. Line 1436: volAllocMap = {} Line 1437: bulkStats = self._getBulkStats(libvirt.VIR_DOMAIN_STATS_BLOCK, Line 1438: _LIBVIRT_BACKING_CHAIN_STATS_FLAG) Line 1439: for i in xrange(bulkStats['block.count']): Line 1432: return _getAllVMsBulkStats(self._connection, [self], statsTypes, Line 1433: statsFlags)[self.id] Line 1434: Line 1435: def _getWriteWatermarks(self): Line 1436: volAllocMap = {} Since this returns watermarks, it would be little bit nicer if you call this map "watermarks" instead of volAllocMap. This is also more consistent with other code such as _getLiveMergeExtendCandidates. Line 1437: bulkStats = self._getBulkStats(libvirt.VIR_DOMAIN_STATS_BLOCK, Line 1438: _LIBVIRT_BACKING_CHAIN_STATS_FLAG) Line 1439: for i in xrange(bulkStats['block.count']): Line 1440: name = bulkStats['block.%i.name' % i] Line 1436: volAllocMap = {} Line 1437: bulkStats = self._getBulkStats(libvirt.VIR_DOMAIN_STATS_BLOCK, Line 1438: _LIBVIRT_BACKING_CHAIN_STATS_FLAG) Line 1439: for i in xrange(bulkStats['block.count']): Line 1440: name = bulkStats['block.%i.name' % i] What is this expected key is missing? Line 1441: try: Line 1442: drive = self._findDriveByName(name) Line 1443: except LookupError: Line 1444: self.log.error("Unable to find drive '%s'", name) Line 1450: path = bulkStats['block.%i.path' % i] Line 1451: alloc = bulkStats['block.%i.allocation' % i] Line 1452: except KeyError as e: Line 1453: self.log.debug("Block stats are missing expected key '%s', " Line 1454: "skipping volume", e.args[0]) Adding volume name would be helpful in this error. Is this expected error? (e.g. always missing in some configuration/condition) If not, this should be an error log. Line 1455: continue Line 1456: volID = _pathToVolumeID(drive, path) Line 1457: volAllocMap[volID] = alloc Line 1458: return volAllocMap Line 1470: except LookupError: Line 1471: # After an active layer merge completes the vdsm metadata will Line 1472: # be out of sync for a brief period. If we cannot find the old Line 1473: # disk then it's safe to skip it. Line 1474: continue Adding log.debug for missing disks can be useful if the brief period turns out longer then we expect. Line 1475: Line 1476: if not drive.blockDev: Line 1477: continue Line 1478: Line 1493: Line 1494: if volInfo['format'].lower() != 'cow': Line 1495: continue Line 1496: Line 1497: if volumeID in watermarks: If the volumeID is not in watermarks, we don't have to get the volume info. Lets check this just after we get the volume id. Line 1498: self.log.debug("Adding live merge extension candidate: " Line 1499: "volume=%s allocation=%i", volumeID, Line 1500: watermarks[volumeID]) Line 1501: candidates[drive.imageID] = { Line 1504: 'capacity': int(volInfo['apparentsize']), Line 1505: 'volumeID': volumeID} Line 1506: else: Line 1507: self.log.warning("No watermark info available for %s", Line 1508: volumeID) Keeping the early exit style used before would be nicer, as we are not dealing with two possible cases, disk with watermarks and disk without watermarks: if volumeID not in watermarks: log warning and continue... add candidate... Line 1509: return candidates Line 1510: Line 1511: def _getExtendCandidates(self): Line 1512: ret = [] Line 4876: # check happens. If libvirt is too old to support this, extend the Line 4877: # internal volume now to accomodate the worst case scenario: no Line 4878: # intersection between the allocated blocks in the base volume and the Line 4879: # top volume. Line 4880: if drive.blockDev and baseCow: Would be nicer to separate this check from the rest. The big comment above is not related to this check. Or move the body and the log comment to a helper method - this method is way too long anyway. Line 4881: if _LIBVIRT_BACKING_CHAIN_STATS_FLAG: Line 4882: self.extendDrivesIfNeeded() Line 4883: else: Line 4884: extendSize = baseSize + topSize Line 4918: self.log.warning("<backingStore/> missing from backing " Line 4919: "chain for drive %s", drive.name) Line 4920: break Line 4921: diskXML = bsXML Line 4922: entry = VolumeChainEntry(pathToVolID(drive, path), path) Can we separate the removal of the "alloc" property to another patch? Line 4923: volChain.insert(0, entry) Line 4924: return volChain or None Line 4925: Line 4926: def _driveGetActualVolumeChain(self, drives): -- To view, visit http://gerrit.ovirt.org/36924 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I632f31e7795ec5d8c6f52a480116b14470c3163f Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
