Laszlo Hornyak has uploaded a new change for review.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
start ksm and ksmtuned when vdsm starts
this patch - separates the check against ksm and ksmtuned - invokes the functions to check and start ksm and ksmtuned from the monitor thread
Change-Id: Ia50087aae112069b2a427fca12168aff975b443e Bug-Url: https://bugzilla.redhat.com/854027 Signed-off-by: Laszlo Hornyak lhornyak@redhat.com --- M vdsm/ksm.py 1 file changed, 23 insertions(+), 6 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/57/8357/1
diff --git a/vdsm/ksm.py b/vdsm/ksm.py index f93007a..ba4166e 100644 --- a/vdsm/ksm.py +++ b/vdsm/ksm.py @@ -20,6 +20,7 @@
import threading, time import os +import logging from vdsm import constants from vdsm import utils from vdsm.config import config @@ -36,9 +37,12 @@ raw=False, sudo=False)[1] if pids: self._pid = pids[0].strip() + self._cif.log.info('starting ksm monitor thread, ksm pid is %s', self._pid) self.start() else: self._cif.log.error('failed to find ksmd thread') + else: + self._cif.log.info('ksm monitor thread disabled, not starting') self.cpuUsage = 0
def _getKsmdJiffies(self): @@ -46,6 +50,7 @@ .read().split()[13:15]))
def run(self): + ksm_start() try: self.state, self.pages = self.readState() KSM_MONITOR_INTERVAL = 60 @@ -58,9 +63,10 @@ jiff0 = jiff1 except: self._cif.log.error("Error monitoring KSM", exc_info=True) + ksm_stop()
def readState(self): - return running(), npages() + return ksm_running(), npages()
def adjust(self): """adjust ksm state according to configuration and current memory stress @@ -80,10 +86,19 @@ except: return 0
-def running(): +def ksm_running(): try: state = int(file('/sys/kernel/mm/ksm/run').read()) & 1 == 1 return state + except: + return False + +def ksmtuned_running(): + try: + logging.debug('checking ksmtuned') + ret = utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'status'], sudo=True)[1][0] + logging.info('ksmtuned: %s', ret) + return ret != "ksmtuned is stopped" except: return False
@@ -94,14 +109,16 @@ except: return 0
-def start(): - if not running(): +def ksm_start(): + if not ksmtuned_running(): utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'start'], sudo=True) + if not ksm_running(): utils.execCmd([constants.EXT_SERVICE, 'ksm', 'start'], sudo=True)
-def stop(): - if running(): +def ksm_stop(): + if ksmtuned_running(): utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'stop'], sudo=True) + if ksm_running(): utils.execCmd([constants.EXT_SERVICE, 'ksm', 'stop'], sudo=True)
-- To view, visit http://gerrit.ovirt.org/8357 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: Ia50087aae112069b2a427fca12168aff975b443e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com
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@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com
Laszlo Hornyak has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 1: (2 inline comments)
.................................................... 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] agreed. waiting for Dan to decide what solution he wants, if we agree in the way this bug should be resolved, then I come back here and fix this. 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(): ok! 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@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com
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@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com
Laszlo Hornyak has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 1: (7 inline comments)
.................................................... 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) strangely, it passed the compilation. is there a fubar with pep8? 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() Ok 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') Done 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 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] Done Line 100: logging.info('ksmtuned: %s', ret) Line 101: return ret != "ksmtuned is stopped" Line 102: except: Line 103: return False
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: What do you want to see here? 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(): since ksm and ksmtuned are two different services 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 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(): So what you want is to try start even if it is running already? 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@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com
Laszlo Hornyak has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 1: (1 inline comment)
.................................................... File vdsm/ksm.py Line 109: except: Line 110: return 0 Line 111: Line 112: def ksm_start(): Line 113: if not ksmtuned_running(): OK. 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:
-- 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@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com
Laszlo Hornyak has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 2: (4 inline comments)
.................................................... File vdsm/ksm.py Line 85 Line 86 Line 87 Line 88 Line 89 so as we discussed, tests were deleted
Line 92: return npages Line 93: except: Line 94: return 0 Line 95: Line 96: def ksm_start(): and it was renamed to ksm_start since the start() already caused confusion (this is not a method of the thread class) Line 97: utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'start'], sudo=True) Line 98: utils.execCmd([constants.EXT_SERVICE, 'ksm', 'start'], sudo=True) Line 99: Line 100: def tune(params):
Line 94 Line 95 Line 96 Line 97 Line 98 no more checking if the service is up or not
Line 98 Line 99 Line 100 Line 101 Line 102 this method is gone since no longer want to stop the services
-- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 2: I would prefer that you didn't submit this
(1 inline comment)
Please explain why the current suggestion is better than starting ksm/ksmtuned in vdsmd.init.in, or in vdsmd.service.
The only difference I see is if ksm/ksmtuned has a bug and won't start. But that a hypothetical bug, not something configureable.
.................................................... File vdsm/ksm.py Line 1: # Line 2: # Copyright 2011 Red Hat, Inc. please update the year. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 6: # the Free Software Foundation; either version 2 of the License, or
-- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com
Shu Ming has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 2: I would prefer that you didn't submit this
(2 inline comments)
Got two comments in line. For Dan, does vdsmd.init or vdsmd.server start ksmtuned/ksm service now?
.................................................... File vdsm/ksm.py Line 65: except: Line 66: self._cif.log.error("Error monitoring KSM", exc_info=True) Line 67: Line 68: def readState(self): Line 69: return ksm_running(), npages() It seems that ksm_running() was not defined after running() was removed. Line 70: Line 71: def adjust(self): Line 72: """adjust ksm state according to configuration and current memory stress Line 73: return whether ksm is running"""
Line 92: return npages Line 93: except: Line 94: return 0 Line 95: Line 96: def ksm_start(): Not sure why you removed the running() check here. In some linux variant node, the service may already be started by administrators. restarting them may cause problem. Line 97: utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'start'], sudo=True) Line 98: utils.execCmd([constants.EXT_SERVICE, 'ksm', 'start'], sudo=True) Line 99: Line 100: def tune(params):
-- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Doron Fediuck has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 2: (1 inline comment)
Dan,
this is better since if configuration does not want ksm, we will not start it. So for now we maintain existing behavior and allow muting ksm when needed.
.................................................... File vdsm/ksm.py Line 92: return npages Line 93: except: Line 94: return 0 Line 95: Line 96: def ksm_start(): Shu Ming,
calling 'start' for a running service does nothing if the service is already running. So this is the desired behavior;
if the service is running (auto-started in the init phase), nothing happens.
Else, it should start now. Line 97: utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'start'], sudo=True) Line 98: utils.execCmd([constants.EXT_SERVICE, 'ksm', 'start'], sudo=True) Line 99: Line 100: def tune(params):
-- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Doron Fediuck has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 2:
One more thing, ksmtuned conflicts with mom, so we should leave it configurable rather than requiring it.
-- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Doron Fediuck has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 3:
Nothing changed from previous patch?
-- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Saggi Mizrahi has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 3: (3 inline comments)
.................................................... File vdsm/ksm.py Line 65: except: Line 66: self._cif.log.error("Error monitoring KSM", exc_info=True) Line 67: Line 68: def readState(self): Line 69: return ksm_running(), npages() Where is ksm_running declared? Line 70: Line 71: def adjust(self): Line 72: """adjust ksm state according to configuration and current memory stress Line 73: return whether ksm is running"""
Line 85: return (int(file('/sys/kernel/mm/ksm/pages_sharing').read())) Line 86: except: Line 87: return 0 Line 88: Line 89: def npages(): If we are in the mood for renaming. This could be pagesToScan(). Line 90: try: Line 91: npages = int(file('/sys/kernel/mm/ksm/pages_to_scan').read()) Line 92: return npages Line 93: except:
Line 92: return npages Line 93: except: Line 94: return 0 Line 95: Line 96: def ksm_start(): No need to change to ksm_start as the module name is sufficient.
You could change to startDaemon() if you want to specify what you are starting.
In any case we use mixedCase in VDSM Line 97: utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'start'], sudo=True) Line 98: utils.execCmd([constants.EXT_SERVICE, 'ksm', 'start'], sudo=True) Line 99: Line 100: def tune(params):
-- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Saggi Mizrahi has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 3: I would prefer that you didn't submit this
-- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Dan Kenigsberg has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 3: I would prefer that you didn't submit this
(1 inline comment)
Shu Ming: currently ksm/ksmtuned are not started nor explicitly required by vdsmd. Doron would like to keep it this way, in order to be able to switch from them to MOM according to a configurable. I tend to accept his argument.
.................................................... File vdsm/ksm.py Line 1: # Line 2: # Copyright 2011 Red Hat, Inc. do you intentionally ignore my stupid first comment? don't you love lawyers like everybody? Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 6: # the Free Software Foundation; either version 2 of the License, or
-- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Mark Wu has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 3: I would prefer that you didn't submit this
The commit messages need an update because we just fix bz854027 and doesn't separate ksm and ksmtuned, which should be done in another patch to resolve the conflict between ksmtuned and MOM.
-- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Noam Slomianko has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 4: (1 inline comment)
.................................................... File vdsm/ksm.py Line 84: try: Line 85: return (int(file('/sys/kernel/mm/ksm/pages_sharing').read())) Line 86: except: Line 87: return 0 Line 88: Oh silly diff....
renamed as suggested also renamed running() to match new style Line 89: def pagesToScan(): Line 90: try: Line 91: pagesToScan = int(file('/sys/kernel/mm/ksm/pages_to_scan').read()) Line 92: return pagesToScan
-- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Noam Slomianko nslomian@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Laszlo Hornyak has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 5: Verified
-- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Noam Slomianko nslomian@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Dan Kenigsberg has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 5: (1 inline comment)
.................................................... File vdsm/ksm.py Line 95: def start(): Line 96: utils.execCmd([constants.EXT_SERVICE, 'ksmtuned', 'start'], sudo=True) Line 97: utils.execCmd([constants.EXT_SERVICE, 'ksm', 'start'], sudo=True) Line 98: Line 99: def running(): only do I care about introducing needless changes? pushing a quick git-log beautification. Line 100: try: Line 101: state = int(file('/sys/kernel/mm/ksm/run').read()) & 1 == 1 Line 102: return state Line 103: except:
-- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Noam Slomianko nslomian@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Dan Kenigsberg has posted comments on this change.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
Patch Set 6: Verified; Looks good to me, approved
-- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Noam Slomianko nslomian@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Dan Kenigsberg has submitted this change and it was merged.
Change subject: start ksm and ksmtuned when vdsm starts ......................................................................
start ksm and ksmtuned when vdsm starts
Change-Id: Ia50087aae112069b2a427fca12168aff975b443e Bug-Url: https://bugzilla.redhat.com/854027 Signed-off-by: Laszlo Hornyak lhornyak@redhat.com --- M vdsm/ksm.py 1 file changed, 8 insertions(+), 10 deletions(-)
Approvals: Dan Kenigsberg: Verified; Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/8357 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: Ia50087aae112069b2a427fca12168aff975b443e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Doron Fediuck dfediuck@redhat.com Gerrit-Reviewer: Laszlo Hornyak lhornyak@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Noam Slomianko nslomian@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
vdsm-patches@lists.fedorahosted.org