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