Dan Kenigsberg has posted comments on this change. Change subject: Adding utility methods and conf for CPU limit MOM integration ......................................................................
Patch Set 20: (5 comments) 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) > since the setting of the data is done directly to the libvirt api and not v the _dom object is a reference to libvirt. If it's ever becomes stale, it's a libvirt bug, which I am not aware of. Line 4394: Line 4395: if domain: Line 4396: vcpuLimitUri = 'http://ovirt.org/param/vcpu_limit' Line 4397: metadataCpuLimit = domain.metadata(2, vcpuLimitUri, 0) Line 4392: try: Line 4393: domain = self._connection.lookupByUUIDString(self.id) Line 4394: Line 4395: if domain: Line 4396: vcpuLimitUri = 'http://ovirt.org/param/vcpu_limit' what is this URI? I see nothing there. Is it documented anywhere? Line 4397: metadataCpuLimit = domain.metadata(2, vcpuLimitUri, 0) Line 4398: Line 4399: if metadataCpuLimit: Line 4400: pattern = "^<vcpulimit>(.*)</vcpulimit>" 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>" > this was taken from elsewere in the code, collector.py at the mom project, The string looks like an xml element. If it is a real xml, it should conform to xml standards. For example, you should expect and ignore <!--comments-->. Your code does not do that. The only safe way to handle this, is with proper xml parsing. If you do not intend to store xml in the metadata string, choose another data format, that would not mislead future users of the data. 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 > I believe Dan refers to the fact that you report 100 instead of none. Doron is right - better report nothing at all than 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: > will fix I see no fix Line 4480: quota = int(quota) Line 4481: self._dom.setSchedulerParameters({'vcpu_quota': quota}) Line 4482: return {'status': doneCode} Line 4483: except ValueError: -- 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: 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
