Dan Kenigsberg has posted comments on this change. Change subject: Adding utility methods and conf for CPU limit MOM integration ......................................................................
Patch Set 48: (9 comments) partial review http://gerrit.ovirt.org/#/c/27258/48/lib/vdsm/config.py.in File lib/vdsm/config.py.in: Line 171: ('vm_sample_balloon_window', '2', None), Line 172: Line 173: ('vm_sample_cpu_tune_interval', '15', None), Line 174: Line 175: ('vm_sample_cpu_tune_window', '2', None), no one needs to be able to set this window, since no more than a single sample is needed (we currently support no less then 2, though). Please have this hard-coded to 2 in vm.py. Line 176: Line 177: ('trust_store_path', '@TRUSTSTORE@', Line 178: 'Where the certificates and keys are situated.'), Line 179: http://gerrit.ovirt.org/#/c/27258/48/lib/vdsm/constants.py.in File lib/vdsm/constants.py.in: Line 49: # Line 50: SASL_USERNAME = "vdsm@ovirt" Line 51: Line 52: # The Metadata URI for VM tunables Line 53: METADATA_VM_TUNE_URI = 'http://ovirt.org/vm/tune/1.0' all these constants should be confined in the only module that uses them. Line 54: Line 55: # A cpu quota of zero Line 56: NO_CPU_QUOTA = 0 Line 57: http://gerrit.ovirt.org/#/c/27258/48/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 265: Line 266: def _sampleCpuTune(self): Line 267: infos = self._dom.schedulerParameters() Line 268: infos['vcpuCount'] = self._dom.vcpusFlags( Line 269: libvirt.VIR_DOMAIN_MEM_CURRENT) That's not one of the flag listed in http://libvirt.org/html/libvirt-libvirt.html#virDomainVcpuFlags Line 270: Line 271: vcpuLimitUri = constants.METADATA_VM_TUNE_URI Line 272: metadataCpuLimit = self._dom.metadata( Line 273: libvirt.VIR_DOMAIN_METADATA_ELEMENT, vcpuLimitUri, 0) Line 267: infos = self._dom.schedulerParameters() Line 268: infos['vcpuCount'] = self._dom.vcpusFlags( Line 269: libvirt.VIR_DOMAIN_MEM_CURRENT) Line 270: Line 271: vcpuLimitUri = constants.METADATA_VM_TUNE_URI Please do not use the constants module. Each module should define its own constants. In this case, _METADATA_VM_TUNE_URI should be local to the vm.py module. Line 272: metadataCpuLimit = self._dom.metadata( Line 273: libvirt.VIR_DOMAIN_METADATA_ELEMENT, vcpuLimitUri, 0) Line 274: # Set an init default user value. Line 275: value = 100 Line 332: 'balloon_target': balloon_target} Line 333: Line 334: def _getCpuTuneInfo(self, stats): Line 335: Line 336: sInfo, eInfo, sampleInterval = self.sampleCpuTune.getStats() this call may return eInfo==None (if not enough samples exist) please handle with if eInfo is None: return Line 337: ret = {} Line 338: # Handling the case where quota is not set, setting to 0. Line 339: # According to libvirt API:"A quota with value 0 means no value." Line 340: if eInfo['vcpu_quota'] is None: Line 333: Line 334: def _getCpuTuneInfo(self, stats): Line 335: Line 336: sInfo, eInfo, sampleInterval = self.sampleCpuTune.getStats() Line 337: ret = {} writing directly into stats seems simpler to me. Line 338: # Handling the case where quota is not set, setting to 0. Line 339: # According to libvirt API:"A quota with value 0 means no value." Line 340: if eInfo['vcpu_quota'] is None: Line 341: ret['vcpu_quota'] = constants.NO_CPU_QUOTA Line 336: sInfo, eInfo, sampleInterval = self.sampleCpuTune.getStats() Line 337: ret = {} Line 338: # Handling the case where quota is not set, setting to 0. Line 339: # According to libvirt API:"A quota with value 0 means no value." Line 340: if eInfo['vcpu_quota'] is None: what fills None into these values? my test gave {'vcpu_quota': -1L, 'vcpu_period': 100000L, 'emulator_period': 100000L, 'emulator_quota': -1L, 'cpu_shares': 1020L} Line 341: ret['vcpu_quota'] = constants.NO_CPU_QUOTA Line 342: else: Line 343: ret['vcpu_quota'] = eInfo['vcpu_quota'] Line 344: Line 501: self._getNetworkStats(stats) Line 502: self._getDiskStats(stats) Line 503: self._getDiskLatency(stats) Line 504: self._getBalloonStats(stats) Line 505: self._getCpuTuneInfo(stats) please add these new elements to vdsm/rpc/vdsmapi-schema.json's @RunningVmStats. Line 506: self._getCpuCount(stats) Line 507: self._getUserCpuTuneInfo(stats) Line 508: Line 509: return stats Line 2703: except Exception: Line 2704: self.log.error("Error setting vm disk stats", Line 2705: exc_info=True) Line 2706: Line 2707: stats['vcpu_quota'] = decStats['vcpu_quota'] are these lines really required? Isn't line 2692 enough? stats[var] = utils.convertToStr(decStats[var]) Line 2708: stats['vcpu_period'] = decStats['vcpu_period'] Line 2709: stats['cpu_count'] = decStats['cpu_count'] Line 2710: stats['userCpuTuneInfo'] = decStats['userCpuTuneInfo'] Line 2711: stats.update(self._getGraphicsStats()) -- 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: 48 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Kobi Ianko <k...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: David Caro <dcaro...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Kobi Ianko <k...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
