Francesco Romani has posted comments on this change.
Change subject: Live Merge: Restore watermark tracking
......................................................................
Patch Set 1: Code-Review-1
(4 comments)
A few initial remarks. Most important: we need some form of dependency from new libvirt,
either hard or soft, don't we?
http://gerrit.ovirt.org/#/c/36924/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 1520: raise LookupError("Unable to find VolumeID for path
'%s'", path)
Line 1521:
Line 1522: volAllocMap = {}
Line 1523: statsFlags = self._libvirtBackingChainStatsFlag()
Line 1524: conn = libvirtconnection.get()
unless you need a specific different connection (then please explain why), just use
self._connection
Line 1525: blkStats = conn.domainListGetStats([self._dom._dom],
Line 1526: libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1527: statsFlags)[0][1]
Line 1528: for i in xrange(0, blkStats['block.count']):
Line 1523: statsFlags = self._libvirtBackingChainStatsFlag()
Line 1524: conn = libvirtconnection.get()
Line 1525: blkStats = conn.domainListGetStats([self._dom._dom],
Line 1526: libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1527: statsFlags)[0][1]
I prefer to avoid the double indexing.
Why we need [0] is sufficiently clear, but I prefer
_, blkStats = conn...
only for clarity
Line 1528: for i in xrange(0, blkStats['block.count']):
Line 1529: name = blkStats['block.%i.name' % i]
Line 1530: try:
Line 1531: drive = self._findDriveByName(name)
Line 1524: conn = libvirtconnection.get()
Line 1525: blkStats = conn.domainListGetStats([self._dom._dom],
Line 1526: libvirt.VIR_DOMAIN_STATS_BLOCK,
Line 1527: statsFlags)[0][1]
Line 1528: for i in xrange(0, blkStats['block.count']):
why the explicit start? Isn't xrange(blkStats['block.count']) enough?
Line 1529: name = blkStats['block.%i.name' % i]
Line 1530: try:
Line 1531: drive = self._findDriveByName(name)
Line 1532: except LookupError:
Line 4858: # block statistics for all volumes in the chain when using a new flag.
Line 4859: try:
Line 4860: return libvirt.VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING
Line 4861: except AttributeError:
Line 4862: return 0
This could be detected once per VDSM run, no need to check it for every operation.
Probably better as module level private constant.
Line 4863:
Line 4864: def merge(self, driveSpec, baseVolUUID, topVolUUID, bandwidth, jobUUID):
Line 4865: if not caps.getLiveMergeSupport():
Line 4866: self.log.error("Live merge is not supported on this
host")
--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: 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