Adam Litke has posted comments on this change.
Change subject: Live Merge: Restore watermark tracking
......................................................................
Patch Set 3:
(6 comments)
https://gerrit.ovirt.org/#/c/60889/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:
PS3, Line 928: else:
A single vm drive could require extensions to a merging volume and the leaf at the same
time.
PS3, Line 961: blockDev
Should this be drive.chuncked?
PS3, Line 984: if volInfo['format'].lower() != 'cow':
: continue
using drive.chuncked above may mitigate the need for this check.
PS3, Line 1008: vol_uuid = os.path.basename(path)
This is not allowed in virt code. We should either add a helper in HSM to convert a path
into imgUUID and volUUID or iterate over the VM devices looking for one that contains a
matching path.
PS3, Line 4738: maxAlloc = 1
This isn't quite right. self.extendDriveVolume expects a unit in bytes for the
curSize parameter. However, 1GB is not the right answer when you have no watermark info
because extendDriveVolume expects to receive the current size, not the amount by which you
want to increase. I'm wodering if it makes sense to use baseInfo['truesize'].
For block volumes this is the LV size and would automatically cause the LV to grow by
1GB. This isn't ideal but the alternatives aren't much better.
PS3, Line 4740: if drive.imageID in candidates.keys():
: mergeCandidate = candidates[drive.imageID]
: maxAlloc = mergeCandidate['alloc']
This would look nicer as a try/except block:
alloc = 1
try:
candidate = self._getLiveMergeExtendCandidates()[drive.imageID]
alloc = candidate['alloc']
except KeyError:
pass
--
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: 3
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