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