Change in vdsm[master]: adding API method for blkio limits MOM integration

nsoffer at redhat.com nsoffer at redhat.com
Tue Jun 10 20:29:13 UTC 2014


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 <gchaplik at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchaplik at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Xavi Francisco <xfrancis at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list