newdevice: Added Coriant support #6026

Merged
merged 25 commits into from Mar 11, 2017

Conversation

Projects
None yet
7 participants
@xbeaudouin
Contributor

xbeaudouin commented Feb 27, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

This PR is the first part or Coriant devices support using TNMS.

It will, using MEF-EVC MIB get a status of the links.

Since MEF support can be used on some other devices it can be added to some other devices.

Coriant have accepted that TNMS-NBI-MIB can be added into Librenms (I can give the official mail for that). Other MIBs are opensource or equivalent.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Feb 27, 2017

Thank you for submitting a PR @xbeaudouin! We have found the following @murrant, @laf and @InsaneSplash based on the history of these files to review this PR.

Thank you for submitting a PR @xbeaudouin! We have found the following @murrant, @laf and @InsaneSplash based on the history of these files to review this PR.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
Xavier Beaudouin
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
Xavier Beaudouin
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

Xavier Beaudouin added some commits Feb 28, 2017

Xavier Beaudouin
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
Xavier Beaudouin
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
Xavier Beaudouin
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
html/images/os/coriant.svg
+ d="m 487.334,64.95 -8.667,-18.283 c 0,0 59.999,-14.333 70.333,55.167 0,59.5 -80,79.833 -109.666,21.166 14,3.667 25.333,1.667 25.333,1.667 0,0 8.334,12.999 27.167,12.333 18.833,-0.666 35.833,-11.999 35.833,-33.666 0,-21.667 -15.999,-37.768 -40.333,-38.384 z" /><path
+ style="fill:#00467f"
+ id="path101"
+ d="M 335.667,58.334 C 334.334,45.667 324,35.001 304,35.001 c -20,0 -31.333,13 -31.333,13 l 10.667,10.333 c 0,0 7.666,-7.333 21.333,-7.333 13.667,0 14,13.333 14,13.333 0,0 -1.667,0 -16.204,0 -14.537,0 -33.796,8 -33.796,23.667 0,15.667 12.333,24 26,25 13.667,1 23.333,-9.667 23.333,-9.667 v 7.334 h 17.667 c 0,0 1.333,-39.667 0,-52.334 z m -19,28.333 c 0,0 -2.667,10.667 -14.204,10.667 C 294,97.334 289,94 289,88.667 c 0,-5.333 9.334,-10.666 17.667,-10.333 8.333,0.333 10,0 10,0 z" /></svg>

This comment has been minimized.

@geordish

geordish Feb 28, 2017

Contributor

Can you run this through something like svgo to compress it down a bit (https://github.com/svg/svgo)

@geordish

geordish Feb 28, 2017

Contributor

Can you run this through something like svgo to compress it down a bit (https://github.com/svg/svgo)

This comment has been minimized.

@xbeaudouin

xbeaudouin Feb 28, 2017

Contributor

Done :)

@xbeaudouin

xbeaudouin Feb 28, 2017

Contributor

Done :)

Xavier Beaudouin
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@@ -0,0 +1 @@
+1.3.6.1.2.1.1.2.0|6|1.3.6.1.4.1.42229.6.22

This comment has been minimized.

@geordish

geordish Feb 28, 2017

Contributor

Can you also add the sysDescr here too please.

@geordish

geordish Feb 28, 2017

Contributor

Can you also add the sysDescr here too please.

This comment has been minimized.

@xbeaudouin

xbeaudouin Mar 1, 2017

Contributor

sysDescr is not relevant. Since Coriant TNMS is a proxy.
On my TNMS we have :
snmpwalk -v2c -c OsB24 par-eqx-tnms-01:50161 1.3.6.1.2.1.1.1.0
iso.3.6.1.2.1.1.1.0 = STRING: "Linux version 3.10.0-327.el7.x86_64"

But you can have also a Windows. So sysDescr is not usuable.

@xbeaudouin

xbeaudouin Mar 1, 2017

Contributor

sysDescr is not relevant. Since Coriant TNMS is a proxy.
On my TNMS we have :
snmpwalk -v2c -c OsB24 par-eqx-tnms-01:50161 1.3.6.1.2.1.1.1.0
iso.3.6.1.2.1.1.1.0 = STRING: "Linux version 3.10.0-327.el7.x86_64"

But you can have also a Windows. So sysDescr is not usuable.

This comment has been minimized.

@laf

laf Mar 1, 2017

Member

Whilst it can vary this is purely for testing so you should add what you have available, the idea is that we can then ensure other changes don't break detection

@laf

laf Mar 1, 2017

Member

Whilst it can vary this is purely for testing so you should add what you have available, the idea is that we can then ensure other changes don't break detection

This comment has been minimized.

@xbeaudouin

xbeaudouin Mar 2, 2017

Contributor

Ok I have added it.

@xbeaudouin

xbeaudouin Mar 2, 2017

Contributor

Ok I have added it.

@laf laf added the Device 🖥 label Mar 1, 2017

@laf laf changed the title from Coriant basic to newdevice: Added Coriant support Mar 1, 2017

+text: 'Coriant TNMS'
+type: network
+icon: coriant
+discovery_modules:

This comment has been minimized.

@laf

laf Mar 1, 2017

Member

From discovery_module you can remove the following as they are all 0 by default:

loadbalancers
toner
libvirt-vminfo
vmware-vminfo
junose-atm-vp

@laf

laf Mar 1, 2017

Member

From discovery_module you can remove the following as they are all 0 by default:

loadbalancers
toner
libvirt-vminfo
vmware-vminfo
junose-atm-vp

+ mef-evc: 1
+ cisco-vrf-lite: 0
+ storage: 0
+poller_modules:

This comment has been minimized.

@laf

laf Mar 1, 2017

Member

From poller_modules you can remove the following as they are all 0 by default:

junose-atm-vp
toner
loadbalancers

@laf

laf Mar 1, 2017

Member

From poller_modules you can remove the following as they are all 0 by default:

junose-atm-vp
toner
loadbalancers

includes/discovery/mef-evc.inc.php
+ */
+ if (dbFetchCell("SELECT COUNT(id) FROM `mefinfo` WHERE `device_id` = ? AND `mefID` = ?", array($device['device_id'], $index)) == 0) {
+ $mefid = dbInsert(array('device_id' => $device['device_id'], 'mefID' => $index, 'mefType' => mres($mefType), 'mefIdent' => mres($mefIdent), 'mefMTU' => mres($mefMtu), 'mefAdmState' => mres($mefAdmState), 'mefRowState' => mres($mefRowState)), 'mefinfo');
+ log_event("MEF link: ". mres($mefIdent) . " (" . $index . ") Discovered", $device, 'system', $mefid);

This comment has been minimized.

@laf

laf Mar 1, 2017

Member

$mefid is actually what the severity should be:

        case 1:
            return "severity-ok"; //OK
            break;
        case 2:
            return "severity-info"; //Informational
            break;
        case 3:
            return "severity-notice"; //Notice
            break;
        case 4:
            return "severity-warning"; //Warning
            break;
        case 5:
            return "severity-critical"; //Critical
            break;
@laf

laf Mar 1, 2017

Member

$mefid is actually what the severity should be:

        case 1:
            return "severity-ok"; //OK
            break;
        case 2:
            return "severity-info"; //Informational
            break;
        case 3:
            return "severity-notice"; //Notice
            break;
        case 4:
            return "severity-warning"; //Warning
            break;
        case 5:
            return "severity-critical"; //Critical
            break;

This comment has been minimized.

@xbeaudouin

xbeaudouin Mar 2, 2017

Contributor

I don't understand what you mean there?

@xbeaudouin

xbeaudouin Mar 2, 2017

Contributor

I don't understand what you mean there?

This comment has been minimized.

@xbeaudouin

xbeaudouin Mar 2, 2017

Contributor

Fixed.

@xbeaudouin

xbeaudouin Mar 2, 2017

Contributor

Fixed.

includes/discovery/mef-evc.inc.php
+ */
+ if (!in_array($db_mef['mefID'], $mefevc_list)) {
+ dbDelete('mefinfo', '`id` = ?', array($db_mef['id']));
+ log_event("MEF link: ".mres($db_mef['mefIdent']).' Removed', $device, 'system', $db_mef['mefID']);

This comment has been minimized.

@laf

laf Mar 1, 2017

Member

Same here for severity

@laf

laf Mar 1, 2017

Member

Same here for severity

This comment has been minimized.

@xbeaudouin

xbeaudouin Mar 2, 2017

Contributor

Fixed.

@xbeaudouin

xbeaudouin Mar 2, 2017

Contributor

Fixed.

includes/defaults.inc.php
@@ -712,6 +712,7 @@
$config['poller_modules']['ntp'] = 1;
$config['poller_modules']['services'] = 1;
$config['poller_modules']['loadbalancers'] = 0;
+$config['poller_modules']['mef-evc'] = 0;

This comment has been minimized.

@laf

laf Mar 1, 2017

Member

mef-evc seems very specific, should we do this as just mef or is that to generic?

@laf

laf Mar 1, 2017

Member

mef-evc seems very specific, should we do this as just mef or is that to generic?

This comment has been minimized.

@xbeaudouin

xbeaudouin Mar 2, 2017

Contributor

Well, I dunno. Seems the mef-evc is kinda related to the mib itself. It should be generic and can be used on some other network devices (like adva for example).
You choose what you will like for that... :)

@xbeaudouin

xbeaudouin Mar 2, 2017

Contributor

Well, I dunno. Seems the mef-evc is kinda related to the mib itself. It should be generic and can be used on some other network devices (like adva for example).
You choose what you will like for that... :)

This comment has been minimized.

@laf

laf Mar 2, 2017

Member

It's a question rather than an ask. I just don't want to see us end up with 10 modules all called mef-this, mef-that, etc.

@laf

laf Mar 2, 2017

Member

It's a question rather than an ask. I just don't want to see us end up with 10 modules all called mef-this, mef-that, etc.

This comment has been minimized.

@murrant

murrant Mar 2, 2017

Member

I'd say start with just mef. We can rename it later if needed.

@murrant

murrant Mar 2, 2017

Member

I'd say start with just mef. We can rename it later if needed.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
includes/discovery/mef-evc.inc.php
+ * Try to discover any MEF Links
+ */
+
+if ($device['os'] == 'coriant') {

This comment has been minimized.

@geordish

geordish Mar 2, 2017

Contributor

Should this not be checking for the mef-evc discovery item being set?

@geordish

geordish Mar 2, 2017

Contributor

Should this not be checking for the mef-evc discovery item being set?

This comment has been minimized.

@xbeaudouin

xbeaudouin Mar 2, 2017

Contributor

Well, as you want. Tell me if you would like I change that.

@xbeaudouin

xbeaudouin Mar 2, 2017

Contributor

Well, as you want. Tell me if you would like I change that.

This comment has been minimized.

@laf

laf Mar 2, 2017

Member

If it's a standard that devices should support then the if check can be completely removed as the module will only be on for relevant devices anyway.

@laf

laf Mar 2, 2017

Member

If it's a standard that devices should support then the if check can be completely removed as the module will only be on for relevant devices anyway.

This comment has been minimized.

@murrant

murrant Mar 2, 2017

Member

I believe the poller code takes care of that.

@murrant

murrant Mar 2, 2017

Member

I believe the poller code takes care of that.

This comment has been minimized.

@xbeaudouin

xbeaudouin Mar 3, 2017

Contributor

Ok so let's do it :) Commit is comming :)

@xbeaudouin

xbeaudouin Mar 3, 2017

Contributor

Ok so let's do it :) Commit is comming :)

includes/polling/mef-evc.inc.php
@@ -0,0 +1,53 @@
+<?php
+
+if ($device['os'] == 'coriant') {

This comment has been minimized.

@geordish

geordish Mar 2, 2017

Contributor

Should this not be checking for the mef-evc poller item being set?

@geordish

geordish Mar 2, 2017

Contributor

Should this not be checking for the mef-evc poller item being set?

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
Xavier Beaudouin
Since MEF-EVC can be used on other devices than Coriant's one, lets t…
…his be used by the poller/discovery itself
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
Xavier Beaudouin
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Mar 5, 2017

Member

I think the current svg should be moved to the logos dir and the os image should be the semi-circle with the dot. (Which matches the favicon on their website)

Member

murrant commented Mar 5, 2017

I think the current svg should be moved to the logos dir and the os image should be the semi-circle with the dot. (Which matches the favicon on their website)

Xavier Beaudouin added some commits Mar 6, 2017

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
Xavier Beaudouin
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

murrant added some commits Mar 7, 2017

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Mar 7, 2017

Member

Thanks @xbeaudouin this is pretty close.

It seems like the discovery and poller modules do much the same thing. What is the difference? If they do the same thing why do we need both?

Member

murrant commented Mar 7, 2017

Thanks @xbeaudouin this is pretty close.

It seems like the discovery and poller modules do much the same thing. What is the difference? If they do the same thing why do we need both?

@xbeaudouin

This comment has been minimized.

Show comment
Hide comment
@xbeaudouin

xbeaudouin Mar 7, 2017

Contributor

@murrant mostly because the links can change in between too polls. I took what was done on VM stuff on vmware esx. Maybe this was not a good example?

Contributor

xbeaudouin commented Mar 7, 2017

@murrant mostly because the links can change in between too polls. I took what was done on VM stuff on vmware esx. Maybe this was not a good example?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Mar 7, 2017

Member

Can you switch the modules to just mef @xbeaudouin ?

Member

laf commented Mar 7, 2017

Can you switch the modules to just mef @xbeaudouin ?

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Mar 11, 2017

The inspection completed: 1 updated code elements

The inspection completed: 1 updated code elements

@murrant murrant merged commit fa29839 into librenms:master Mar 11, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment