Adam Litke has posted comments on this change.
Change subject: Live Merge: Restore watermark tracking
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.ovirt.org/#/c/36924/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 169:
Line 170:
Line 171: # Since libvirt 1.2.13, the virConnectGetAllDomainStats API will return
Line 172: # block statistics for all volumes in the chain when using a new flag.
Line 173: _libvirtBackingChainStatsFlag = getattr(
Why do you a constant which looks like a variable? Can this value
change af
No it cannot. I'll capitalize it.
Line 174: libvirt, "VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING", 0)
Line 175:
Line 176:
Line 177: class VmStatsThread(AdvancedStatsThread):
Line 712: 'writeLatency': str(compute_latency('wr')),
Line 713: 'flushLatency': str(compute_latency('flush'))}
Line 714:
Line 715:
Line 716: def _vmsGetBulkStats(conn, vmList, statsTypes=0, statsFlags=0):
I think _getAllVMsBulkStats would be more clear
ok.
Line 717: domList = [x._dom._dom for x in vmList]
Line 718: if statsTypes == 0:
Line 719: statsTypes = (libvirt.VIR_DOMAIN_STATS_STATE |
Line 720: libvirt.VIR_DOMAIN_STATS_CPU_TOTAL |
Line 1419: self.conf['timeOffset'] = newTimeOffset
Line 1420:
Line 1421: def _getBulkStats(self, statsTypes, statsFlags):
Line 1422: return _vmsGetBulkStats(self._connection, [self], statsTypes,
Line 1423: statsFlags)[self.id]
So we must get stats for all vms for getting single vm stats?
Yes this is a good long-term idea but I am not sure I want to commit to it here.
Getting all of the stats for all VMs could be expensive inside libvirt since it involves
multiple qemu monitor calls to each VM. A 2 second sampling interval may not be
appropriate for all of these things.
Further, we cannot replace the current call to dom.getBlockInfo (for the active layer
monitoring because the bulk stats API only provides values for capacity and allocation.
We need also the physical value which would then require calls down to the storage
getVolumeInfo which won't end up making this any more efficient.
Line 1424:
Line 1425: def _getWriteWatermarks(self):
Line 1426: def pathToVolID(drive, path):
Line 1427: for vol in drive.volumeChain:
Line 1424:
Line 1425: def _getWriteWatermarks(self):
Line 1426: def pathToVolID(drive, path):
Line 1427: for vol in drive.volumeChain:
Line 1428: if os.path.realpath(vol['path']) ==
os.path.realpath(path):
Finding the real path should be done once.
Sure I can add a
comment. Thanks for your suggestion on how to look it up using os.stat. I'll adopt
that and move the logic info a global helper function.
Line 1429: return vol['volumeID']
Line 1430: raise LookupError("Unable to find VolumeID for path
'%s'", path)
Line 1431:
Line 1432: volAllocMap = {}
Line 1436: name = bulkStats['block.%i.name' % i]
Line 1437: try:
Line 1438: drive = self._findDriveByName(name)
Line 1439: except LookupError:
Line 1440: continue
Is this expected? I think we like at least a debug log here to
understand w
No, not expected. I'll add a log.error message.
Line 1441: if not drive.blockDev or drive.format != 'cow':
Line 1442: continue
Line 1443:
Line 1444: try:
--
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: 3
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