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

dfediuck at redhat.com dfediuck at redhat.com
Thu Oct 4 11:53:49 UTC 2012


Doron Fediuck 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

(2 inline comments)

Hi Laszlo.
Good catch. I think you should improve the service handling by working with the exit codes. 

See inline for the relevant parts.

....................................................
File vdsm/ksm.py
Line 95: 
Line 96: def ksmtuned_running():
Line 97:     try:
Line 98:         logging.debug('checking ksmtuned')
Line 99:         ret = utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'status'], sudo=True)[1][0]
Why not use the standard exit status of the process?

IIRC when a service is running, the exist status is 0. Otherwise you get non-zero.
This should be better than parsing and text comparison.
Line 100:         logging.info('ksmtuned: %s', ret)
Line 101:         return ret != "ksmtuned is stopped"
Line 102:     except:
Line 103:         return False


Line 108:         return npages
Line 109:     except:
Line 110:         return 0
Line 111: 
Line 112: def ksm_start():
Can you please log the results if exit code is not zero?
In this way the logs will say we failed to start the service.

Also, if service start fails, why run the monitor?
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)


--
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: Doron Fediuck <dfediuck at redhat.com>


More information about the vdsm-patches mailing list