Change in vdsm[master]: start ksm and ksmtuned when vdsm starts

danken at redhat.com danken at redhat.com
Tue Oct 9 15:52:34 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: start ksm and ksmtuned when vdsm starts
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(7 inline comments)

I think this patch has more changes that are needed to solve the bug.

Please consider solving the bug by adding ksmtuned to vdsmd.init.in's NEEDED_SERVICES (and in systemd, too). I know that Doron would like to have vdsm running while keeping ksmtuned stopped, but I do not think it is very important.

You may take the current path, of starting ksmtuned in code - but please keep it simple.

....................................................
File vdsm/ksm.py
Line 36:             pids = utils.execCmd([constants.EXT_PGREP, '-xf', 'ksmd'],
Line 37:                                  raw=False, sudo=False)[1]
Line 38:             if pids:
Line 39:                 self._pid = pids[0].strip()
Line 40:                 self._cif.log.info('starting ksm monitor thread, ksm pid is %s', self._pid)
please keep short lines
Line 41:                 self.start()
Line 42:             else:
Line 43:                 self._cif.log.error('failed to find ksmd thread')
Line 44:         else:


Line 62:                                 os.sysconf('SC_CLK_TCK') / KSM_MONITOR_INTERVAL
Line 63:                 jiff0 = jiff1
Line 64:         except:
Line 65:             self._cif.log.error("Error monitoring KSM", exc_info=True)
Line 66:         ksm_stop()
why should we worry about stopping ksm on exception?
Line 67: 
Line 68:     def readState(self):
Line 69:         return ksm_running(), npages()
Line 70: 


Line 94:         return False
Line 95: 
Line 96: def ksmtuned_running():
Line 97:     try:
Line 98:         logging.debug('checking ksmtuned')
I usually love logging, but 'service ksmtude status' seems good enough to me.
Line 99:         ret = utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'status'], sudo=True)[1][0]
Line 100:         logging.info('ksmtuned: %s', ret)
Line 101:         return ret != "ksmtuned is stopped"
Line 102:     except:


Line 98:         logging.debug('checking ksmtuned')
Line 99:         ret = utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'status'], sudo=True)[1][0]
Line 100:         logging.info('ksmtuned: %s', ret)
Line 101:         return ret != "ksmtuned is stopped"
Line 102:     except:
please try to avoid this catch-all "except:". it is bad practice. (I know running() has it before this patch, but still)
Line 103:         return False
Line 104: 
Line 105: def npages():
Line 106:     try:


Line 108:         return npages
Line 109:     except:
Line 110:         return 0
Line 111: 
Line 112: def ksm_start():
what is the motivation for these name changes? we sit in the ksm module, so why's the prefix needed?

I think execCmd does the logging for you.
Line 113:     if not ksmtuned_running():
Line 114:         utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'start'], sudo=True)
Line 115:     if not ksm_running():
Line 116:         utils.execCmd([constants.EXT_SERVICE, 'ksm', 'start'], sudo=True)


Line 109:     except:
Line 110:         return 0
Line 111: 
Line 112: def ksm_start():
Line 113:     if not ksmtuned_running():
frankly, I believe that these testings are quite annoying and needless. start() is called only once. testing if ksmtuned is running is as hard as actually running it, so why bother.
Line 114:         utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'start'], sudo=True)
Line 115:     if not ksm_running():
Line 116:         utils.execCmd([constants.EXT_SERVICE, 'ksm', 'start'], sudo=True)
Line 117: 


Line 115:     if not ksm_running():
Line 116:         utils.execCmd([constants.EXT_SERVICE, 'ksm', 'start'], sudo=True)
Line 117: 
Line 118: def ksm_stop():
Line 119:     if ksmtuned_running():
here, too, I do not see the need in this test.
Line 120:         utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'stop'], sudo=True)
Line 121:     if ksm_running():
Line 122:         utils.execCmd([constants.EXT_SERVICE, 'ksm', 'stop'], sudo=True)
Line 123: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia50087aae112069b2a427fca12168aff975b443e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Laszlo Hornyak <lhornyak at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck at redhat.com>
Gerrit-Reviewer: Laszlo Hornyak <lhornyak at redhat.com>


More information about the vdsm-patches mailing list