New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[plugins] New Collectd plugin #866

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@arcolife
Copy link
Contributor

arcolife commented Aug 12, 2016

This plugin gathers config files for collectd on a system.and display active plugins in sos_reports/sos.txt as illustrated below:

collectd
========================================================================
-  files copied:
  * /etc/collectd.d/postgresql.conf
  * /etc/collectd.conf
-  alerts:
  ! Active Plugin found: syslog
  ! Active Plugin found: cpu
  ! Active Plugin found: interface
  ! Active Plugin found: load
  ! Active Plugin found: memory

Signed-off-by: Archit Sharma arcsharm@redhat.com

@bmr-cymru

This comment has been minimized.

Copy link
Member

bmr-cymru commented Aug 12, 2016

  ! Active Plugin found: memory <br>

Plain text reports should not contain HTML markup, where are those <br> coming from?

with open("/etc/collectd.conf") as f:
for line in f:
if p.match(line):
self.add_alert("Active Plugin found: %s <br>\n" %

This comment has been minimized.

@bmr-cymru

bmr-cymru Aug 12, 2016

Member

You can't use HTML markup in add_alert(); the report backend renders plain text as well as HTML reports.

This comment has been minimized.

@bmr-cymru

bmr-cymru Aug 12, 2016

Member

It would be much better if collectd had some command that could be run to obtain the plugin list; we try to avoid parsing configuration as much as possible.

This comment has been minimized.

@arcolife

arcolife Aug 12, 2016

Author Contributor

It doesn't as far as I know. Do you think we should open an issue in collectd to do that instead ?

This comment has been minimized.

@arcolife

arcolife Aug 12, 2016

Author Contributor

I referred to an example. I'll replace this with plain text or refer to other plugins for a better version. Thanks

self.add_copy_spec([
'/etc/collectd.conf',
'/etc/collectd.d/*.conf',
])

This comment has been minimized.

@bmr-cymru

bmr-cymru Aug 12, 2016

Member

Any authentication or other secrets stored in these locations?

This comment has been minimized.

@arcolife

arcolife Aug 12, 2016

Author Contributor

yes there might be credentials to postgresql database in the conf file.

This comment has been minimized.

@bmr-cymru

bmr-cymru Aug 24, 2016

Member

We'll need to get a postproc() method worked out for those then and test with some real world data.

This comment has been minimized.

@arcolife

arcolife Sep 5, 2016

Author Contributor

I've added a new commit that uses postproc() to redact URL, Host, Port, Password, User, Address for different plugins that might be loaded in collectd, per se, PostgreSQL, mysql, modbus, curl_xml, curl_json, ascent, amqp and so on. I've tested this against a current deployment.

Let me know if this is too much obfuscation and if people might require Host/Port addresses that plugins connect to (and leave out just the passwords). Note that Hostname has not been redacted (it's the one that represents the hostname of the machine)

from sos.plugins import Plugin, RedHatPlugin


class Collectd(Plugin, RedHatPlugin):

This comment has been minimized.

@bmr-cymru

bmr-cymru Aug 12, 2016

Member

collectd supports a bunch of other distributions and platforms - since the plugin isn't doing anything fancy with paths or other Red Hat specific configuration its probably worth enabling it for the other distributions that package it.

This comment has been minimized.

@arcolife

arcolife Aug 12, 2016

Author Contributor

yeah I wasn't sure about the paths. Sometimes they differ and so I just included RedHatPlugin for now. Will include all.

for line in f:
if p.match(line):
self.add_alert("Active Plugin found: %s <br>\n" %
line.split()[-1])

This comment has been minimized.

@bmr-cymru

bmr-cymru Aug 12, 2016

Member

Please add a Vim modeline at the bottom of the file as you'll find in other .py files in the tree.

This comment has been minimized.

@arcolife

arcolife Aug 12, 2016

Author Contributor

okay

@bmr-cymru

This comment has been minimized.

Copy link
Member

bmr-cymru commented Aug 12, 2016

Please follow the Contributor Guidelines for commit message formatting.

@arcolife

This comment has been minimized.

Copy link
Contributor Author

arcolife commented Aug 12, 2016

@bmr-cymru sure thing. Will edit that asap

arcolife added a commit to arcolife/sos that referenced this pull request Aug 12, 2016

@arcolife arcolife force-pushed the arcolife:master branch from ae780fa to a82b403 Aug 12, 2016

arcolife added a commit to arcolife/sos that referenced this pull request Aug 12, 2016

add plugin: collectd
fixes as per comments in sosreport#866 - 8273e89

Signed-off-by: Archit Sharma <arcsharm@redhat.com>
@arcolife

This comment has been minimized.

Copy link
Contributor Author

arcolife commented Aug 12, 2016

@portante added a few changes and a plugin for collectd. please review, thanks!

@bmr-cymru

This comment has been minimized.

Copy link
Member

bmr-cymru commented Aug 15, 2016

Thanks! I'm on vacation at the moment but I'll be able to take a closer look later in the week.

profiles = ('services', 'webserver')

files = (
'/etc/collectd.conf',

This comment has been minimized.

@portante

portante Aug 24, 2016

Contributor

Could we use this below instead of repeating the list of files? Can you check other plugins to see how they do it?

This comment has been minimized.

@bmr-cymru

bmr-cymru Aug 24, 2016

Member

Absolutely - there are a few other plugins that do similar things. You can define a variable at class or file scope and use that to avoid repeating yourself with file, package or other lists.

This comment has been minimized.

@arcolife

arcolife Sep 5, 2016

Author Contributor

added a new commit that takes care of this.

@arcolife arcolife force-pushed the arcolife:master branch from a82b403 to 9280ada Sep 5, 2016

arcolife added a commit to arcolife/sos that referenced this pull request Sep 5, 2016

add plugin: collectd
fixes as per comments in sosreport#866 - 8273e89

Signed-off-by: Archit Sharma <arcsharm@redhat.com>

@arcolife arcolife force-pushed the arcolife:master branch from 9280ada to f0115fd Sep 5, 2016

arcolife added a commit to arcolife/sos that referenced this pull request Sep 5, 2016

add plugin: collectd
fixes as per comments in sosreport#866 - 8273e89

Signed-off-by: Archit Sharma <arcsharm@redhat.com>

@arcolife arcolife force-pushed the arcolife:master branch from f0115fd to ae09c3b Sep 5, 2016

arcolife added a commit to arcolife/sos that referenced this pull request Sep 5, 2016

add plugin: collectd
fixes as per comments in sosreport#866 - 8273e89

Signed-off-by: Archit Sharma <arcsharm@redhat.com>

@arcolife arcolife force-pushed the arcolife:master branch from ae09c3b to dc2b825 Sep 6, 2016

arcolife added a commit to arcolife/sos that referenced this pull request Sep 6, 2016

add plugin: collectd
fixes as per comments in sosreport#866 - 8273e89

Signed-off-by: Archit Sharma <arcsharm@redhat.com>

@arcolife arcolife force-pushed the arcolife:master branch from dc2b825 to a12957f Sep 6, 2016

arcolife added a commit to arcolife/sos that referenced this pull request Sep 6, 2016

[collectd] new plugin: collectd
New module for collecting active plugins info & configurations in collectd.

Resolves: sosreport#866

Signed-off-by: Archit Sharma <arcsharm@redhat.com>
@arcolife

This comment has been minimized.

Copy link
Contributor Author

arcolife commented Sep 14, 2016

@bmr-cymru Hey, I had made some changes as per diff comments and updated the comment thread. Is this ready for merge yet?

@bmr-cymru

This comment has been minimized.

Copy link
Member

bmr-cymru commented Sep 15, 2016

@arcolife thanks - will try to take a look in the next week.

@arcolife

This comment has been minimized.

Copy link
Contributor Author

arcolife commented Oct 4, 2016

@bmr-cymru Hi, did you get a chance to go through this? Thanks.

@portante

This comment has been minimized.

Copy link
Contributor

portante commented Oct 17, 2016

Hi Bryn (@bmr-cymru) can you comment on what you think needs to be done for this PR? Thanks!

@bmr-cymru

This comment has been minimized.

Copy link
Member

bmr-cymru commented Oct 18, 2016

@portante afaics it's in good shape - I've been trying to make time to get outstanding PRs merged since last month, but I've been tied up with other work.

The patch as it stands is fine - the eliding of secrets does go a little bit further than typical (it scrubs hostnames and port configuration), but I am fine leaving that as-is - we can always make it less strict in the future if users find it hampers debugging efforts.

@battlemidget

This comment has been minimized.

Copy link
Member

battlemidget commented Oct 18, 2016

ack from me based on #866 (comment), i can merge this up once a second ACK is made

@arcolife

This comment has been minimized.

Copy link
Contributor Author

arcolife commented Oct 18, 2016

@bmr-cymru exactly, I was thinking I should dampen that. I'll change it asap and send you and @battlemidget another ping?

@bmr-cymru

This comment has been minimized.

Copy link
Member

bmr-cymru commented Oct 18, 2016

@arcolife - fine with me, thanks! Generally, unless something seems obviously out of keeping with sos conventions we leave the final decision with plugin authors, so if you agree that port/hostname details don't need to be elided I can get that merged when I receive your updated branch.

[collectd] new plugin: collectd
New module for collecting active plugins info & configurations in collectd.

Resolves: #866

Signed-off-by: Archit Sharma <arcsharm@redhat.com>

@arcolife arcolife force-pushed the arcolife:master branch from a12957f to 93e4432 Oct 24, 2016

@arcolife

This comment has been minimized.

Copy link
Contributor Author

arcolife commented Oct 24, 2016

@bmr-cymru @battlemidget scratched host/port off of protect_keys and added them in a comment instead.

@portante

This comment has been minimized.

Copy link
Contributor

portante commented Dec 20, 2016

@bmr-cymru, seems like this patch is in limbo. Is there a timeline to get it merged?

@bmr-cymru

This comment has been minimized.

Copy link
Member

bmr-cymru commented Dec 20, 2016

Just fell off my list - I'll try to get it in with the next batch (3.4 is still some way off).

@bmr-cymru bmr-cymru closed this in 8e63b8e Jan 17, 2017

@bmr-cymru

This comment has been minimized.

Copy link
Member

bmr-cymru commented Jan 17, 2017

Merged for 3.4, thanks!

TurboTurtle added a commit to TurboTurtle/sos that referenced this pull request Apr 19, 2017

[collectd] new plugin: collectd
New module for collecting active plugins info & configurations in collectd.

Resolves: sosreport#866

Signed-off-by: Archit Sharma <arcsharm@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment