Nir Soffer has posted comments on this change. Change subject: host stats: Add cpusStatus to vdsStats ......................................................................
Patch Set 7: (4 comments) https://gerrit.ovirt.org/#/c/46270/7/tests/samplingTests.py File tests/samplingTests.py: Line 271: self._hs = sampling.HostStatsThread(self.log) Line 272: self.assertEquals(self._hs.get(), expected) Line 273: Line 274: def testCpusStatus(self): Line 275: cpusStatus = [True, False, True, False] > do we really need this temporary? Please inline it I think we do - otherwise you have to duplicate this list, once in the return value of the fake getCPUMap, and once in the assert. Line 276: Line 277: def get(): Line 278: class LibvirtConnection(object): Line 279: def getCPUMap(self): Line 280: return (4, cpusStatus, 2) Line 281: return LibvirtConnection() Line 282: Line 283: with MonkeyPatchScope([(time, 'time', lambda: 0), Line 284: (libvirtconnection, 'get', get)]): Because you monkeypatch libvirtconnection.get, you need to clear its cache in tearDown. Line 285: self._hs = sampling.HostStatsThread(self.log) Line 286: self.assertEquals(self._hs.get()['cpusStatus'], cpusStatus) Line 287: Line 288: def testSamplesWraparound(self): https://gerrit.ovirt.org/#/c/46270/7/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 2072: # the KSM feature (in bytes) Line 2073: # Line 2074: # @memCommitted: Amount of memory committed to running VMs (in MB) Line 2075: # Line 2076: # @cpusStatus: Boolean list indicating wich cpus are online List of booleans indicating which cpus are online (True) or offline (False). Better add this at the end, make it easier to find when a feature was added. Line 2077: # (new in version 4.17.9) Line 2078: # Line 2079: # @swapTotal: The total amount of swap space (in MB) Line 2080: # https://gerrit.ovirt.org/#/c/46270/7/vdsm/virt/sampling.py File vdsm/virt/sampling.py: Line 687: corresponds to the CPU index. Line 688: Line 689: :return list like [True, True, False, False, True] Line 690: """ Line 691: return libvirtconnection.get().getCPUMap()[1] Two issues: - What if libvirt is blocked when we request the data? - What is this raises (e.g, ibvirt internal error) this will fail the entire request, right? Typically we poll state regularly and report cached values, which solve both issues. Last, do we plan to return more info about cpus in the future? It would be sad to have a third data structure. If we have a slight chance to have more info, I would use a dictionary which is easy to extend in future version: [ {'online': True}, {'online': False}, ... ] So when we add another feature, e,g, cpu temperature, it will be very easy: [ {'online': True, 'temp', 87}, {'online': False, 'temp', 52}, ... ] Line 692: Line 693: Line 694: def _get_cpu_core_stats(first_sample, last_sample): Line 695: interval = last_sample.timestamp - first_sample.timestamp -- To view, visit https://gerrit.ovirt.org/46270 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I73c92f563c7c3e8ffbbcf639eba27fbe55389994 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Roman Mohr <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Roman Mohr <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
