CISCO-NTP-MIB #4005

Merged
merged 4 commits into from Aug 17, 2016

Projects

None yet

3 participants

@adaniels21487
Contributor

A new Discovery/Poller module to collect NTP statistics from devices which support the CISCO-NTP-MIB

  • Discovered peers are stored using components
  • Statistics are displayed using 'applications'
  • A critical alarm is raised when a stratum of 16 is reported.
@adaniels21487 adaniels21487 CISCO-NTP-MIB
A new Discovery/Poller module to collect NTP statistics from devices which support the CISCO-NTP-MIB
Discovered peers are stored using components and statistics are displayed using 'applications'
A critical alarm is raised when a stratum of 16 is reported.
b9747b2
@laf
Member
laf commented Aug 8, 2016

All looks good except that I think the disco and poller modules should just be called ntp as cisco based devices won't be the only thing that supports them.

We are also just about to merge a PR which tidies up the rrdtool functions in polling/discovery so you will need to update the code when that's merged.

@adaniels21487
Contributor

Hi @laf
Re: rrdtool function.
No worries. I will update when its merged.

Re: rename the modules.
The MIB is Cisco proprietry and part of the Cisco management tree (1.3.6.1.4.1.9.9.168), I dont think anything non-Cisco will be implementing this MIB but happy to rename if you like.
Please confirm.

@laf
Member
laf commented Aug 9, 2016

Sorry I didn't mean that other vendors will use this mib, more that other vendors will have an implementation of NTP so we should make the module re-usable.

Like for instance the bgp polling, that does cisco and juniper with an if/elseif. So in the disco / poller module for this have ntp.inc.php, that then just does

if ($device['os'] == "cisco") {
    require_once 'includes/discovery/ntp/cisco.inc.php';
}
@laf
Member
laf commented Aug 10, 2016

Ok, big PR merged. If you can update your code to match that would be ace.

@adaniels21487
Contributor

Hi @laf
Understood, will do.

adaniels21487 added some commits Aug 12, 2016
@adaniels21487 adaniels21487 Merge branch 'master' into issue-3999 27eb389
@adaniels21487 adaniels21487 - Modified to use standardised RRD functions 4c34541
@adaniels21487 adaniels21487 - Made module generic, not cisco specific.
4c060a4
@adaniels21487
Contributor

Hi @laf
Please let me know if this is what you had in mind?

@laf
Member
laf commented Aug 14, 2016

Initial look is what I was trying to say yes :)

@adaniels21487
Contributor

I dont believe this is still a blocker.

@laf laf commented on the diff Aug 17, 2016
html/includes/graphs/device/ntp_delay.inc.php
+ if ( isset($config['graph_colours']['mixed'][$count]) ) {
+ $color = $config['graph_colours']['mixed'][$count];
+ } else {
+ $color = $config['graph_colours']['oranges'][$count-7];
+ }
+
+ $rrd_additions .= " DEF:DS" . $count . "=" . $rrd_filename . ":delay:AVERAGE ";
+ $rrd_additions .= " LINE1.25:DS" . $count . "#" . $color . ":'" . str_pad(substr($array['peer'].' (s)',0,15),15) . "'" . $stack;
+ $rrd_additions .= " GPRINT:DS" . $count . ":LAST:%7.0lf ";
+ $rrd_additions .= " GPRINT:DS" . $count . ":MIN:%7.0lf ";
+ $rrd_additions .= " GPRINT:DS" . $count . ":MAX:%7.0lf\\\l ";
+ $count++;
+ }
+}
+
+if ($rrd_additions == "") {
@laf
laf Aug 17, 2016 Member

Seems odd to check and not do anything, may as well just !empty() or something and then append to $rrd_options.

Not a show stopper but would be nice to change it post PR.

@adaniels21487
adaniels21487 Aug 17, 2016 Contributor

So the plan here was to try and display an error message on the graph when there was no DS but I couldnt figure out how and then I forgot all about it. You will find this is most of the graphs I have created.

Do you know how/if one can display an error message on a graph without a DS?
Or I am happy to just remove them if you prefer?

@laf laf commented on the diff Aug 17, 2016
html/includes/graphs/device/ntp_dispersion.inc.php
+ if ( isset($config['graph_colours']['mixed'][$count]) ) {
+ $color = $config['graph_colours']['mixed'][$count];
+ } else {
+ $color = $config['graph_colours']['oranges'][$count-7];
+ }
+
+ $rrd_additions .= " DEF:DS" . $count . "=" . $rrd_filename . ":dispersion:AVERAGE ";
+ $rrd_additions .= " LINE1.25:DS" . $count . "#" . $color . ":'" . str_pad(substr($array['peer'].' (s)',0,15),15) . "'" . $stack;
+ $rrd_additions .= " GPRINT:DS" . $count . ":LAST:%7.0lf ";
+ $rrd_additions .= " GPRINT:DS" . $count . ":MIN:%7.0lf ";
+ $rrd_additions .= " GPRINT:DS" . $count . ":MAX:%7.0lf\\\l ";
+ $count++;
+ }
+}
+
+if ($rrd_additions == "") {
@laf
laf Aug 17, 2016 Member

Same here.

@laf laf removed the Blocker label Aug 17, 2016
@laf
Member
laf commented Aug 17, 2016

I'm merging this but we really really need to have all tables paginated and I was considering not merging this at present.

However, can you please submit a new PR to resolve this.

@laf laf merged commit a42ba9a into librenms:master Aug 17, 2016

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adaniels21487
Contributor

Thanks @laf
Are you referring to:
html/pages/apps/ntp.inc.php
html/pages/device/apps/ntp.inc.php

Let me know and I'll get it done.

@adaniels21487 adaniels21487 deleted the adaniels21487:issue-3999 branch Aug 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment