Adam Litke has posted comments on this change.
Change subject: Live Merge: Restore watermark tracking
......................................................................
Patch Set 6:
(9 comments)
Nice!
https://gerrit.ovirt.org/#/c/60889/6/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 198: for vol in drive.volumeChain:
Line 199: if vol["path"] == path:
Line 200: return vol["volumeID"]
Line 201:
Line 202: raise LookupError("Unable to find a volume matching path:%s"
Need a space after %s
Line 203: "in drive:%s",
Line 204: path, drive.name)
Line 205:
Line 206:
Line 926: self.conf['timeOffset'] = newTimeOffset
Line 927:
Line 928: def _getExtendCandidates(self):
Line 929: ret = []
Line 930: mergeCandidates = self._getLiveMergeExtendCandidates()
If _getLiveMergeExtendCandidates() returns a list of tuple(drive, vol_id, capacity, alloc,
physical) values then you can simplify this code:
ret = self._getLiveMergeExtendCandidates()
for drive in self._chunckedDrives():
... append this drive's info to ret ...
return ret
Line 931:
Line 932: for drive in self._chunkedDrives():
Line 933: try:
Line 934: capacity, alloc, physical = self._getExtendInfo(drive)
Line 950: mergeCandidate['physical']))
Line 951: return ret
Line 952:
Line 953: def _getLiveMergeExtendCandidates(self):
Line 954: candidates = {}
Return a list of tuples.
Line 955: for job in self.conf['_blockJobs'].values():
Line 956: try:
Line 957: drive = self._findDriveByUUIDs(job['disk'])
Line 958: except LookupError:
Line 979: try:
Line 980: volInfo = self._getVolumeInfo(drive.domainID, drive.poolID,
Line 981: drive.imageID, volUUID)
Line 982: except StorageUnavailableError:
Line 983: continue
We should still log a warning here since this may cause us to not issue an extend request
and the vm could be later paused.
Line 984:
Line 985: if volInfo['format'].lower() != 'cow':
Line 986: continue
Line 987:
Line 993: candidates[drive.imageID] = {
Line 994: 'alloc': watermarks[volumeID],
Line 995: 'physical': int(volInfo['truesize']),
Line 996: 'capacity': int(volInfo['apparentsize']),
Line 997: 'volumeID': volUUID}
Just make a tuple and append it to candidates list.
Line 998: else:
Line 999: self.log.warning("No watermark info available for
%s",
Line 1000: volUUID)
Line 1001:
Line 1005: vmSample = sampling.stats_cache.get(self.id)
Line 1006: lastSample = vmSample.last_value
Line 1007: for key in lastSample:
Line 1008: if not key.endswith("path"):
Line 1009: continue
Really nice way to shorten this:
keys = [key for key in lastSample if key.endswith('path')]
for key in keys:
...
Use it if you like it.
Line 1010:
Line 1011: _, index, _ = key.split(".", 2)
Line 1012: try:
Line 1013: name = lastSample['block.%s.name' % index]
Line 1007: for key in lastSample:
Line 1008: if not key.endswith("path"):
Line 1009: continue
Line 1010:
Line 1011: _, index, _ = key.split(".", 2)
In this case I would prefer you use short descriptive names for the unused parts of the
split.
Line 1012: try:
Line 1013: name = lastSample['block.%s.name' % index]
Line 1014: except KeyError:
Line 1015: self.log.warning("Drive %s is not in bulk stats,
skipping"
Line 1012: try:
Line 1013: name = lastSample['block.%s.name' % index]
Line 1014: except KeyError:
Line 1015: self.log.warning("Drive %s is not in bulk stats,
skipping"
Line 1016: " it",
Get rid it " it" from the message and bring the line with name up one line.
Line 1017: name)
Line 1018: continue
Line 1019:
Line 1020: if name != drive.name:
Line 1023: path = lastSample[key]
Line 1024: if volUUID == _pathToVolumeID(drive, path):
Line 1025: allocKey = 'block.%s.allocation' % index
Line 1026: return lastSample[allocKey]
Line 1027:
Best log a warning message here too that the volume could not be found.
Line 1028: return None
Line 1029:
Line 1030: def _chunkedDrives(self):
Line 1031: """
--
To view, visit
https://gerrit.ovirt.org/60889
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52199bb38a2d3f439d33b9e78743c3e611349672
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes