Change in vdsm[master]: Adding utility methods and conf for CPU limit MOM integration
msivak at redhat.com
msivak at redhat.com
Wed Jun 11 13:11:36 UTC 2014
Martin Sivák has posted comments on this change.
Change subject: Adding utility methods and conf for CPU limit MOM integration
......................................................................
Patch Set 48:
(9 comments)
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 sam
Done
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 at 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.
Done
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-libvir
fixed
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 c
Done
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 handl
Done
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.
It might be, but I believe this approach is not that bad either. Also I am pretty sure I saw it on other places as well.
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
I am checking that with the libvirt guys as we are not sure what happens when the VM is early in the start phase. I would like to keep this for now just to be on the defensive side.
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 @RunningVmS
Done
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?
I agree and fixed that. And I changed the naming to be consistent with the rest of the fields.
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 <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: Doron Fediuck <dfediuck 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