Change in vdsm[master]: vdsm: Add tuneBlockDevIo interface
wudxw at linux.vnet.ibm.com
wudxw at linux.vnet.ibm.com
Fri Aug 30 00:10:47 UTC 2013
Mark Wu has posted comments on this change.
Change subject: vdsm: Add tuneBlockDevIo interface
......................................................................
Patch Set 13: Code-Review-1
(16 comments)
I am sorry for not pointing out those problems in previous reviews.
....................................................
File lib/vdsm/define.py
Line 146: ERROR = 1
Line 147: NORMAL = 0
Line 148:
Line 149:
Line 150: def translateErrCode(errKey, errMsg=None):
I don't think it's kind of translation. so you should give it a better name, vdsmErrCode or customizedErrCode? I don't know.
Line 151: error = errCode[errKey].copy()
Line 152: if errMsg:
Line 153: error['status'].update({'message': errMsg})
Line 149:
Line 150: def translateErrCode(errKey, errMsg=None):
Line 151: error = errCode[errKey].copy()
Line 152: if errMsg:
Line 153: error['status'].update({'message': errMsg})
is error['status']['message'] = errMsg simpler?
....................................................
File vdsm/vm.py
Line 4313: self.log.error(errLogMsg, exc_info=True)
Line 4314: return translateErrCode('balloonErr',
Line 4315: 'An integer is required for target')
Line 4316: except libvirt.libvirtError as e:
Line 4317: self.log.error(errLogMsg, exc_info=True)
you could just use self.log.exception(errLogMsg) instead.
Line 4318: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 4319: return translateErrCode('noVM')
Line 4320: return translateErrCode('balloonErr', e.message)
Line 4321: else:
Line 4315: 'An integer is required for target')
Line 4316: except libvirt.libvirtError as e:
Line 4317: self.log.error(errLogMsg, exc_info=True)
Line 4318: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 4319: return translateErrCode('noVM')
it doesn't do any translation on noVM, right? so it's not a good name.
Line 4320: return translateErrCode('balloonErr', e.message)
Line 4321: else:
Line 4322: for dev in self.conf['devices']:
Line 4323: if dev['type'] == BALLOON_DEVICES and \
Line 4334: if device.get('device') == 'disk' and device.get('name'):
Line 4335: devName = device['name']
Line 4336: result = (device.get('specParams', {}).get('ioTune', {}))
Line 4337: if result:
Line 4338: results[devName] = result
try:
results[devName] = device['specParams']['ioTune']
except KeyError:
continue
is it better?
Line 4339: return results
Line 4340:
Line 4341: def _checkIoTuneInvalidParams(self, params):
Line 4342: validKeys = set(('total_bytes_sec', 'read_bytes_sec',
Line 4345: paramKeys = set(params)
Line 4346:
Line 4347: invalidParams = paramKeys - validKeys
Line 4348: invalidParamNames = ', '.join(invalidParams)
Line 4349: return invalidParamNames
I think we can use raise exception to replace passing back:
if invalidParamNames:
raise ValueError('tuneBlkDevIoErr',
'Parameter %s name(s) are invalid' %
invalidParamNames)
Line 4350:
Line 4351: def _checkIoTuneCategories(self, params):
Line 4352: categories = ("bytes", "iops")
Line 4353: for category in categories:
Line 4353: for category in categories:
Line 4354: if params.get('total_' + category + '_sec', 0) and \
Line 4355: (params.get('read_' + category + '_sec', 0) or
Line 4356: params.get('write_' + category + '_sec', 0)):
Line 4357: return category
raise ValueError('A non-zero total and non-zero read'
'/write value for %s_sec can not be'
' set at the same time' % category)
Line 4358:
Line 4359: def _validateIoTuneParams(self, params):
Line 4360: errLogMsg = 'Tuning block device I/O param is invalid'
Line 4361:
Line 4363: if invalidParamNames:
Line 4364: self.log.error(errLogMsg)
Line 4365: return translateErrCode('tuneBlkDevIoErr',
Line 4366: 'Parameter %s name(s) are invalid' %
Line 4367: invalidParamNames)
just call self._checkIoTuneInvalidParams(params) here
Line 4368:
Line 4369: conflictCategory = self._checkIoTuneCategories(params)
Line 4370: if conflictCategory:
Line 4371: self.log.error(errLogMsg)
Line 4372: return translateErrCode('tuneBlkDevIoErr',
Line 4373: 'A non-zero total and non-zero read'
Line 4374: '/write value for %s_sec can not be'
Line 4375: ' set at the same time' %
Line 4376: conflictCategory)
just call self._checkIoTuneCategories(params)
Line 4377:
Line 4378: try:
Line 4379: for key, value in params.iteritems():
Line 4380: params[key] = int(value)
Line 4381: except ValueError:
Line 4382: self.log.error(errLogMsg, exc_info=True)
Line 4383: return translateErrCode('tuneBlkDevIoErr',
Line 4384: 'Integer is required for parameter %s' %
Line 4385: key)
raise ValueError('tuneBlkDevIoErr',
'Integer is required for parameter % key)
Line 4386:
Line 4387: def tuneBlockDevIo(self, dev, params):
Line 4388: errLogMsg = 'Tune block device I/O failed'
Line 4389:
Line 4384: 'Integer is required for parameter %s' %
Line 4385: key)
Line 4386:
Line 4387: def tuneBlockDevIo(self, dev, params):
Line 4388: errLogMsg = 'Tune block device I/O failed'
this message is not very accurate for both of set and get tune info.
Line 4389:
Line 4390: errorInfo = self._validateIoTuneParams(params)
Line 4391: if errorInfo:
Line 4392: return errorInfo
Line 4388: errLogMsg = 'Tune block device I/O failed'
Line 4389:
Line 4390: errorInfo = self._validateIoTuneParams(params)
Line 4391: if errorInfo:
Line 4392: return errorInfo
try:
self._validateIoTuneParams(params)
except ValueError:
self.log.exception('Tuning block device I/O param is invalid')
return translateErrCode('tuneBlkDevIoErr', e.message)
Line 4393:
Line 4394: try:
Line 4395: '''
Line 4396: Set I/O tuning parameters.
Line 4402: '''
Line 4403: self._dom.setBlockIoTune(dev, params,
Line 4404: libvirt.VIR_DOMAIN_AFFECT_CURRENT)
Line 4405: except Exception as e:
Line 4406: self.log.error(errLogMsg, exc_info=True)
use self.log.exception instead.
Line 4407: return translateErrCode('tuneBlkDevIoErr', e.message)
Line 4408: else:
Line 4409: '''
Line 4410: Get the I/O tuning info after seting, since the info may be
Line 4414: try:
Line 4415: info = self._dom.blockIoTune(dev,
Line 4416: libvirt.VIR_DOMAIN_AFFECT_CURRENT)
Line 4417: except:
Line 4418: self.log.error(errLogMsg, exc_info=True)
the same as line 4406
Line 4419: return translateErrCode('tuneBlkDevIoErr',
Line 4420: 'Get new block I/O Tune params failed'
Line 4421: ' after setting.')
Line 4422: else:
Line 4422: else:
Line 4423: for device in self.conf['devices']:
Line 4424: if device.get('name') == dev or device.get('path') == dev:
Line 4425: if not device.get('specParams'):
Line 4426: device['specParams'] = {}
you could just use device.setdefault('specParams', {}) to replace the above two lines.
Line 4427: device['specParams']['ioTune'] = info
Line 4428:
Line 4429: # persist the tune value to make it consistent after recovery
Line 4430: self.saveState()
....................................................
File vdsm_api/vdsmapi-schema.json
Line 5416: #
Line 5417: # @guestFQDN: Fully qualified domain name of the guest OS. (Reported
Line 5418: # by the guest agent)
Line 5419: #
Line 5420: # @tuneBlkDevIoInfo: #optional I/O tune parameters for the VM disk devices
you'd better to clarify that if a vm is never tuned before, then tuneBlkDevIoInfo will not be included in 'RunningVmStats'
Line 5421: #
Line 5422: # Since: 4.10.0
Line 5423: ##
Line 5424: {'type': 'RunningVmStats',
--
To view, visit http://gerrit.ovirt.org/14394
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb33510a081d221af0f69d4dd2d55adf0b79efd2
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mei Liu <liumbj at linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Eli Mesika <emesika at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Mei Liu <liumbj at linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list