Change in vdsm[master]: core: Add boot time to the getVdsStats API

danken at redhat.com danken at redhat.com
Thu Mar 27 17:58:48 UTC 2014


Dan Kenigsberg has posted comments on this change.

Change subject: core: Add boot time to the getVdsStats API
......................................................................


Patch Set 6: Code-Review-1

(3 comments)

http://gerrit.ovirt.org/#/c/25877/6/tests/samplingTests.py
File tests/samplingTests.py:

Line 52: procs_blocked 0
Line 53: """
Line 54: 
Line 55:     def testBootTimeParsing(self):
Line 56:         (f, path) = tempfile.mkstemp()
use

 with namedTemporaryDir()

to avoid leaving garbage behind when assertion is raised.
Line 57:         os.write(f, self.boot_time_fixture_good)
Line 58:         os.close(f)
Line 59:         self.assertTrue(sampling.getBootTime(path) == 1395249141)
Line 60:         os.remove(path)


http://gerrit.ovirt.org/#/c/25877/6/vdsm/sampling.py
File vdsm/sampling.py:

Line 148: 
Line 149: 
Line 150: @utils.memoized
Line 151: def getBootTime(filepath=None):
Line 152:     if filepath is None:
What's the benefit of this clause over putting /proc/stat on the arg list?

The "None" trick is useful with mutables, not with strings.
Line 153:         filepath = '/proc/stat'
Line 154: 
Line 155:     with file(filepath) as f:
Line 156:         btime_line = [l for l in f.readlines() if l.startswith('btime')][0]


Line 460:         stats['cpuLoad'] = hs1.cpuLoad
Line 461: 
Line 462:         stats['diskStats'] = hs1.diskStats
Line 463:         stats['thpState'] = hs1.thpState
Line 464:         if hasattr(hs1, 'bootTime'):
how could it possibly be that bootTime is missing?

More importantly: getBootTime() can never hang and can never change. It should not be copied repeatedly on each sample; Why not have a simple

  stats['bootTime'] = getBootTime()

here?
Line 465:             stats['bootTime'] = hs1.bootTime
Line 466:         return stats
Line 467: 
Line 468:     def _getInterfacesStats(self):


-- 
To view, visit http://gerrit.ovirt.org/25877
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I319e619cdaecac2f86d0154e3adbb3beda9c57d6
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei 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