Dan Kenigsberg has posted comments on this change. Change subject: Adding utility methods and conf for CPU limit MOM integration ......................................................................
Patch Set 22: Code-Review-1 (5 comments) http://gerrit.ovirt.org/#/c/27258/22/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 4375: Line 4376: def _getUserCpuTuneInfo(self): Line 4377: ret = {} Line 4378: try: Line 4379: domain = self._dom There's no benefit in defining this variable, which is used only once. Line 4380: Line 4381: if domain: Line 4382: # A unique URL to define the cpu limit parameter at the metadata section of libvirt Line 4383: vcpuLimitUri = 'http://ovirt.org/param/vcpu_limit' Line 4379: domain = self._dom Line 4380: Line 4381: if domain: Line 4382: # A unique URL to define the cpu limit parameter at the metadata section of libvirt Line 4383: vcpuLimitUri = 'http://ovirt.org/param/vcpu_limit' Is this URL yours to use? In my opinion, it is not descriptive enough, and it certainly leads to nowhere when browsing to it. What is this bit of metadata? Which components use it? What is its format and meaning? Which component owns it? Line 4384: metadataCpuLimit = domain.metadata(2, vcpuLimitUri, 0) Line 4385: Line 4386: if metadataCpuLimit: Line 4387: pattern = "^<vcpulimit>(.*)</vcpulimit>" Line 4393: Line 4394: except (AttributeError, libvirt.libvirtError): Line 4395: # Metadata is not set, nothing to do, set the defaults Line 4396: self.log.debug('Domain Metadata is not set') Line 4397: ret['vcpu_user_limit'] = 100 I am not convinced that reporting a value that is not truly there is a good thing to do. Line 4398: return ret Line 4399: Line 4400: def _getCpuTuneInfo(self): Line 4401: ret = {} Line 4416: ret['vcpu_period'] = 1000 Line 4417: Line 4418: except (AttributeError, libvirt.libvirtError): Line 4419: # Metadata is not set, nothing to do, set the defaults Line 4420: self.log.debug('Domain Metadata is not set') please add exc_info=True so we can tell which error took place. Please reduce the size of the try-except block, or eliminate it altogether. Line 4421: ret['vcpu_quota'] = -1 Line 4422: ret['vcpu_period'] = 1000 Line 4423: return ret Line 4424: Line 4468: try: Line 4469: self._dom.setSchedulerParameters({'vcpu_quota': quota}) Line 4470: return {'status': doneCode} Line 4471: except libvirt.libvirtError as e: Line 4472: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: why do we need this special handling of VIR_ERR_NO_DOMAIN? Do we expect the VM to be destroyed while this verb is being run? Line 4473: return self._reportError(key='noVM', entity='vcpu quota') Line 4474: return self._reportError(msg=e.message, entity='vcpu quota') Line 4475: Line 4476: def setCpuTunePeriod(self, period): -- 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: 22 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: Doron Fediuck <[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
