Nir Soffer has posted comments on this change. Change subject: adding API methods for blkio limits MOM integration ......................................................................
Patch Set 1: (13 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 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 supporting only some of the keys libvirt supports. If you do so, then you *must* explain here why you are supporting only some keys. If you just list the name of the keys libvirt supports, lets remove this from the commit message. 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? Why should I care about this detail when I read your commit message? What I like to see here is why this change is needed - what problem it solve. Why this is the best solution to this problem. Finally, please keep the commit message lines within 80 characters. As is, it looks sloppy. 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: vdsm_api/vdsmapi-schema.json There is no point in duplication the documentation here and in the schema. What happens when libvirt support a new parameter? do we have to go and change this documentation? 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 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 let if fail? Most of the code in this patch is about validation of the parameters. This mean that when libvirt support new parameters, we have to edit this code to support new parameters. It would be much better if we accept any parameter and pass it to libvirt, so supporting new parameters requires only change in the engine side. Validation is needed only if libvirt input validation is broken and we may pass parameters that can break libvirt. If libvirt has robust input validation, I don't want to maintain this code here. Also when this fail, we will raise ValueError, while if the libvirt call fails, we return self._reportError(). This may be the poor convention in this code, so please check if other methods here behave like 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. 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? This should be _checkIoTuneValidParams. 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" 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. 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: if invalidKeys: names = ', '.join(invalidKeys) raise ValueError('Parameter %s name(s) are invalid' % names) 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? 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? If the we expect string->int, we don't need to modify the values in the map, so we should do: for key, value in blkIoTuneMap.iteritems(): try: int(value) except ValueError: ... If we expect string->string, then this method name is wrong, this is not a "check" but "normalize" or some other name that make it clear that we modify the input argument. 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 deprecated. You also assume that raise will raise the original object with your modification, which is possibly true. This make this code fragile for no benefit. The clean way to do this is to raise a new ValueError here: raise ValueError('Integer is required for parameter %s' % key) But we would like to show the bad value, so: raise ValueError("Integer required for %s: %s" % (key, value)) 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: 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
