Change in vdsm[master]: Adding utility methods and conf for CPU limit MOM integration

danken at redhat.com danken at redhat.com
Wed May 21 08:51:23 UTC 2014


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 <kobi at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: David Caro <dcaroest at redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchaplik at redhat.com>
Gerrit-Reviewer: Kobi Ianko <kobi at redhat.com>
Gerrit-Reviewer: Martin Sivák <msivak at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list