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 <alitke(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(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