Francesco Romani has posted comments on this change. Change subject: Add IO tunables support to updateVmPolicy ......................................................................
Patch Set 22: Code-Review-1 (5 comments) Thanks for splitting this. After a closer inspection, looks like the code in vmtune.py has some opportunities to be made nicer and (I believe) simpler. However, since we are already at patchset 22, this can be delayed to future work. I don't see big problems here, just a few (hopefully) minor things. Please see inline comments (but let's discard my big comment in updateIoTuneDom and let's save this for future work). -1 for visibility, but seems quite near to be merged. http://gerrit.ovirt.org/#/c/28715/22/vdsm/virt/vmtune.py File vdsm/virt/vmtune.py: Line 151: Line 152: return result Line 153: Line 154: Line 155: def createDeviceIndex(ioTune): Why 'Device'? Maybe something like createIoTuneIndex is better. Line 156: """ Line 157: Create by name / by path dictionaries from the XML representation. Line 158: Returns a tuple (by_name, by_path) where the items are the respective Line 159: dictionaries. Line 167: Line 168: for el in ioTune.getElementsByTagName("device"): Line 169: try: Line 170: ioTuneByPath[el.getAttribute("path")] = el Line 171: except KeyError: Sorry, I don't quite get what can trigger a KeyError here Line 172: pass Line 173: Line 174: try: Line 175: ioTuneByName[el.getAttribute("name")] = el Line 171: except KeyError: Line 172: pass Line 173: Line 174: try: Line 175: ioTuneByName[el.getAttribute("name")] = el ditto Line 176: except KeyError: Line 177: pass Line 178: Line 179: return ioTuneByName, ioTuneByPath Line 203: old_tune = ioTuneByName[limit_object["name"]] Line 204: ioTune.removeChild(old_tune) Line 205: elif ("path" in limit_object Line 206: and limit_object["path"] in ioTuneByPath): Line 207: old_tune = ioTuneByPath[limit_object["path"]] In a later patch: all of the above can be factored in a function. def findIoTuneInIndex(ioTuneByName, ioTuneByPath, limit_object): if ("name" in limit_object and limit_object["name"] in ioTuneByName): return ioTuneByName[limit_object["name"]] elif ("path" in limit_object and limit_object["path"] in ioTuneByPath): return ioTuneByPath[limit_object["path"]] return None then in the client code ... for limit_object in tunables: old_tune = findIoTuneInIndex(ioTuneByName, ioTuneByPath, limit_object) if old_tune: ioTune.removeChild(old_tune) old_object = ... This can probably pave the road to more refactoring about how the loop is performed, but this can be made in followup patches Line 208: ioTune.removeChild(old_tune) Line 209: Line 210: if old_tune is not None: Line 211: old_object = ioTuneDomToValues(old_tune) Line 224: if ("path" in limit_object Line 225: and limit_object["path"] in ioTuneByPath): Line 226: ioTuneByPath[limit_object["path"]] = new_tune Line 227: Line 228: return count Do we care of the actual number of tunables changed? Doesn't seem so. If that's right, what about return count > 0 (or something even more esplicit), so in the calling code we can just have metadata_modified = updateIoTuneDom(...) -- To view, visit http://gerrit.ovirt.org/28715 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ed108fbb2bf9d9af80577b2905242bf9f8c4221 Gerrit-PatchSet: 22 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Sivák <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
