Adam Litke has posted comments on this change.
Change subject: Live Merge: Restore watermark tracking
......................................................................
Patch Set 5:
(18 comments)
Great patch! I gave this a pretty hands-on review because I saw some room for making it a
bit cleaner and more efficient.
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 volUUID if
they match?
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. We don't
want to ignore KeyErrors from this line.
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() inside the for
loop. See below.
PS5, Line 961: watermarks = self._getWriteWatermarks()
Looking at this closely, I think we can optimize this code further... see below.
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:
# After an active layer merge completes the volume ID of the drive is updated and it no
longer matches the original volume ID specified in the saved block jobs. If we can't
find the drive we assume this is the reason and that it's safe to skip it.
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:
self.log.debug("Cannot find drive for block job %s. Skipping extend check",
job['id'])
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'. This way
we delay watermark parsing until absolutely necessary. Usually watermark info should not
be missing so this condition should almost never occur.
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
StorageUnavailableError and continue
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...
This is the first time you actually need the watermarks. So in all the above cases we can
avoid calling it and instead here we can do:
alloc = self._getVolumeWatermark(drive, volUUID)
if alloc:
... add candidate ...
else:
... Warn about missing info ...
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 search for
just one watermark.
PS5, Line 1019: if key.endswith("path"):
If you write this as:
if not key.endswith('path'):
continue
Then you can save one level of indentation for the rest of the function.
PS5, Line 1023: xcept KeyError:
This is an unexpected situation. You should probably add a warning message.
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.
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.
PS5, Line 1032: if not drive.chunked:
: continue
This function doesn't care about this. It just looks up watermarks for this drive and
returns the one associated with the given volume id.
We already skip non-chunked drives in the caller.
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):
return allocation
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:
capacity, _, _ = self._getExtendInfo(drive)
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 for
this variable. If you don't use alloc above you can use that name here.
Our comment should state that we would prefer to get the alloc value from libvirt but in
case its not available we use the truesize which will guarantee an extension will be done
in order to prevent the merge from failing immediately.
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