Francesco Romani has posted comments on this change. Change subject: Add IO tunables support to updateVmPolicy ......................................................................
Patch Set 19: Code-Review-1 (9 comments) The code doesn't look bad, but in the hope to start bringing back vm.py to sanity, I'd like to see this code moved in a new module or, as last resort, in vdsm/virt/utils.py. http://gerrit.ovirt.org/#/c/28715/19/tests/vmTests.py File tests/vmTests.py: Line 875: def testBuildCmdLinePPC64(self): Line 876: self.assertBuildCmdLine(CONF_TO_DOMXML_PPC64) Line 877: Line 878: def _xml_sanitizer(self, text): Line 879: return re.sub(">[\t\n ]+<", "><", text).strip() this could be a module-private function. No big deal, can be extracted in a future patch. Your call. Line 880: Line 881: def testUpdateVmPolicy(self): Line 882: with FakeVM() as machine: Line 883: dom = FakeDomain() http://gerrit.ovirt.org/#/c/28715/19/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 2348: # @VmDiskDeviceTuneLimits: Line 2349: # Line 2350: # Extra parameters for VM disk devices. Line 2351: # Line 2352: # @name: #optional The name of the taget device typo: taRget Line 2353: # Line 2354: # @path: #optional The path of the taget device Line 2355: # Line 2356: # @guaranteed: IO tune parameters - guranteed values Line 2350: # Extra parameters for VM disk devices. Line 2351: # Line 2352: # @name: #optional The name of the taget device Line 2353: # Line 2354: # @path: #optional The path of the taget device ditto Line 2355: # Line 2356: # @guaranteed: IO tune parameters - guranteed values Line 2357: # Line 2358: # @maximum: IO tune parameters - the hard limits Line 2352: # @name: #optional The name of the taget device Line 2353: # Line 2354: # @path: #optional The path of the taget device Line 2355: # Line 2356: # @guaranteed: IO tune parameters - guranteed values typo: guAranteed Line 2357: # Line 2358: # @maximum: IO tune parameters - the hard limits Line 2359: # Line 2360: # Since: 4.15.0 http://gerrit.ovirt.org/#/c/28715/19/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 3682: def _ioTuneValuesToDom(self, values, dom): Line 3683: ops = ["total", "read", "write"] Line 3684: units = ["bytes", "iops"] Line 3685: Line 3686: for op, unit in itertools.product(ops, units): Nice, but how about an explicit tuple of constants? do you plan to add more tunables in the foreseeable future? If not, I'd have a preference for the constants. Line 3687: name = op + "_" + unit + "_sec" Line 3688: if name in values and values[name] >= 0: Line 3689: el = XMLElement(name) Line 3690: el.appendTextNode(str(values[name])) Line 3702: Line 3703: def _ioTuneDomToValues(self, dom): Line 3704: values = {} Line 3705: Line 3706: # Parse device name redundant comment Line 3707: if dom.hasAttribute("name"): Line 3708: values["name"] = dom.getAttribute("name") Line 3709: Line 3710: # Parse device path Line 3706: # Parse device name Line 3707: if dom.hasAttribute("name"): Line 3708: values["name"] = dom.getAttribute("name") Line 3709: Line 3710: # Parse device path ditto Line 3711: if dom.hasAttribute("path"): Line 3712: values["path"] = dom.getAttribute("path") Line 3713: Line 3714: # Parse guranteed and maximum ioTune limits Line 3710: # Parse device path Line 3711: if dom.hasAttribute("path"): Line 3712: values["path"] = dom.getAttribute("path") Line 3713: Line 3714: # Parse guranteed and maximum ioTune limits ditto Line 3715: els = dom.getElementsByTagName("guaranteed") Line 3716: if els: Line 3717: values["guaranteed"] = {} Line 3718: self._collectInnerElements(els[0], values["guaranteed"]) Line 3871: if ("path" in limit_object Line 3872: and limit_object["path"] in ioTuneByPath): Line 3873: ioTuneByPath[limit_object["path"]] = new_tune Line 3874: Line 3875: metadata_modified = True Most, if not all, of this code seems to not use self, and since vm.py is already too huge and extremely painful to work with, I'd suggest to move as much as this code as is possible in a new vdsm/virt/vmtune.py module, or whatever nome seems more appropriate, and/or into vdsm/virt/utils.py In a later patch, we can move more tune-related code away, unchanged (except for the imports of course), so with little to none risk of breaking things. This should also require little additional verification. Line 3876: Line 3877: del params['ioTune'] Line 3878: Line 3879: # Check remaining fields in params and report the list of unsupported -- 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: 19 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
