Ala Hino has posted comments on this change.
Change subject: Live Merge: Restore watermark tracking
......................................................................
Patch Set 5:
(18 comments)
https://gerrit.ovirt.org/#/c/60889/5/vdsm/virt/vm.py
File vdsm/virt/vm.py:
PS5, Line 200: st = os.stat(vol["path"])
Is there a reason you can't just compare the path strings and
return the vo
Done
PS5, Line 946: ret.append((drive, mergeCandidate['volumeID'],
: mergeCandidate['capacity'],
: mergeCandidate['alloc'],
: mergeCandidate['physical']))
This should be outside the try block (or in an else clause of the try
block
Done
PS5, Line 956: # The common case is that there are no active jobs.
: if not self.conf['_blockJobs']:
: return {}
This might not be needed if we move the call to
self._getWriteWatermarks()
Done
PS5, Line 961: watermarks = self._getWriteWatermarks()
Looking at this closely, I think we can optimize this code further...
see b
Done
PS5, Line 966: # After an active layer merge completes the vdsm metadata will
: # be out of sync for a brief period. If we cannot find the
old
: # disk then it's safe to skip it.
Let's update this comment a bit:
Done
PS5, Line 969: self.log.debug("Couldn't find drive %s. After an active"
: " layer merge completes the vdsm
metadata"
: " will be out of sync for a brief
period."
: " If we cannot find the old disk then
it's"
: " safe to skip it",
: job['disk'])
And we can make this log message more concise:
Done
PS5, Line 987: if volumeID not in watermarks:
: self.log.warning("No watermark info available for
%s",
: volumeID)
: continue
Move this block after the volumeInfo call and check for format ==
'cow'. T
Done
PS5, Line 992: res = self.cif.irs.getVolumeInfo(drive.domainID, drive.poolID,
: drive.imageID, volumeID)
: if res['status']['code'] != 0:
: self.log.error("Unable to get the info of volume %s
(domain: "
: "%s image: %s)", volumeID,
drive.domainID,
: drive.imageID)
: continue
: volInfo = res['info']
please use the self._getVolumeInfo() helper for this. You'll
have to catch
Done
Line 998: continue
Line 999: volInfo = res['info']
Line 1000: if volInfo['format'].lower() != 'cow':
Line 1001: continue
Line 1002:
Here is where you can finally get the watermark...
Done
Line 1003: self.log.debug("Adding live merge extension candidate: "
Line 1004: "volume=%s allocation=%i", volumeID,
Line 1005: watermarks[volumeID])
Line 1006: candidates[drive.imageID] = {
PS5, Line 1014: self
Let's rename this to _getVolumeWriteWatermark(self, drive,
volUUID) and sea
Done
PS5, Line 1019: if key.endswith("path"):
If you write this as:
Done
PS5, Line 1023: xcept KeyError:
This is an unexpected situation. You should probably add a warning
message
Done
Line 1020: prefix, index, attr = key.split(".", 2)
Line 1021: try:
Line 1022: name = lastSample['block.%s.name' % index]
Line 1023: except KeyError:
Line 1024: continue
If name doesn't match the drive name we can move to the next one.
Done
Line 1025:
Line 1026: try:
Line 1027: drive = self._findDriveByName(name)
Line 1028: except LookupError:
PS5, Line 1026: try:
: drive = self._findDriveByName(name)
: except LookupError:
: self.log.error("Unable to find drive
'%s'", name)
: continue
We don't need to look up the drive anymore since we were already
passed it.
Done
PS5, Line 1032: if not drive.chunked:
: continue
This function doesn't care about this. It just looks up
watermarks for thi
Done
Line 1034:
Line 1035: path = lastSample[key]
Line 1036: allocKey = 'block.%s.allocation' % index
Line 1037: allocation = lastSample[allocKey]
Line 1038: volumeID = _pathToVolumeID(drive, path)
if volumeID == _pathToVolumeID(drive, path):
Done
Line 1039: watermarks[volumeID] = allocation
Line 1040:
Line 1041: return watermarks
Line 1042:
PS5, Line 4765: capacity, alloc, physical
We're only using capacity so let's do:
Done
Line 4762: return response.error('mergeErr')
Line 4763:
Line 4764: if drive.chunked and baseInfo['format'] == 'COW':
Line 4765: capacity, alloc, physical = self._getExtendInfo(drive)
Line 4766: # maxAlloc is set by default to disk true size
Let's explain this a bit more thoroughly. Also,
'maxAlloc' is a bad name f
Done
Line 4767: maxAlloc = baseInfo['truesize']
Line 4768: try:
Line 4769: candidates = self._getLiveMergeExtendCandidates()
Line 4770: mergeCandidate = candidates[drive.imageID]
--
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: 5
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