Nir Soffer has posted comments on this change. Change subject: adding API method for blkio limits MOM integration ......................................................................
Patch Set 2: (9 comments) http://gerrit.ovirt.org/#/c/28547/2//COMMIT_MSG Commit Message: Line 5: CommitDate: 2014-06-10 22:05:00 +0300 Line 6: Line 7: adding API method for blkio limits MOM integration Line 8: Line 9: for more information refer to: www.ovirt.org/Features/blkio-support The commit message should explain this patch. Don't send us to read feature pages. If you cannot explain the why this patch is needed in few sentences, this patch is probably not needed :-) Line 10: Line 11: Change-Id: I43b25627e5365fdd1145a75e91fb5b7714e46aaf http://gerrit.ovirt.org/#/c/28547/2/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 1292: raise ValueError('A non-zero total value and non-zero' Line 1293: ' read/write value for %s_sec can not be' Line 1294: ' set at the same time' % category) Line 1295: Line 1296: def _normalizeIoTuneParams(self): This is not related to this change. Please do this in another patch. Line 1297: ioTuneParams = ('total_bytes_sec', 'read_bytes_sec', Line 1298: 'write_bytes_sec', 'total_iops_sec', Line 1299: 'write_iops_sec', 'read_iops_sec') Line 1300: for key, value in self.specParams['ioTune'].iteritems(): Line 4618: except ValueError: Line 4619: return self._reportError(key='cpuTuneErr', msg= Line 4620: 'an integer is required for period') Line 4621: except libvirt.libvirtError as e: Line 4622: return self._reportError(key='cpuTuneErr', msg=str(e)) How is this related to your change? Please separate logical changes - cleanup error handling - improve function names - add setDiskBlkioTuneMap Line 4623: return {'status': doneCode} Line 4624: Line 4625: def setDiskBlkioTuneMap(self, disk, blkIoTuneMap): Line 4626: if not disk.device == 'disk' or not isVdsmImage(disk): Line 4637: except libvirt.libvirtError as e: Line 4638: return self._reportError(key='blkioTuneErr' msg=e.message) Line 4639: return {'status': doneCode} Line 4640: Line 4641: def _checkIoTuneValidParams(self, blkIoTuneMap): Lets call this _validateIoTuneKyes. Line 4642: validKeys = set(('total_bytes_sec', 'read_bytes_sec', Line 4643: 'write_bytes_sec', 'total_iops_sec', Line 4644: 'write_iops_sec', 'read_iops_sec')) Line 4645: keys = set(blkIoTuneMap) Line 4646: Line 4647: invalidKeys = keys - validKeys Line 4648: if invalidKeys: Line 4649: names = ', '.join(invalidKeys) Line 4650: raise ValueError('Parameter %s name(s) are invalid' % names) Lets move this below _normalizeIoTuneParams, since this is utility used by that function. Line 4651: Line 4652: def _normalizeIoTuneParams(self, blkIoTuneMap): Line 4653: self._checkIoTuneValidParams(blkIoTuneMap) Line 4654: self._checkIoTuneCategories(blkIoTuneMap) Line 4650: raise ValueError('Parameter %s name(s) are invalid' % names) Line 4651: Line 4652: def _normalizeIoTuneParams(self, blkIoTuneMap): Line 4653: self._checkIoTuneValidParams(blkIoTuneMap) Line 4654: self._checkIoTuneCategories(blkIoTuneMap) These two methods are not about normalization but about validation. So I think we should have 3 methods: - _validateIoTuneKeys - _validateIoTuneCategories - _validateIoTuneValues - this does both normalization and validation, since it raises when a value cannot be converted to int. So maybe _validateAndNormalizeIoTuneValues? It seems that you need these 3 functions twice in this module, so it is probably not a good idea to duplicate the code to call all 3 of them, but putting 2 of them inside the third is not the correct way. You need a forth one to call all of them: def _validateIoTuneMap(self, map): self._validateIoTuneMapKeys(map) self._validateIoTuneMapValues(map) self._validateIoTuneCategories(map) Line 4655: Line 4656: try: Line 4657: for key, value in blkIoTuneMap.iteritems(): Line 4658: blkIoTuneMap[key] = int(value) Line 4657: for key, value in blkIoTuneMap.iteritems(): Line 4658: blkIoTuneMap[key] = int(value) Line 4659: except ValueError as e: Line 4660: raise ValueError("Integer required for %s: %s" % (key, value)) Line 4661: raise goto fail? Line 4662: Line 4663: def _reportError(self, key='unavail', msg=None): Line 4664: """ Line 4665: Convert an exception to an error status. http://gerrit.ovirt.org/#/c/28547/2/vdsm_api/vdsmapi-schema.json File vdsm_api/vdsmapi-schema.json: Line 7332: # @VM.setCpuTunePeriod: Line 7333: # Line 7334: # Set the vCpu period tune parameter to the VM Line 7335: # Line 7336: # @period: a number representing the period to be setf setf? writing too much lisp? Line 7337: # Returns: Line 7338: # Status code Line 7339: # Line 7340: # Since: 4.15.0 Line 7350: # Disk: Name or source file for one of the disk devices Line 7351: # attached to domain Line 7352: # Map acceptable keys: 'total_bytes_sec', 'read_bytes_sec', Line 7353: # 'write_bytes_sec', 'total_iops_sec', 'read_iops_sec', 'write_iops_sec'. Line 7354: # Map acceptable value type for all keys: integer. You have to document each key like all other commands. Line 7355: # Returns: Line 7356: # Status code Line 7357: # Line 7358: # Since: 4.15.0 -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[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
