8000 New plugin to read RAID events by mfijalko · Pull Request #2841 · collectd/collectd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

New plugin to read RAID events #2841

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 9, 2020
Merged

New plugin to read RAID events #2841

merged 3 commits into from
Apr 9, 2020

Conversation

mfijalko
Copy link
@mfijalko mfijalko commented Jun 28, 2018 •
edited by kkepka
Loading

This plugin, named mdevents, is responsible for gathering the events from RAID arrays that were written to syslog by mdadm utility (which is a user-space software for managing the RAIDs). Then, based on configuration provided by user, plugin will decide whether to send the collectd notification or not.

Mdevents needs the syslog and mdadm to be present on a platform that collectd is launched.

Based on the naming of existing dpdk plugins (dpdkstat for metrics, dpdkevents for notifications) and the fact that there is already plugin called md for gathering metrics from RAIDs, giving this plugin name mdevents felt like the best option.

A slight change to ignorelist API was introduced - it is now possible to retrieve the count of nodes in array and the current ignore setting for given array.

ChangeLog: mdevents plugin: Generate notifications about events in RAID arrays.

[edit @kkepka:] added changelog

@mfijalko mfijalko changed the title Add mdevents plugin New plugin to read RAID events Jun 28, 2018
kwiatrox
kwiatrox reviewed Jul 18, 2018
View reviewed changes
Copy link
Member
@kwiatrox kwiatrox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me the plugin looks good, I have added only minor comments.

kwiatrox
kwiatrox previously approved these changes Jul 30, 2018
View reviewed changes
@mfijalko
Copy link
Author
mfijalko commented Aug 8, 2018

@rubenk @octo @tokkee any chances that you guys would take a look at this and decide whether it would be useful for collectd project?

@octo octo added the New plugin label Oct 6, 2018
rpv-tomsk
rpv-tomsk suggested changes Oct 18, 2018
View reviewed changes
@rpv-tomsk
Copy link
Contributor

This plugin, named mdevents, is responsible for gathering the events from RAID arrays that were written to syslog by mdadm utility (which is a user-space software for managing the RAIDs). Then, based on configuration provided by user, plugin will decide whether to send the collectd notification or not.

Mdevents needs the syslog and mdadm to be present on a platform that collectd is launched.

What do you think about implementation, which will not require syslog in the chain?
Unfortunately, I found no such solution. Maybe we can ask mdadm developers to start implementing the library for such purposes?

@rpv-tomsk rpv-tomsk added this to the 5.9 milestone Oct 18, 2018
@octo octo modified the milestones: 5.9, Features Oct 18, 2018
@mfijalko
Copy link
Author

What do you think about implementation, which will not require syslog in the chain?
Unfortunately, I found no such solution. Maybe we can ask mdadm developers to start implementing the library for such purposes?

Hi! First of all, sorry for that prolonged silence. Thanks for taking a look at this. I'm going to post a patch that addresses your comments.

Answering your question - from what I see, mdadm doesn't look like it is "alive", their github is silent for more or less half a year. But you know, we can give it a shot and post a feature request.

On the other hand, does having a syslog dependency is that painful?

Thanks again and looking forward for your response!

@mrunge mrunge modified the milestones: 5.9, 5.10 Jul 10, 2019
@mrunge mrunge modified the milestones: 5.10.0, 5.11.0 Oct 15, 2019
@mkobyli
This plugin, named mdevents, is responsible for gathering the events …
ccf4cd8 
…from RAID arrays that were written to syslog by mdadm utility (which is a user-space software for managing the RAIDs). Then, based on configuration provided by user, plugin will decide whether to send the collectd notification or not.

Mdevents needs the syslog and mdadm to be present on a platform that collectd is launched.

Based on the naming of existing dpdk plugins (dpdkstat for metrics, dpdkevents for notifications) and the fact that there is already plugin called md for gathering metrics from RAIDs, giving this plugin name mdevents felt like the best option.

A slight change to ignorelist API was introduced - it is now possible to retrieve the count of nodes in array and the current ignore setting for given array.

ChangeLog: New plugin to read RAID events
@rpv-tomsk
Copy link
Contributor
rpv-tomsk commented Jan 14, 2020 •
edited
Loading

This PR is something similar to or related to #3045

@rpv-tomsk
Copy link
Contributor

On the other hand, does having a syslog dependency is that painful?

The longer chain - lower reliability.

@kkepka
Copy link
Contributor
kkepka commented Jan 15, 2020

This PR is something similar to or related to #3045

You are right @rpv-tomsk, functionally both plugins are watching files for particular messages to generate notification about.
The difference is that mdevents is to be just enabled and ready to go, while in logparser case user needs to provide definition of what want to monitor (via proper configuration). So it's trade of easy-to-use vs flexibility and being error prone on configuration side.

@dago
Copy link
Contributor
dago commented Feb 29, 2020

@mfijalko There is a failing test, would you mind having a look?

@dago dago added the Waiting for response - 1st time Waiting for contributor to respond - 1st call label Feb 29, 2020
@dago dago modified the milestones: 5.11.0, 5.12.0 Mar 2, 2020
@kkepka
Copy link
Contributor
kkepka commented Mar 2, 2020 •
edited
Loading

Red Hat check have more verbose UT logs.
From there we can see, that UT fails for init unit test when there is no /var/log/syslog or /var/log/messages available on the system, what is the case in ci env, so i'd say it's more unitests implementation / test env issue, rather then plugin itself.
I don't want to create any debt here, but as ci environment doesn't have dependency for this unit test case, would it make sense to remove it? or add syslog in ci containers.

mkobyli added 2 commits March 3, 2020 15:36
@mkobyli
quick fix for UT fails
d32c8e2 
At the moment collectd ci environment doesn't have dependency for removed unit test case.
@mkobyli
removed profiling options from unit tests
76a4d49
@sunkuranganath sunkuranganath removed the Waiting for response - 1st time Waiting for contributor to respond - 1st call label Mar 3, 2020
@sunkuranganath
Copy link
Member

Looks like all the changes requested has been resolved along with any of the UT/CI issues. Could we remove "changes requested"?

@dago
Copy link
Contributor
dago commented Mar 3, 2020

@sunkuranganath At least I cannot. @rpv-tomsk Can you resolve the changerequest?

@dago dago removed the request for review from rpv-tomsk April 9, 2020 14:27
dago
dago reviewed Apr 9, 2020
View reviewed changes
Copy link
Contributor
@dago dago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dago dago merged commit 86275a6 into collectd:master Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rpv-tomsk rpv-tomsk rpv-tomsk requested changes

@dago dago dago approved these changes

@kwiatrox kwiatrox kwiatrox left review comments

Assignees
No one assigned
Labels
5449 linux only New plugin
Projects
None yet
Milestone
5.12
Development

Successfully merging this pull request may close these issues.
9 participants
@mfijalko @rpv-tomsk @kkepka @dago @sunkuranganath @kwiatrox @octo @mrunge @mkobyli
0