Skip to content
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

Added monitoring of Mikrotik router temperature #1049

Merged

Conversation

alejandroscf
Copy link
Contributor

Monitor temperature reported by Mikrotik routers via SNMP protocol.

PS: The multigraph approach really make easier add functionality.

@@ -57,6 +58,7 @@ if (defined $ARGV[0] and $ARGV[0] eq "snmpconf") {
print "require $sysFlashTotalOID\n";
print "require $sysRAMUsageOID\n";
print "require $sysRAMTotalOID\n";
print "require $sysTempOID\n";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this line as some routers didn't report temperature and I don't know if it could affect how the snmpconf parameter works for that routers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably you are right (I do not know SNMP plugins too well). Thus maybe just remove it and add a comment above, that only attributes available on all routers should be listed here as required.

At least this is my understanding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I found it in the file SNMPConfig.pm. All OIDs are checked and if any fails then the plugins is not suggested, so the it should be removed. I'll do it.

But I found out that the requirements for the other OIDs are not working as expected. Just now the 'snmp__mikrotik' plugin is being recommended for any device, even if the OID is not present (at least in my system).
That happen because the require {OID} could have a second parameter like require {OID} [{filter}] and without a filter the plugin is suggested regardless of the OID. I've tested it against devices from other vendors (that they don't have any of the requested OIDs) and it get suggested anyway.

The lines should be something like:

print "require $OID ^\\d\n";

But I don't know if I should fix that in this PR or in another.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The filter should be optional (at least from reading the code). But maybe the implementation of _snmp_check_require in munin is broken?

Just for testing a theory - maybe you could change the following line:

my $response = $session->get_request($oid);

into the following:

my $response = $session->get_request(-varbindlist => [ $oid ],);

At least this seems to be documented way for calling get_request. Does this fix the issue for you?

(I do not have an SNMP-capable device around, thus I cannot test it easily)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tested adding the '-varbindlist' parameter and it has no effect. I've also tested if any error was returned when trying to get an inaccessible oid and no error was reported. I found this bug that it's not exactly the same but has a similar effect.
I noticed that other plugins like snmp__load or snmp__print_supplies just use the filter parameter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we proceed with this plugin now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been testing a perl script with only Net::SNMP and I found what looks like a bug in that perl library that didn't throw an error when trying to get an inaccessible oid when working with version 2c of snmp. I'm going to report it, maybe the cause is the same that the other bug.

We can wait until this is solved (but Net::SNMP doesn't looks very active) or I can upload the version I have with the filter parameter and merge it.

Copy link
Contributor Author

@alejandroscf alejandroscf Apr 10, 2020

Choose a reason for hiding this comment

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

Here is the bug report to Net::SNMP if you want to test it or try to found a solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for digging further into this issue!

I would be glad, if you would take the following steps:

  1. open an issue against munin describing the current problem with the required OID setting (I would have a hard time putting the details together, sorry)
  2. apply your filter workaround to the plugin and reference the issue above (in order to remove the workaround somewhen in the future)

Thank you a lot for your attention to the details!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just done both things!

@sumpfralle
Copy link
Collaborator

sumpfralle commented Mar 27, 2020

PS: The multigraph approach really make easier add functionality.

I am glad, you like it :)

Your changes look good to me.

@sumpfralle
Copy link
Collaborator

@alejandroscf: did you already find the time to look into the filter issue? Do you want to prepare the plugin with the added filter workaround for now?

(just curious - no pressure intended ...)

@alejandroscf
Copy link
Contributor Author

alejandroscf commented Apr 22, 2020

I mostly work in this the weekends and last one I had to work, but tomorrow is holiday and I just posted the issue #1073

@sumpfralle sumpfralle merged commit 699158f into munin-monitoring:master Apr 25, 2020
@sumpfralle
Copy link
Collaborator

Great - thank you!

@alejandroscf alejandroscf deleted the feature/mikrotik-temp branch April 26, 2020 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants