Change in vdsm[master]: Live Merge: Restore watermark tracking

nsoffer at redhat.com nsoffer at redhat.com
Thu Jan 29 17:11:11 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: Live Merge: Restore watermark tracking
......................................................................


Patch Set 3:

(5 comments)

Added few more comments, I still need more time to finish the review.

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 after it was set?
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
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?

I think we should get bulkstats for all vms in the sampling thread, and use cached data here.

Today perform virDomainBlockInfo for each disk on each vm every 2 seconds (and some other calls every 15 and 60 seconds). We can replace this with one call to get bulk stats every 2 seconds, and use cached values everywhere else.
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.

Can you explain which values we get from libvirt and why we need to normalize them accessing the file system?

Why not use os.stat instead of the complex and expensive dance that is os.path.realpath()?

    it = os.stat(path)

    for vol in drive.volumeChain:
        st = os.stat(vol["path"])
        if st.st_ino, st.st_dev == it.st_ino, it.st_dev:
            found it...

Last, having inline function make it harder to read the code and increase the chance for duplicate code doing the same thing. I think we should avoid these unless we must (e.g. decorators).
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 why this happens.
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 at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list