4:27 a.m.
Mark Wu has uploaded a new change for review.
Change subject: [WIP] MOM Integration
......................................................................
[WIP] MOM Integration
This patch tries to integrate MOM with vdsm. For the descprition of
how this patch works, please see Adam's mail:
https://fedorahosted.org/pipermail/vdsm-devel/2012-February/000628.html
Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
---
M vdsm.spec.in
M vdsm/API.py
M vdsm/Makefile.am
M vdsm/clientIF.py
M vdsm/config.py.in
M vdsm/ksm.py
M vdsm/libvirtvm.py
A vdsm/mom.conf
A vdsm/mom.policy
M vdsm/supervdsmServer.py
A vdsm/vdsMOM.py
M vdsm/vm.py
12 files changed, 271 insertions(+), 9 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/67/2367/1
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
8:17 a.m.
Adam Litke has posted comments on this change.
Change subject: [WIP] MOM Integration
......................................................................
Patch Set 1: (4 inline comments)
....................................................
File vdsm/clientIF.py
Line 91: self.ksmMonitor = ksm.KsmMonitorThread(self)
You can combine this if clause into the above if statement.
....................................................
File vdsm/config.py.in
Line 146:
Hmm, I am not sure that we actually need this config option. Eventually we will always
want to use MOM when it is installed so the only safety check we need right now is one
that re-enables the KSM thread when MOM fails to load.
....................................................
File vdsm/mom.conf
Line 29:
You can remove Balloon since the current vdsm policy is not using the Balloon Controller.
....................................................
File vdsm/vdsMOM.py
Line 48:
There has been a recent move to stop using the traceback.format_exc() function in logging
calls. Please change to:
self._cif.log.error("MOM initialization failed", exc_info=True)
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
5:35 a.m.
Doron Fediuck has posted comments on this change.
Change subject: [WIP] MOM Integration
......................................................................
Patch Set 6: (4 inline comments)
Hi Mark,
Looks good. Still some minor issues I commented on inline.
From vdsm's point of view, we may want to consider a better
interface / api for monitoring (possibly also executing)
some factors. In this context see my comment in clientIF.py.
....................................................
File vdsm/clientIF.py
Line 92:
Although this works, I'm troubled from developer's point of view.
This means that on each and every point we had and will have regarding ksm,
the developer needs to know whether to call self.ksmMonitor or self.vdsMOM.
Any thoughts about it?
Looking forward I think it will be better to have an interface (Protocol / API) both ksm
and mom will implement, so the developer may call the same thing all over the code.
Does this make sense?
....................................................
File vdsm.spec.in
Line 351: cp %{_sysconfdir}/%{vdsm_name}/mom.conf %{_sysconfdir}/mom.conf
Mark,
Can you please explain this part;
Usually, rpm handles configuration files quite well, so no real need
for such intervention.
If the existing mom.conf belongs to someone else (mom rpm?),
then it's considered a bad practice to overrun it in this way. ie- one
rpm should not mv / rm another rpm's files.
So if I understand correctly, and you're running over the original
mom's conf, I suggest do one of the following:
1. Change mom's default conf file name to mom.conf.samle (or template),
and then it's perfectly fine to cp or generate vdsm conf file for mom.
2. If vdsm creates its own mom-vdsm.conf, mom should be able to
be started with a flag specifying the relevant conf file.
....................................................
File vdsm/supervdsmServer.py
Line 192:
There's a trailing white tab here, which git really hates...
....................................................
File vdsm/vdsMOM.py
Line 35: self.MOM = mom.MOM("/etc/vdsm/mom.conf")
This should probably be taken from vdsm.conf file, thus improving
a previous packaging issue I commented on.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <sming56(a)gmail.com>
5:23 p.m.
New subject: Change in vdsm[master]: MOM Integration
Dan Kenigsberg has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 7: I would prefer that you didn't submit this
(14 inline comments)
Thanks for pursuing this integration, particularly when ksmtune usage for Fedora is
broken.
Could you think of a unittest for this new feature?
....................................................
File vdsm/clientIF.py
Line 85: # If MOM is not available, we need failback to KsmMonitorThread.
s/failback/to fall back/
....................................................
File vdsm/ksm.py
Line 102: def tune(params):
I'm not very fond of basket args like this "params". why haven't you
listed the 3 valid args in the function's signature?
Line 103: allowed = set(('ksm_run', 'ksm_sleep_millisecs',
'ksm_pages_to_scan'))
ALLOWED is a constant
Line 104: if set(params.keys()) > allowed:
smells like something you should
assert
Line 106: for (k, v) in params.items():
not really important for such a short dict, but iteritems() is quicker.
Line 109: f.write(str(v))
use 'with' for auto-close on exception.
Line 111: except IOError:
is this expected? if not, log.error().
Line 113: return True
it is more pythonic to return None on success, and raise an exception on failure.
....................................................
File vdsm/supervdsmServer.py
Line 190: def ksmTune(self, tuningParams):
where is this new verb used? in a future patch? if it's via a hidden path from the MOM
object, I could really use some commenting.
....................................................
File vdsm/vdsMOM.py
Line 25: def __init__(self, cif):
Saggi would tell you that the single configurable of this function should be an arg to
this constructor.
Line 33: # it must be initialized by calling setConnection() with the vdsm cif
object.
please keep lines short and add new modules to PEP8WHITELIST
Line 34: vdsmInterface.setConnection(self._cif)
_cif is not the proper API to interact with Vdsm. API.py is.
Please change MOM to conform to this, if possible.
Line 37: self.MOM = mom.MOM(conf)
self.MOM is not a class nor a public constant. it should probably be called self._mom.
Line 40: self.running = True
is there any reason to return an object if this has failed? it seems cleaner to avoid
self.running and raise the exception as-is.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
10:19 a.m.
Adam Litke has posted comments on this change.
Change subject: [WIP] MOM Integration
......................................................................
Patch Set 3: (1 inline comment)
....................................................
File vdsm/vdsMOM.py
Line 22: import traceback
You do not need to import traceback anymore.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: shu ming <sming56(a)gmail.com>
4:38 a.m.
New subject: Change in vdsm[master]: MOM Integration
Dan Kenigsberg has submitted this change and it was merged.
Change subject: MOM Integration
......................................................................
MOM Integration
This patch integrates MOM into vdsm. For the descprition of
how it works, please see Adam's mail:
https://fedorahosted.org/pipermail/vdsm-devel/2012-February/000628.html
Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Signed-off-by: Mark Wu <wudxw(a)linux.vnet.ibm.com>
---
M .gitignore
M Makefile.am
M vdsm.spec.in
M vdsm/API.py
M vdsm/Makefile.am
M vdsm/clientIF.py
M vdsm/config.py.in
M vdsm/ksm.py
M vdsm/libvirtvm.py
A vdsm/mom.conf.in
A vdsm/mom.policy
A vdsm/momIF.py
M vdsm/supervdsmServer.py
M vdsm/vm.py
14 files changed, 280 insertions(+), 23 deletions(-)
Approvals:
Mark Wu: Verified
Doron Fediuck: Looks good to me, but someone else must approve
Dan Kenigsberg: Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
4:38 a.m.
New subject: Change in vdsm[master]: MOM Integration
Dan Kenigsberg has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 17:
Congrats
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
3:56 a.m.
New subject: Change in vdsm[master]: MOM Integration
Mark Wu has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 17: Verified
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
5:38 a.m.
New subject: Change in vdsm[master]: MOM Integration
Dan Kenigsberg has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 17: Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
5:11 p.m.
New subject: Change in vdsm[master]: MOM Integration
Doron Fediuck has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 17: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
3:12 p.m.
New subject: Change in vdsm[master]: MOM Integration
Dan Kenigsberg has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 15: Looks good to me, but someone else must approve
(2 inline comments)
this patch has waited long enough.
....................................................
File vdsm/ksm.py
Line 109: KSM_PARAMS = {'run': 3, 'sleep_millisecs': 0x100000000,
I still do not think KSM_PARAMS should be public.
Line 114: if k not in KSM_PARAMS.keys():
use iterkeys() when there is no reason to create a new list.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
1:58 a.m.
New subject: Change in vdsm[master]: MOM Integration
Mark Wu has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 14: (1 inline comment)
....................................................
File vdsm/clientIF.py
Line 112: if self.mom:
Yes, you're right. I will fix it in next patch. Thanks for the review!
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
1:55 a.m.
New subject: Change in vdsm[master]: MOM Integration
Xu He Jie has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 14: I would prefer that you didn't submit this
(1 inline comment)
minor issue
....................................................
File vdsm/clientIF.py
Line 112: if self.mom:
if any exception raised before _prepareMON, self.mom will be non-exist. this will overlap
the real exception
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
10:05 p.m.
New subject: Change in vdsm[master]: MOM Integration
Mark Wu has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 14:
Ayal,
All problems you pointed out has been fixed in the new patch.
Many thanks for the review!
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
2:22 p.m.
New subject: Change in vdsm[master]: MOM Integration
Ayal Baron has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 13: I would prefer that you didn't submit this
(5 inline comments)
Minor issues
....................................................
File vdsm/ksm.py
Line 114: raise Exception('Invalid KSM tuning parameter: %s=%s' % (k,
v))
you could also make sure that values are proper (run in [0,1,2] etc)
....................................................
File vdsm/libvirtvm.py
Line 2067: if self.cif.ksmMonitor:
for symmetry perhaps this should be if not self.mom
....................................................
File vdsm/vm.py
Line 572: and self.cif.ksmMonitor:
again, perhaps not self.mom
Line 574: # behaviors on VM start/destroy. Because the tuning can be
s/. B/, b/
Line 575: # done automatically acccording its statistics data.
s/its statistics/to its statistical/
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
7:48 a.m.
New subject: Change in vdsm[master]: MOM Integration
Mark Wu has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 12:
Other questions except the file name(MOM.py) has been answered.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
9:33 p.m.
New subject: Change in vdsm[master]: MOM Integration
Mark Wu has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 11: (2 inline comments)
....................................................
File vdsm/MOM.py
Line 30: class momThread(threading.Thread):
we can't use the name mom.py, because it's the name of the python module MOM
provides. If using mom.py, it will fail to import mom
/usr/lib/python2.7/site-packages/mom
The original file name is vdsmMOM.py or vdsMOM.py. As you mentioned before, the prefix is
useless because it's in vdsm project. I didn't figure out a better name than
MOM.py. If you have any suggestion, I will change it. Thanks!
Line 44: ret['ksmState'] = stats['ksm_run']
Thanks for pointing out it!
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
3:36 p.m.
New subject: Change in vdsm[master]: MOM Integration
Dan Kenigsberg has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 11: (1 inline comment)
....................................................
File vdsm/MOM.py
Line 44: ret['ksmState'] = stats['ksm_run']
I suspect that a subtle change if api is hidden here: ksmMonitor returns True/False for
ksmState.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
3:14 p.m.
New subject: Change in vdsm[master]: MOM Integration
Dan Kenigsberg has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 11: I would prefer that you didn't submit this
(2 inline comments)
thanks, Mark, for the update. I have very few stylistic comments.
....................................................
File vdsm/ksm.py
Line 109: KSM_PARAMS = ('run', 'sleep_millisecs',
'pages_to_scan')
do you envisage someone outside this module using this tupple? if not, keep _PRIVATE, or
in-function.
....................................................
File vdsm/MOM.py
Line 30: class momThread(threading.Thread):
style: class names should begin with Upper case (yeah, we have ugly baggage not
conforming, but let's improve!)
the name of the module, though, would better be lower-case mom.py (I think).
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
9:42 p.m.
New subject: Change in vdsm[master]: MOM Integration
Dan Kenigsberg has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 10: I would prefer that you didn't submit this
(4 inline comments)
....................................................
File vdsm/constants.py.in
Line 165: #
is this really needed to be exposed in the global constants.py? I'd rather hide it in
ksm.py.
....................................................
File vdsm/vdsmMOM.py
Line 22: from vdsm.config import config
due to the filename change it is a bit hard to check if all my questions were answered.
at least one, has not - this module should not have direct access to vdsm.config.config.
VdsmMOM can take momconf as an arg.
that's better for testability and good relations with Saggi.
Line 30: class VdsmMOM(threading.Thread):
I do not have good suggestions, but I'm not happy with the name. We all know it is a
part of the vdsm project, so the Vdsm prefix does not really help.
Line 32: def __init__(self, log):
I think the signature would be prettier if "log" is dropped, and replaced with a
logging.getLogger() call.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
4:29 a.m.
New subject: Change in vdsm[master]: MOM Integration
Dan Kenigsberg has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 7:
How about moving MOM development to gerrit.ovirt.org? ;-)
I think that mom should receive an API object (even API.Global()!) that it needs for its
operation. Even passing BindingXMLRPC() object is more APIish than the internal clientIF
object.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
2:30 a.m.
New subject: Change in vdsm[master]: MOM Integration
Mark Wu has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 7:
Hi Dan,
Thanks a lot for the review. I am going to revise the patch as per your comments. And the
unittest will be included in next version too. The only problem needing more discussions
is that how mom interacts with vdsm. Currently, vdsm passes the vdsm internal instance of
clientIF to mom to enable mom call vdsm API functions defined in API.py. Let's think
MOM is kind of plugin for vdsm, then it would make sense that passing cif to mom. Without
cif, mom would have to interact with vdsm via xmlrpc. In that case, mom works like a
standalone application on system. So which idea do you like more? Any other suggestions?
Thanks!
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
8:58 a.m.
New subject: Change in vdsm[master]: MOM Integration
Doron Fediuck has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 7: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
4:38 a.m.
New subject: Change in vdsm[master]: MOM Integration
Mark Wu has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 9:
Adam,
The MOM patch is already done in my local repo. I wrote it to test this patch. I will
submit later.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
8:42 p.m.
New subject: Change in vdsm[master]: MOM Integration
Xu He Jie has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 9: (2 inline comments)
....................................................
File vdsm/Makefile.am
Line 241: $(DESTDIR)$(vdsmconfdir)/mom.policy
use tab instead of space
Line 243: $(DESTDIR)$(vdsmconfdir)/mom.conf
use tab instead of space
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
2:59 p.m.
New subject: Change in vdsm[master]: MOM Integration
Adam Litke has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 9:
One additional comment. You will need to submit a patch to MOM to change the
initialization sequence since you no longer need to receive a reference to clientIF.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
2:57 p.m.
New subject: Change in vdsm[master]: MOM Integration
Adam Litke has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 9: I would prefer that you didn't submit this
(10 inline comments)
....................................................
File Makefile.am
Line 56: vdsm_cli/vdsClient.py \
Please help keep these sorted.
Line 60:
No need to add whitespace here.
....................................................
File vdsm/clientIF.py
Line 70: self.memOvercommit = None
This variable name should make it more obvious what it is. How about naming it
MOMController ?
....................................................
File vdsm/ksm.py
Line 28: class KsmMonitorThread(momBase):
I am not sure I like having this thread inherit from a MomBase class. The
KsmMonitorThread is on its way to deprecation so we should leave this class as-is. I
would rather see if statements in the few areas of the code that check if MOM is
initialized and fall back to ksmThread as necessary.
....................................................
File vdsm/libvirtvm.py
Line 2050: self.cif.memOvercommit.adjust()
I don't understand this adjust call. It seems to be a no-op.
....................................................
File vdsm/mom.conf
Line 45: policy: /etc/vdsm/mom.policy
Maybe build this value using autoconf and use the @confdir@ variable.
Line 51: log: /var/log/vdsm/mom.log
Same as above
....................................................
File vdsm/vdsMOM.py
Line 25: class momBase(threading.Thread):
This is not a great abstraction in my opinion. The KsmMonitorThread really has nothing to
do with MOM so I would prefer that we don't try to merge the two things together. Is
there any other reason to have this class around?
....................................................
File vdsm/vm.py
Line 571: if self.lastStatus != 'Don' and 'recover' not in
self.conf:
accidental bug addition: s/Don/Down/
Line 572: self.cif.memOvercommit.adjust()
Just add an if statement here to see if you should use MOM or KsmMonitorThread.
Basically, if ksmMonitor is not None, call its adjust method.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
11:41 a.m.
New subject: Change in vdsm[master]: MOM Integration
Xu He Jie has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 9: I would prefer that you didn't submit this
(5 inline comments)
....................................................
File vdsm/ksm.py
Line 30: momBase.__init__(self, log, self.run, 'KsmMonitor')
I know this work. I guess only use one way, overriding run() method or set target, will be
more clean
Line 39: self.start()
It looks can't start thread at base class. But move start out of __init__, and just
overriding start() method will be clean. is right?
....................................................
File vdsm/vdsMOM.py
Line 25: class momBase(threading.Thread):
the name should be MomBase
Line 45: class vdsMOM(momBase):
same with above
Line 48: import mom
why not import mom at top of file
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
7:51 a.m.
New subject: Change in vdsm[master]: MOM Integration
Adam Litke has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 7: I would prefer that you didn't submit this
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
7:51 a.m.
New subject: Change in vdsm[master]: MOM Integration
Adam Litke has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 7:
I would also like to request that a set of tests be added to the functional test suite to
check the MOM integration code.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
12:50 a.m.
New subject: Change in vdsm[master]: MOM Integration
Mark Wu has posted comments on this change.
Change subject: MOM Integration
......................................................................
Patch Set 7:
Change from patch set 6:
- fix file list sorting problem in Makefile.am and vdsm.spec.in according to Ayal's
comments
- fix mom.conf packaging issue according to Doron's comments
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
3:56 a.m.
Doron Fediuck has posted comments on this change.
Change subject: [WIP] MOM Integration
......................................................................
Patch Set 6: (1 inline comment)
....................................................
File vdsm/clientIF.py
Line 92:
AFAIR, MOM is Memory Overcommitment Manager, and the policy rules will come from
ovirt-engine.
My initial comment was from the dev's point of view, who (after this change) will need
to check whether ksm or mom is in use. Going forward a single entity in vdsm side will
make it simpler, and we can later decide if it represents only MOM or something more
generic.
If you want to continue discussing it, I suggest move it to some mailing list, although I
already said we can leave this for now and revisit in next generation.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
5:26 p.m.
Ayal Baron has posted comments on this change.
Change subject: [WIP] MOM Integration
......................................................................
Patch Set 6: (7 inline comments)
Note I have not reviewed the patch, just replying to Doron's comment so I cannot rate
the patch yet.
....................................................
File vdsm/clientIF.py
Line 92:
Mom is a policy engine, it doesn't mandate a specific policy so if the policy changes
in the future, we need not change the API (or the code at all) just the policy passed to
mom, so I'm not sure what you're suggesting.
....................................................
File vdsm/Makefile.am
Line 40: vdsMOM.py \
Again, please keep alphabetical sort.
Line 134: mom.conf
and again
Line 156: install-data-libvirtpass \
would be nice to sort this list
Line 174: uninstall-data-libvirtpass \
same here
....................................................
File vdsm.spec.in
Line 493: %{_sysconfdir}/%{vdsm_name}/mom.policy
please sort alphabetically
Line 578: %{_datadir}/%{vdsm_name}/vdsMOM.py*
same here
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
9:48 a.m.
Doron Fediuck has posted comments on this change.
Change subject: [WIP] MOM Integration
......................................................................
Patch Set 6: (1 inline comment)
....................................................
File vdsm/clientIF.py
Line 92:
Adam, the interface I was referring to is a more generic one, than a specific MOM policy.
Yet I'm willing to put this aside for now, and revisit after the current generation.
Depricating ksmMonitorThread should be more than just a cleanup, and I'll be waiting
there.
So for the time being, this can remain as Mark wrote it.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <sming56(a)gmail.com>
9:35 a.m.
Adam Litke has posted comments on this change.
Change subject: [WIP] MOM Integration
......................................................................
Patch Set 6: (1 inline comment)
....................................................
File vdsm/clientIF.py
Line 92:
The interface is MOM and its policy. Note that MOM needs to perform all monitoring and
tuning via vdsm APIs in this configuration so vdsm will have internal mechanisms for doing
everything that mom currently is doing.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <sming56(a)gmail.com>
10:10 a.m.
Doron Fediuck has posted comments on this change.
Change subject: [WIP] MOM Integration
......................................................................
Patch Set 6: (1 inline comment)
....................................................
File vdsm/clientIF.py
Line 92:
Hi Adam,
Unsure how good or bad KSM integration is. It may be nice to have it as a fallback,
unless Dan / Ayal think otherwise.
The main thing I see here, is a need to keep it unified. Even if you decide to upgrade mom
in the future into turbo-mom, we'd still need to keep some sort of an interface,
allowing developers to use a single monitoring/perf-tuning entity.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <sming56(a)gmail.com>
8:52 a.m.
Adam Litke has posted comments on this change.
Change subject: [WIP] MOM Integration
......................................................................
Patch Set 6: (2 inline comments)
....................................................
File vdsm/clientIF.py
Line 92:
Hi Doron. We are looking to deprecate the ksmMonitorThread but would like to preserve
compatibility for one short generation in case a distro is unable to ship MOM right away.
But after this one generation, all ksm operations will be routed through MOM.
....................................................
File vdsm.spec.in
Line 351: cp %{_sysconfdir}/%{vdsm_name}/mom.conf %{_sysconfdir}/mom.conf
Mark. MOM allows you to pass in a configuration file in its constructor function. So,
just install the mom.conf into the vdsm conf directory and use that path when initializing
MOM.
--
To view, visit http://gerrit.ovirt.org/2367
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61e68f72e9115a913d5bc0f4903b906b0d0cce2f
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <sming56(a)gmail.com>
4274
days inactive
4434
days old
vdsm-patches@lists.fedorahosted.org
36 comments
6 participants
participants (6)
-
abaron＠redhat.com -
agl＠us.ibm.com -
Dan Kenigsberg -
dfediuck＠redhat.com -
wudxw＠linux.vnet.ibm.com -
Xu He Jie