Change in vdsm[master]: vdsm: Add blkIoTune support at vm creation

wudxw at linux.vnet.ibm.com wudxw at linux.vnet.ibm.com
Fri May 17 02:35:21 UTC 2013


Mark Wu has posted comments on this change.

Change subject: vdsm: Add blkIoTune support at vm creation
......................................................................


Patch Set 6: I would prefer that you didn't submit this

(5 inline comments)

just minor problems.

....................................................
Commit Message
Line 13: Adding 'blkIoTune':{'read_bytes_sec': 6120000, 'total_iops_sec': 800} for a
Line 14: block device will add the following in dom xml for that device.
Line 15: 
Line 16: <iotune>
Line 17:     <read_bytes_sec>6120000</total_bytes_sec>
yep, good catch! The end tag should be 'read_bytes_sec'
Line 18:     <total_iops_sec>800</total_iops_sec>
Line 19: </iotune>
Line 20: 
Line 21: These parameters constrains the I/O bandwith for the device.


....................................................
File tests/libvirtvmTests.py
Line 394:             {'index': '0', 'propagateErrors': 'on', 'iface': 'virtio',
Line 395:              'name': 'vda', 'format': 'cow', 'device': 'disk',
Line 396:              'path': '/tmp/disk1.img', 'type': 'disk', 'readonly': 'False',
Line 397:              'shared': 'True', 'serial': SERIAL,
Line 398:              'blkIoTune': {'read_iops_sec': 1000, 'total_iops_sec': 2000}},
it's better to generate test devConfs dynamically instead of hard coding since only the value of 'blkIoTune' changes. It make the code look cleaner.
Line 399: 
Line 400:             {'index': '0', 'propagateErrors': 'on', 'iface': 'virtio',
Line 401:              'name': 'vda', 'format': 'cow', 'device': 'disk',
Line 402:              'path': '/tmp/disk1.img', 'type': 'disk', 'readonly': 'False',


Line 422:             ' than zero',
Line 423:             'parameter aaa name is invalid',
Line 424:             'an integer is required for blkIoTune parameter read_iops_sec']
Line 425: 
Line 426:         blockDevs = [False, False, False, False]
If your feature is not related to the type of backend storage, you don't need a list for it. You could just assign False to drive._blockDev in line 435
Line 427:         vmConfs = [{'custom': {'viodiskcache': 'writethrough'}},
Line 428:                    {'custom': {'viodiskcache': 'writethrough'}},
Line 429:                    {}, {}]
Line 430: 


....................................................
File vdsm/libvirtvm.py
Line 1123:             else:
Line 1124:                 raise ValueError('parameter %s value should be'
Line 1125:                                  ' equal or greater than zero' % key)
Line 1126: 
Line 1127:     def _checkCategories(self):
probably you need rename this function because it's too generic. It's only related to IO tune, right?
Line 1128:         categories = ("bytes", "iops")
Line 1129:         for category in categories:
Line 1130:             if self.blkIoTune.get('total_' + category + '_sec', 0) and \
Line 1131:                     (self.blkIoTune.get('read_' + category + '_sec', 0) or


Line 1210:                 diskelem.appendChildWithArgs('readonly')
Line 1211: 
Line 1212:         if hasattr(self, 'blkIoTune'):
Line 1213:             self._validateIoTuneParams()
Line 1214:             self. _checkCategories()
it is also validating the parameters, right?  Why not calling it in _validateIoTuneParams?
Line 1215:             iotune = XMLElement('iotune')
Line 1216:             for tuneElementKey in self.blkIoTune.keys():
Line 1217:                 iotune.appendChildWithArgs(
Line 1218:                     tuneElementKey,


--
To view, visit http://gerrit.ovirt.org/14636
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4034648620ed9212c06c12607bb889d97cca9d6
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mei Liu <liumbj at linux.vnet.ibm.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck at redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvallare at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Martin Sivák <msivak at redhat.com>
Gerrit-Reviewer: Mei Liu <liumbj at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list