Change in vdsm[master]: vdsm-tool: add collectd configurator

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: add collectd configurator
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/78364/1/static/usr/share/vdsm/collectd.virt.conf
File static/usr/share/vdsm/collectd.virt.conf:

Line 12: Connection "qemu:///system"
Line 13: Instances 5
Line 14: ExtraStats "disk pcpu"
Line 15: 
Line 16
this is all? what about enabling the rest of what we need? host-deploy is 
planed to do so?


-- 
To view, visit https://gerrit.ovirt.org/78364
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ac14bc996078d01b986210bd08c94fe93f69ac2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Arik Hadas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Yaniv Kaul 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm-tool: add collectd configurator

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: add collectd configurator
..


Patch Set 1:

(5 comments)

https://gerrit.ovirt.org/#/c/78364/1//COMMIT_MSG
Commit Message:

Line 15: We can't do better than that.
Line 16: 
Line 17: vdsm-tool assumes that the virt plugin is not configured at all
Line 18: (otherwise see above). If the vdsm sampling is disabled, the
Line 19: tool will configure collectd to do the monitoring.
I think you should use the --force - if force is set then we overwrite current 
conf files and remove the old once (no fallback - maybe keep a backup for 
removal). if its without --force you can fail the configure with a request to 
add the configuration manually or use the force flag.. otherwise you can mess 
up not only our reports but also the old configurations the user had..
Line 20: 
Line 21: vdsm-tool will also check the other way around: if vdsm sampling
Line 22: is enabled, it will make sure that collectd is not monitoring the
Line 23: VMs.


https://gerrit.ovirt.org/#/c/78364/1/lib/vdsm/tool/configurators/collectd.py
File lib/vdsm/tool/configurators/collectd.py:

Line 35: from vdsm.common.cmdutils import CommandPath
Line 36: 
Line 37: from . import YES, NO
Line 38: 
Line 39: # TODO: use constants.py
why? its fine to keep it private here.. no other package should use those 
variables
Line 40: _COLLECTD_CONF_CUR = "/etc/collectd.d/virt.conf"
Line 41: _COLLECTD_CONF_VDSM = "/usr/share/vdsm/collectd.virt.conf"
Line 42: _COLLECTD_SERVICE = "collectd.service"
Line 43: 


Line 41: _COLLECTD_CONF_VDSM = "/usr/share/vdsm/collectd.virt.conf"
Line 42: _COLLECTD_SERVICE = "collectd.service"
Line 43: 
Line 44: 
Line 45: _SYSTEMCTL = CommandPath("systemctl", "/bin/systemctl", 
"/usr/bin/systemctl")
you can use the service module if we still have it
Line 46: 
Line 47: # Configuratior interface
Line 48: name = "collectd"
Line 49: services = []


Line 45: _SYSTEMCTL = CommandPath("systemctl", "/bin/systemctl", 
"/usr/bin/systemctl")
Line 46: 
Line 47: # Configuratior interface
Line 48: name = "collectd"
Line 49: services = []
services must be declared? I think by default its empty list
Line 50: 
Line 51: 
Line 52: class NotFound(Exception):
Line 53: """configuration file not found"""


Line 99: _log("collectd configured outside vdsm")
Line 100: return YES
Line 101: except NotFound:
Line 102: _log("collectd requires configuration")
Line 103: return NO
what about removeConf?
Line 104: 
Line 105: 
Line 106: def _collectd_configured():
Line 107: """


-- 
To view, visit https://gerrit.ovirt.org/78364
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ac14bc996078d01b986210bd08c94fe93f69ac2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Arik Hadas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Yaniv Kaul 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org