Kobi Ianko has posted comments on this change. Change subject: Adding support in CPU monitoring ......................................................................
Patch Set 3: (11 comments) http://gerrit.ovirt.org/#/c/27240/3//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-04-30 09:51:58 +0300 Line 4: Commit: Kobi Ianko <[email protected]> Line 5: CommitDate: 2014-04-30 15:19:28 +0300 Line 6: Line 7: Adding support in CPU monitoring > I would appreciate a bit more information here. For example, a sentence or Done Line 8: Line 9: Change-Id: I31428499ae7b34cafa3eacf5ca0d7a39eabe23ec http://gerrit.ovirt.org/#/c/27240/3/mom/Collectors/GuestCpuTune.py File mom/Collectors/GuestCpuTune.py: Line 17: This Collector uses hypervisor interface to collect guest cpu info Line 18: """ Line 19: def getFields(self=None): Line 20: return set(['vcpu_quota', 'vcpu_period', 'user_vcpu_limit', 'vcpu_count']) Line 21: > I like that you have the 'vcpu' prefix for most of these fields. Could you Done Line 22: def __init__(self, properties): Line 23: self.hypervisor_iface = properties['hypervisor_iface'] Line 24: self.uuid = properties['uuid'] Line 25: self.logger = logging.getLogger('mom.Collectors.CpuTuneInfo') Line 36: Line 37: def collect(self): Line 38: userStat = self.hypervisor_iface.getUserVmCpuTuneInfo(self.uuid) Line 39: cpuStat = self.hypervisor_iface.getVmCpuTuneInfo(self.uuid) Line 40: vcpuCount = self.hypervisor_iface.getVmCpuCount > Since these are all called together, I think it may make sense to combine t Done Line 41: Line 42: if userStat == None or cpuStat == None or vcpuCount == None: Line 43: self.stats_error('getVmCpuTuneInfo() is not ready') Line 44: else: Line 38: userStat = self.hypervisor_iface.getUserVmCpuTuneInfo(self.uuid) Line 39: cpuStat = self.hypervisor_iface.getVmCpuTuneInfo(self.uuid) Line 40: vcpuCount = self.hypervisor_iface.getVmCpuCount Line 41: Line 42: if userStat == None or cpuStat == None or vcpuCount == None: > A more compact way to write this is: Done Line 43: self.stats_error('getVmCpuTuneInfo() is not ready') Line 44: else: Line 45: self.cpu_tune_info_available = True Line 46: http://gerrit.ovirt.org/#/c/27240/3/mom/Collectors/HostCpu.py File mom/Collectors/HostCpu.py: Line 13: Line 14: from mom.Collectors.Collector import * Line 15: Line 16: class HostCpu(Collector): Line 17: > This class needs a docstring detailing the fields it produces. Done Line 18: def __init__(self, properties): Line 19: self.cpuinfo = open_datafile("/proc/cpuinfo") Line 20: Line 21: def __del__(self): http://gerrit.ovirt.org/#/c/27240/3/mom/Controllers/CpuTune.py File mom/Controllers/CpuTune.py: Line 16: class CpuTune: Line 17: """ Line 18: Controller that uses the hypervisor interface to manipulate Line 19: the cpu tuning parameters. Line 20: """ > Please provide detail on the valid tunables that can be set in this docstri Done Line 21: def __init__(self, properties): Line 22: self.hypervisor_iface = properties['hypervisor_iface'] Line 23: self.logger = logging.getLogger('mom.Controllers.Cputune') Line 24: Line 24: Line 25: def process_guest(self, guest): Line 26: quota = guest.GetControl('vcpu_quota') Line 27: period = guest.GetControl('vcpu_period') Line 28: if quota is not None and period is not None: > If the policy specifies only one of quota or period, then you will silently It's Ok they come as a couple, one without the other has no real meaning. Line 29: quota = int(quota) Line 30: period = int(period) Line 31: uuid = guest.Prop('uuid') Line 32: name = guest.Prop('name') Line 34: prev_period = guest.period Line 35: self.logger.info("CpuTune guest:%s from quota:%s period:%s to quota:%s period:%s", \ Line 36: name, prev_quota, prev_period, quota, period) Line 37: self.hypervisor_iface.setVmCpuTuneQuota(uuid, quota) Line 38: self.hypervisor_iface.setVmCpuTunePeriod(uuid, period) > Does it make sense to pass these both in a single call to the Hypervisor in Done Line 39: Line 40: def process(self, host, guests): Line 41: for guest in guests: http://gerrit.ovirt.org/#/c/27240/3/mom/HypervisorInterfaces/libvirtInterface.py File mom/HypervisorInterfaces/libvirtInterface.py: Line 254: if metadataCpuLimit: Line 255: ret['user_vcpu_limit'] = metadataCpuLimit Line 256: else: Line 257: ret['user_vcpu_limit'] = 100 Line 258: > Under which circumstances can this be None? Are we sure this default is al The user is not obligated to set the limit, I'm sure most user will not set it(because this is an advanced feature). in that case I don't want to see an error in the log. Setting the limit to 100, is like setting no limit at all, so it's a nice way to get around it without raising an error Line 259: return ret Line 260: Line 261: def getVmCpuTuneInfo(self, uuid): Line 262: domain = self._getDomainFromUUID(uuid) Line 267: ret['vcpu_quota'] = -1 Line 268: Line 269: if ret['vcpu_period'] == None: Line 270: ret['vcpu_period'] = 1000 Line 271: > Same questions as above. same answer as above :) Line 272: return ret Line 273: Line 274: def getVmCpuCount(self, uuid): Line 275: Line 276: domain = self._getDomainFromUUID(uuid) Line 277: if domain.getMaxVcpus() != -1: Line 278: return domain.getMaxVcpus() Line 279: else: Line 280: return 0 > I am not sure if it makes sense to use 0 here. It means the policy will al you are right, using the zero value will cause division by zero in the policy. I'll change it to: self.logger.error('Failed to get VM cpu count') return None and in the calling collector I will check for None. Done. Line 281: Line 282: def setVmBalloonTarget(self, uuid, target): Line 283: dom = self._getDomainFromUUID(uuid) Line 284: if dom is not None: -- To view, visit http://gerrit.ovirt.org/27240 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I31428499ae7b34cafa3eacf5ca0d7a39eabe23ec Gerrit-PatchSet: 3 Gerrit-Project: mom Gerrit-Branch: master Gerrit-Owner: Kobi Ianko <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Kobi Ianko <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
