Adam Litke has posted comments on this change.
Change subject: Live Merge: Restore watermark tracking
......................................................................
Patch Set 10:
(4 comments)
https://gerrit.ovirt.org/#/c/60889/10/vdsm/virt/vm.py
File vdsm/virt/vm.py:
PS10, Line 199: if vol["path"].split('/')[-1] ==
path.split('/')[-1]:
This makes assumptions about the format of path strings which is not allowed in virt code.
Is there a reason you can't just compare path and vol["path"} directly?
Line 935: except libvirt.libvirtError as e:
Line 936: self.log.error("Unable to get watermarks for drive %s:
%s",
Line 937: drive.name, e)
Line 938: continue
Line 939:
Nice :)
Line 940: return ret
Line 941:
Line 942: def _getLiveMergeExtendCandidates(self):
Line 943: candidates = []
Line 991: volUUID)
Line 992:
Line 993: return candidates
Line 994:
Line 995: def _getVolumeWriteWatermarks(self, drive, volUUID):
Looks good! Simpler to follow since we're looking for just one volume now.
It might be helpful to add a comment here explaining how we look up the alloc value
including an example of the keys and values from the bulk stats that are involved in the
lookup.
Line 996: vmSample = sampling.stats_cache.get(self.id)
Line 997: lastSample = vmSample.last_value
Line 998: keys = [key for key in lastSample if key.endswith('path')]
Line 999: for key in keys:
PS10, Line 4744: drive.imageID
I'm baffled by the use of imageID. Isn't that supposed to take a volumeID?
Do you think it would be a good idea to just trigger the top-level extendDrivesIfNeeded
flow from here?
--
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: 10
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