Dan Kenigsberg has posted comments on this change. Change subject: Adding utility methods and conf for CPU limit MOM integration ......................................................................
Patch Set 20: Code-Review-1 (7 comments) http://gerrit.ovirt.org/#/c/27258/20/vdsm/API.py File vdsm/API.py: Line 711: if not v: Line 712: return errCode['noVM'] Line 713: return v.setCpuTuneQuota(quota) Line 714: Line 715: def setCpuTunePeriod(self, period): please test these new APIs in tests/functional/momTests.py Line 716: v = self._cif.vmContainer.get(self._UUID) Line 717: if not v: Line 718: return errCode['noVM'] Line 719: return v.setCpuTunePeriod(period) http://gerrit.ovirt.org/#/c/27258/20/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 4389: Line 4390: def _getUserCpuTuneInfo(self): Line 4391: ret = {} Line 4392: try: Line 4393: domain = self._connection.lookupByUUIDString(self.id) we should already have self._dom ready - no need to look it up again. Line 4394: Line 4395: if domain: Line 4396: vcpuLimitUri = 'http://ovirt.org/param/vcpu_limit' Line 4397: metadataCpuLimit = domain.metadata(2, vcpuLimitUri, 0) Line 4396: vcpuLimitUri = 'http://ovirt.org/param/vcpu_limit' Line 4397: metadataCpuLimit = domain.metadata(2, vcpuLimitUri, 0) Line 4398: Line 4399: if metadataCpuLimit: Line 4400: pattern = "^<vcpulimit>(.*)</vcpulimit>" xml cannot be parsed reliably with regular expressions, please use a proper xml.dom.minidom.parseString like elsewhere in the code. Line 4401: metadataCpuLimit = re.search(pattern, Line 4402: metadataCpuLimit, re.M) Line 4403: ret['vcpu_user_limit'] = metadataCpuLimit.group(1) Line 4404: else: Line 4404: else: Line 4405: ret['vcpu_user_limit'] = 100 Line 4406: Line 4407: except (AttributeError, libvirt.libvirtError): Line 4408: # Metadata is not set, nothing to do, set the defaults why should we report it, if we do not know the value? it's better to report nothing if the alternative is to lie... Line 4409: self.log.debug('Domain Metadata is not set') Line 4410: ret['vcpu_user_limit'] = 100 Line 4411: return ret Line 4412: Line 4475: def setCpuTuneQuota(self, quota): Line 4476: Line 4477: if self._dom is None: Line 4478: return self.reportError(entity='vcpu quota') Line 4479: try: please make try-except block as short as possible. in this case, if setSchedulerParameters() raises a ValueError for some reason, you'd be reporting a false error. Similarly, try to catch a libvirtError error only in places that may raise it. Line 4480: quota = int(quota) Line 4481: self._dom.setSchedulerParameters({'vcpu_quota': quota}) Line 4482: return {'status': doneCode} Line 4483: except ValueError: Line 4503: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: Line 4504: return self.reportError(key='noVM', entity='vcpu period') Line 4505: return self.reportError(msg=e.message, entity='vcpu period') Line 4506: Line 4507: def reportError(self, key='Err', msg=None, entity=None): this function should not be public. Line 4508: self.log.error("Set new "+entity+" failed", exc_info=True) Line 4509: if msg is None: Line 4510: error = errCode[key] Line 4511: else: Line 4504: return self.reportError(key='noVM', entity='vcpu period') Line 4505: return self.reportError(msg=e.message, entity='vcpu period') Line 4506: Line 4507: def reportError(self, key='Err', msg=None, entity=None): Line 4508: self.log.error("Set new "+entity+" failed", exc_info=True) style: spaces around operators. Line 4509: if msg is None: Line 4510: error = errCode[key] Line 4511: else: Line 4512: error = {'status': {'code': errCode[key] -- To view, visit http://gerrit.ovirt.org/27258 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic502d9a4a976cd76bb6042bbb51f6cd281199631 Gerrit-PatchSet: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Kobi Ianko <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: David Caro <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Kobi Ianko <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
