Gilad Chaplik has posted comments on this change. Change subject: adding API method for blkio limits MOM integration ......................................................................
Patch Set 1: (14 comments) http://gerrit.ovirt.org/#/c/28547/1//COMMIT_MSG Commit Message: Line 7: adding API methods for blkio limits MOM integration Line 8: Line 9: Adding API methods to vm.py to integrate with MOM's collectors and controllers: Line 10: setBlkioTuneMap(device, blkioTuneMap). Line 11: This method sets blkio keys & values to the VM's device using the libvirt api. > & -> and Done Line 12: Line 13: Acceptable keys: 'total_bytes_sec', 'read_bytes_sec', 'write_bytes_sec', 'total_iops_sec', 'read_iops_sec', 'write_iops_sec'. Line 14: Acceptable value type for all keys: integer. Line 15: Line 9: Adding API methods to vm.py to integrate with MOM's collectors and controllers: Line 10: setBlkioTuneMap(device, blkioTuneMap). Line 11: This method sets blkio keys & values to the VM's device using the libvirt api. Line 12: Line 13: Acceptable keys: 'total_bytes_sec', 'read_bytes_sec', 'write_bytes_sec', 'total_iops_sec', 'read_iops_sec', 'write_iops_sec'. > This details are not needed in the commit message, unless you are supportin Done. Line 14: Acceptable value type for all keys: integer. Line 15: Line 16: Change-Id: I43b25627e5365fdd1145a75e91fb5b7714e46aaf Line 10: setBlkioTuneMap(device, blkioTuneMap). Line 11: This method sets blkio keys & values to the VM's device using the libvirt api. Line 12: Line 13: Acceptable keys: 'total_bytes_sec', 'read_bytes_sec', 'write_bytes_sec', 'total_iops_sec', 'read_iops_sec', 'write_iops_sec'. Line 14: Acceptable value type for all keys: integer. > Isn't this also part of libvirt api? Done Line 15: Line 16: Change-Id: I43b25627e5365fdd1145a75e91fb5b7714e46aaf http://gerrit.ovirt.org/#/c/28547/1/vdsm/API.py File vdsm/API.py: Line 723: """ Line 724: Sets a Blkio Tune parameter map per device. Line 725: Acceptable keys: 'total_bytes_sec', 'read_bytes_sec', 'write_bytes_sec', 'total_iops_sec', 'read_iops_sec', 'write_iops_sec'. Line 726: Acceptable value type for all keys: integer. Line 727: """ > These docs belong in the vdsm schema (psausdo) json file: Done Line 728: v = self._cif.vmContainer.get(self._UUID) Line 729: if not v: Line 730: return errCode['noVM'] Line 731: return v.setDeviceBlkioTuneMap(dev, blkIoTuneMap) http://gerrit.ovirt.org/#/c/28547/1/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 4604: except libvirt.libvirtError as e: Line 4605: return self._reportError(msg=e.message, entity='vcpu period') Line 4606: return {'status': doneCode} Line 4607: Line 4608: def setBlkioTuneMap(self, dev, blkIoTuneMap): > missing validate device in vm Done Line 4609: self._validateIoTuneParams(self, blkIoTuneMap) Line 4610: Line 4611: try: Line 4612: self._dom.setBlockIoTune(dev, blkIoTuneMap, libvirt.VIR_DOMAIN_AFFECT_CURRENT) Line 4605: return self._reportError(msg=e.message, entity='vcpu period') Line 4606: return {'status': doneCode} Line 4607: Line 4608: def setBlkioTuneMap(self, dev, blkIoTuneMap): Line 4609: self._validateIoTuneParams(self, blkIoTuneMap) > Why do we need to validate the arguments? why not pass them to libvirt and I'd let danken decide on that. Line 4610: Line 4611: try: Line 4612: self._dom.setBlockIoTune(dev, blkIoTuneMap, libvirt.VIR_DOMAIN_AFFECT_CURRENT) Line 4613: except ValueError: Line 4612: self._dom.setBlockIoTune(dev, blkIoTuneMap, libvirt.VIR_DOMAIN_AFFECT_CURRENT) Line 4613: except ValueError: Line 4614: return self._reportError(msg='a map is required for blkio tune') Line 4615: except libvirt.libvirtError as e: Line 4616: return self._reportError(msg=e.message) > Should be msg=str(e). Do not assume existence of e.message. Done Line 4617: return {'status': doneCode} Line 4618: Line 4619: def _checkIoTuneInvalidParams(self, blkIoTuneMap): Line 4620: validKeys = set(('total_bytes_sec', 'read_bytes_sec', Line 4615: except libvirt.libvirtError as e: Line 4616: return self._reportError(msg=e.message) Line 4617: return {'status': doneCode} Line 4618: Line 4619: def _checkIoTuneInvalidParams(self, blkIoTuneMap): > This name is strange - I want to check that the parameters are invalid? Thi Done Line 4620: validKeys = set(('total_bytes_sec', 'read_bytes_sec', Line 4621: 'write_bytes_sec', 'total_iops_sec', Line 4622: 'write_iops_sec', 'read_iops_sec')) Line 4623: paramKeys = set(blkIoTuneMap) Line 4619: def _checkIoTuneInvalidParams(self, blkIoTuneMap): Line 4620: validKeys = set(('total_bytes_sec', 'read_bytes_sec', Line 4621: 'write_bytes_sec', 'total_iops_sec', Line 4622: 'write_iops_sec', 'read_iops_sec')) Line 4623: paramKeys = set(blkIoTuneMap) > Should be named "keys" Done Line 4624: Line 4625: invalidParams = paramKeys - validKeys Line 4626: if invalidParams: Line 4627: invalidParamNames = ', '.join(invalidParams) Line 4621: 'write_bytes_sec', 'total_iops_sec', Line 4622: 'write_iops_sec', 'read_iops_sec')) Line 4623: paramKeys = set(blkIoTuneMap) Line 4624: Line 4625: invalidParams = paramKeys - validKeys > This should be named invalidKeys. Done Line 4626: if invalidParams: Line 4627: invalidParamNames = ', '.join(invalidParams) Line 4628: raise ValueError('Parameter %s name(s) are invalid' % Line 4629: invalidParamNames) Line 4625: invalidParams = paramKeys - validKeys Line 4626: if invalidParams: Line 4627: invalidParamNames = ', '.join(invalidParams) Line 4628: raise ValueError('Parameter %s name(s) are invalid' % Line 4629: invalidParamNames) > In the scope of this if block, we don't need such a long name, so: Done Line 4630: Line 4631: def _validateIoTuneParams(self, blkIoTuneMap): Line 4632: self._checkIoTuneInvalidParams(blkIoTuneMap) Line 4633: self._checkIoTuneCategories(blkIoTuneMap) Line 4629: invalidParamNames) Line 4630: Line 4631: def _validateIoTuneParams(self, blkIoTuneMap): Line 4632: self._checkIoTuneInvalidParams(blkIoTuneMap) Line 4633: self._checkIoTuneCategories(blkIoTuneMap) > I guess this method is missing from this patch? luckily no :-) Line 4634: Line 4635: try: Line 4636: for key, value in blkIoTuneMap.iteritems(): Line 4637: blkIoTuneMap[key] = int(value) Line 4633: self._checkIoTuneCategories(blkIoTuneMap) Line 4634: Line 4635: try: Line 4636: for key, value in blkIoTuneMap.iteritems(): Line 4637: blkIoTuneMap[key] = int(value) > Do we expect map of string -> int or string -> string? Done. /s/.../_normalizeIoTuneParams/g Line 4638: except ValueError as e: Line 4639: e.message = 'Integer is required for parameter %s' % key Line 4640: raise Line 4641: Line 4636: for key, value in blkIoTuneMap.iteritems(): Line 4637: blkIoTuneMap[key] = int(value) Line 4638: except ValueError as e: Line 4639: e.message = 'Integer is required for parameter %s' % key Line 4640: raise > You assume that ValueError has a message attribute, which I think is deprec Done Line 4641: Line 4642: def _reportError(self, key='Err', msg=None): Line 4643: """ Line 4644: Print an error and return an error status. -- To view, visit http://gerrit.ovirt.org/28547 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I43b25627e5365fdd1145a75e91fb5b7714e46aaf Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Xavi Francisco <[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
