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