oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9314/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10098/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/875/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10254/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5180/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3338/ : SUCCESS

Martin Sivák has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 3: Verified-1

WIP, with some design decisions still pending.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9336/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10120/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/882/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10276/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5202/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3360/ : SUCCESS

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 5: Code-Review-1 Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9419/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10203/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10359/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5285/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3443/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/917/ : There was an infra issue, please contact infra@ovirt.org

Martin Sivák has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 6: Verified+1

Verified both live and using unittests.

Martin Sivák has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 8: Verified+1

Verified both live and using unittests after rebase.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 9:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9757/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10542/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1076/ : There was an infra issue, please contact infra@ovirt.org

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10699/ : FAILURE

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 11:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9858/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10643/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1100/ : There was an infra issue, please contact infra@ovirt.org

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10800/ : SUCCESS

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 13:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9875/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10660/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1111/ : There was an infra issue, please contact infra@ovirt.org

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10817/ : SUCCESS

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 15:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9911/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10696/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1131/ : There was an infra issue, please contact infra@ovirt.org

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10853/ : SUCCESS

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 17:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9925/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10710/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1141/ : There was an infra issue, please contact infra@ovirt.org

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10867/ : SUCCESS

Martin Sivák has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 18: Verified+1

Verified using updateVmPolicy to set the value and virsh and getIoTunePolicy to get the data back.

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

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 20:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9996/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10781/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1167/ : There was an infra issue, please contact infra@ovirt.org

http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/78/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc19_created/110... : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/89/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10938/ : SUCCESS

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 21:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10052/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10837/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1188/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/85/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc19_created/117... : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/96/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10994/ : SUCCESS

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(...)

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 23:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10088/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10873/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1200/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/89/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc19_created/121... : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/100/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11030/ : SUCCESS

Francesco Romani has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 22:

(1 comment)

http://gerrit.ovirt.org/#/c/28715/22/vdsm/virt/vmtune.py File vdsm/virt/vmtune.py:

Line 167: Line 168: for el in ioTune.getElementsByTagName("device"): Line 169: try: Line 170: ioTuneByPath[el.getAttribute("path")] = el Line 171: except KeyError:

The path is not mandatory.

I see. Then please here and right below in this function add a comment (a copy/paste of the answer above is just fine) stating this, and please switch to the

if el.hasAttribute("path"): ioTuneByPath[el.getAttribute("path")] = el

idiom like is done above in the code (ioTuneDomToValues). I think in this particular context LBYL is better before EAFP. Line 172: pass Line 173: Line 174: try: Line 175: ioTuneByName[el.getAttribute("name")] = el

Francesco Romani has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 24: Code-Review+1

good enough for me.

Dan Kenigsberg has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 24:

(1 comment)

http://gerrit.ovirt.org/#/c/28715/24/vdsm/virt/vmtune.py File vdsm/virt/vmtune.py:

Line 26: Line 27: log = logging.getLogger(__name__) Line 28: Line 29: Line 30: def ioTuneValuesToDom(values, dom): We've agreed that new functions, certainly in new modules, should have modern-looking names - insteadOfTheCamelCaseUsedHere.

It's quite late in the review process (and a lot of s/// are due), but would you conform to this guideline? Line 31: """ Line 32: Create a DOM representation of the passed iotune values and Line 33: attach it to the dom object in the form of nodes. Line 34:

Dan Kenigsberg has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 24:

(2 comments)

http://gerrit.ovirt.org/#/c/28715/24/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json:

Line 7575: # Line 7576: # Since: 4.15.0 Line 7577: ## Line 7578: {'command': {'class': 'VM', 'name': 'updateVmPolicy'}, Line 7579: 'data': {'vmID': 'UUID', '*vcpuLimit': 'int', '*ioTune': ['VmDiskDeviceTuneLimits']}}

No change is needed there. This method gets all arguments using params dict

but lines are better kept below 80 chars.

http://gerrit.ovirt.org/#/c/28715/24/vdsm/virt/vmtune.py File vdsm/virt/vmtune.py:

Line 26: Line 27: log = logging.getLogger(__name__) Line 28: Line 29: Line 30: def ioTuneValuesToDom(values, dom):

I would prefer to do that in another patch if that is ok with you, it will

fine. a follow up patch would do - please do not forget it. Line 31: """ Line 32: Create a DOM representation of the passed iotune values and Line 33: attach it to the dom object in the form of nodes. Line 34:

Martin Sivák has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 25: Verified+1

No chane in code since the last verification, only the schema changed and passed the sanity checks.

Dan Kenigsberg has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 25: Code-Review+2

Same as version acked by Francesco

Martin Sivák has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 26: Verified+1

Rebase only, tests ok.

Dan Kenigsberg has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 26: Code-Review+2

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add IO tunables support to updateVmPolicy ......................................................................

Patch Set 27:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5596/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3754/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged/1612/ : SUCCESS