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

Add basic support for MRV OptiDriver Optical Transport Platform #6656

Merged
merged 5 commits into from May 18, 2017

Conversation

Projects
None yet
4 participants
@thecityofguanyu
Contributor

thecityofguanyu commented May 16, 2017

Add basic support for MRV OptiDriver Optical Transport Platform

  • OS Detection
  • Sensors
  • Chassis temperature
  • State
  • Tx/Rx Optical Interface Power
  • Power Supply
  • Fan

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.

Testers

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

Committer: Chris A. Evans <thecityofguanyu@outlook.com>
Add basic support for MRV OptiDriver Optical Transport Platform
 * OS Detection
 * Sensors
  * Chassis temperature
  * State
   * Tx/Rx Optical Interface Power
   * Power Supply
   * Fan
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented May 16, 2017

Auto-Deploy finished, Test PR at http://6656.ci.librenms.org or https://6656.ci.librenms.org

@laf

Quite a few changes needed in this, mostly code optimsations / tidy ups

Show outdated Hide outdated includes/definitions/mrv-optidriver.yaml
@@ -0,0 +1,10 @@
os: mrv-optidriver

This comment has been minimized.

@laf

laf May 16, 2017

Member

Is this the best os name?

@laf

laf May 16, 2017

Member

Is this the best os name?

Show outdated Hide outdated includes/definitions/mrv-optidriver.yaml
@@ -0,0 +1,10 @@
os: mrv-optidriver
group: mrv

This comment has been minimized.

@laf

laf May 16, 2017

Member

Do you need this? It's designed to bring a number of OS' together so polling can be done centrally.

@laf

laf May 16, 2017

Member

Do you need this? It's designed to bring a number of OS' together so polling can be done centrally.

This comment has been minimized.

@laf

laf May 16, 2017

Member

If it is removed, add:

mib_dir:
    - mrv
@laf

laf May 16, 2017

Member

If it is removed, add:

mib_dir:
    - mrv

This comment has been minimized.

@thecityofguanyu

thecityofguanyu May 16, 2017

Contributor

@laf I based that off of the pre-existing ./includes/definitions/mrvld.yaml, which denotes the mrv group. MRV has a larger suite of devices than just OptiDriver (OptiPacket, OptiSwitch), so I would guess that they could have definition OS types.

@thecityofguanyu

thecityofguanyu May 16, 2017

Contributor

@laf I based that off of the pre-existing ./includes/definitions/mrvld.yaml, which denotes the mrv group. MRV has a larger suite of devices than just OptiDriver (OptiPacket, OptiSwitch), so I would guess that they could have definition OS types.

This comment has been minimized.

@laf

laf May 16, 2017

Member

It's only needed if they share the same code which they don't appear to.

@laf

laf May 16, 2017

Member

It's only needed if they share the same code which they don't appear to.

Show outdated Hide outdated includes/definitions/mrv-optidriver.yaml
icon: mrv
discovery:
- sysObjectId:
- .1.3.6.1.4.1.629.200.1.1.1.

This comment has been minimized.

@laf

laf May 16, 2017

Member

You might be able to change this to .1.3.6.1.4.1.629.200.

@laf

laf May 16, 2017

Member

You might be able to change this to .1.3.6.1.4.1.629.200.

This comment has been minimized.

@thecityofguanyu

thecityofguanyu May 16, 2017

Contributor

The 1.1.1 is what's showing in the full snmpget of sysObjectID.

.1.3.6.1.2.1.1.2.0 = OID: .1.3.6.1.4.1.629.200.1.1.1
SNMPv2-MIB::sysObjectID.0 = OID: NBS-CMMC-MIB::nbsCmmcObjects.1.1

Still want it shorted?

@thecityofguanyu

thecityofguanyu May 16, 2017

Contributor

The 1.1.1 is what's showing in the full snmpget of sysObjectID.

.1.3.6.1.2.1.1.2.0 = OID: .1.3.6.1.4.1.629.200.1.1.1
SNMPv2-MIB::sysObjectID.0 = OID: NBS-CMMC-MIB::nbsCmmcObjects.1.1

Still want it shorted?

This comment has been minimized.

@laf

laf May 16, 2017

Member

Looking at our stats, nothing else uses it so yes it should be ok to be shortened.

@laf

laf May 16, 2017

Member

Looking at our stats, nothing else uses it so yes it should be ok to be shortened.

Show outdated Hide outdated includes/discovery/sensors/dbm/mrv-optidriver.inc.php
$multiplier = 1;
$divisor = 1000;
$limit_low = -100;

This comment has been minimized.

@laf

laf May 16, 2017

Member

Are these limits sane?

@laf

laf May 16, 2017

Member

Are these limits sane?

This comment has been minimized.

@thecityofguanyu

thecityofguanyu May 16, 2017

Contributor

Multiplier and divisor are right, but those limits are definitely not sane. I'll see if I can find some documentation to set those accordingly.

@thecityofguanyu

thecityofguanyu May 16, 2017

Contributor

Multiplier and divisor are right, but those limits are definitely not sane. I'll see if I can find some documentation to set those accordingly.

This comment has been minimized.

@laf

laf May 16, 2017

Member

You can set them to null and we will calculate some values.

@laf

laf May 16, 2017

Member

You can set them to null and we will calculate some values.

Show outdated Hide outdated includes/discovery/sensors/dbm/mrv-optidriver.inc.php
'dbm',
$device,
$oid,
'rx-' . $index,

This comment has been minimized.

@laf

laf May 16, 2017

Member

probably better to do 'nbsCmmcPortRxPower.'.$index,

@laf

laf May 16, 2017

Member

probably better to do 'nbsCmmcPortRxPower.'.$index,

Show outdated Hide outdated includes/discovery/sensors/state/mrv-optidriver.inc.php
}
// Fan 1
if (is_array($oids)) {

This comment has been minimized.

@laf

laf May 16, 2017

Member

Same as the psu ones above.

@laf

laf May 16, 2017

Member

Same as the psu ones above.

Show outdated Hide outdated includes/discovery/sensors/temperature/mrv-optidriver.inc.php
*/
if ($device['os'] == 'mrv-optidriver') {

This comment has been minimized.

@laf

laf May 16, 2017

Member

This check isn't needed

@laf

laf May 16, 2017

Member

This check isn't needed

Show outdated Hide outdated includes/discovery/sensors/temperature/mrv-optidriver.inc.php
if ($device['os'] == 'mrv-optidriver') {
echo("MRV OptiDriver:");
// Chassis temperature
$high_limit = 70;

This comment has been minimized.

@laf

laf May 16, 2017

Member

Are these sane limits?

@laf

laf May 16, 2017

Member

Are these sane limits?

Show outdated Hide outdated includes/discovery/sensors/temperature/mrv-optidriver.inc.php
$value = snmp_get($device, 'nbsCmmcChassisTemperature.1', '-Ovqs', 'NBS-CMMC-MIB', $config['install_dir'].'/mibs/mrv');
//$value = str_replace('"', '', $value);
if (is_numeric($value)) {
discover_sensor($valid['sensor'], 'temperature', $device, $valueoid, 1, 'mrv-optidriver', $descr, '1', '1', $low_limit, $low_warn_limit, $high_warn_limit, $high_limit, $value);

This comment has been minimized.

@laf

laf May 16, 2017

Member

Change the index of 1 to be nbsCmmcChassisTemperature.1

@laf

laf May 16, 2017

Member

Change the index of 1 to be nbsCmmcChassisTemperature.1

Show outdated Hide outdated includes/polling/os/mrv-optidriver.inc.php
* the source code distribution for details.
*/
$hardware = snmp_get($device, 'nbsCmmcChassisModel.1', '-Ovqs', 'NBS-CMMC-MIB', $config['install_dir'].'/mibs/mrv');

This comment has been minimized.

@laf

laf May 16, 2017

Member

Please convert all of these to use a single snmp_mult_get_oid call

@laf

laf May 16, 2017

Member

Please convert all of these to use a single snmp_mult_get_oid call

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 16, 2017

Member

You've got numerous issues that need fixing in the code to make it PSR-2 compliant: https://travis-ci.org/librenms/librenms/jobs/232922495

Also needs unit tests.

Member

laf commented May 16, 2017

You've got numerous issues that need fixing in the code to make it PSR-2 compliant: https://travis-ci.org/librenms/librenms/jobs/232922495

Also needs unit tests.

renamed: includes/definitions/mrv-optidriver.yaml -> includes/defini…
…tions/mrv-od.yaml

 	new file:   includes/discovery/sensors/dbm/mrv-od.inc.php
 	deleted:    includes/discovery/sensors/dbm/mrv-optidriver.inc.php
 	renamed:    includes/discovery/sensors/pre-cache/mrv-optidriver.inc.php -> includes/discovery/sensors/pre-cache/mrv-od.inc.php
 	renamed:    includes/discovery/sensors/state/mrv-optidriver.inc.php -> includes/discovery/sensors/state/mrv-od.inc.php
 	new file:   includes/discovery/sensors/temperature/mrv-od.inc.php
 	deleted:    includes/discovery/sensors/temperature/mrv-optidriver.inc.php
 	renamed:    includes/polling/os/mrv-optidriver.inc.php -> includes/polling/os/mrv-od.inc.php
@thecityofguanyu

This comment has been minimized.

Show comment
Hide comment
@thecityofguanyu

thecityofguanyu May 16, 2017

Contributor

@laf I believe that I have addressed for of your optimization / tidy up issues in the latest commit. I have not yet addressed the Travis complaints. I want to ensure the code is all how you want it before I touch on that.

Contributor

thecityofguanyu commented May 16, 2017

@laf I believe that I have addressed for of your optimization / tidy up issues in the latest commit. I have not yet addressed the Travis complaints. I want to ensure the code is all how you want it before I touch on that.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented May 16, 2017

Auto-Deploy finished, Test PR at http://6656.ci.librenms.org or https://6656.ci.librenms.org

modified: includes/discovery/sensors/dbm/mrv-od.inc.php
  Added missing ')' in the if conditionals.
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented May 16, 2017

Auto-Deploy finished, Test PR at http://6656.ci.librenms.org or https://6656.ci.librenms.org

@laf

Last few changes then all good. Can you remove the .mib from the end of the MIB file names pls.

Show outdated Hide outdated includes/definitions/mrv-od.yaml
discovery:
- sysObjectId:
- .1.3.6.1.4.1.629.200.
- sysDescr:

This comment has been minimized.

@laf

laf May 17, 2017

Member

You can probably drop the sysDescr check

@laf

laf May 17, 2017

Member

You can probably drop the sysDescr check

Show outdated Hide outdated includes/discovery/sensors/temperature/mrv-od.inc.php
// Chassis temperature
$descr = "Chassis Temperature";
$valueoid = ".1.3.6.1.4.1.629.200.6.1.1.15.1";
$value = snmp_get($device, 'nbsCmmcChassisTemperature.1', '-Ovqs', 'NBS-CMMC-MIB', $config['install_dir'].'/mibs/mrv');

This comment has been minimized.

@laf

laf May 17, 2017

Member

You shouldn't need (or use) ``$config['install_dir'].'/mibs/mrv'`. Our code will populate that.

@laf

laf May 17, 2017

Member

You shouldn't need (or use) ``$config['install_dir'].'/mibs/mrv'`. Our code will populate that.

$hardware = $multi_get_array[1]['NBS-CMMC-MIB::nbsCmmcChassisModel'];
$version = $multi_get_array[0]['NBS-CMMC-MIB::nbsCmmcSysFwVers'];
$serial = $multi_get_array[1]['NBS-CMMC-MIB::nbsCmmcChassisSerialNum'];
$features = '';

This comment has been minimized.

@laf

laf May 17, 2017

Member

Add unset($multi_get_array);

@laf

laf May 17, 2017

Member

Add unset($multi_get_array);

Requested changes
 	modified:   includes/definitions/mrv-od.yaml
 	modified:   includes/discovery/sensors/state/mrv-od.inc.php
 	modified:   includes/discovery/sensors/temperature/mrv-od.inc.php
 	modified:   includes/polling/os/mrv-od.inc.php
 	removed extensions from all new mib files
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented May 18, 2017

Auto-Deploy finished, Test PR at http://6656.ci.librenms.org or https://6656.ci.librenms.org

Add snmprec file to satisfy test units
	new file:   tests/snmpsim/mrv-od.snmprec
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented May 18, 2017

Auto-Deploy finished, Test PR at http://6656.ci.librenms.org or https://6656.ci.librenms.org

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier May 18, 2017

The inspection completed: No new issues

scrutinizer-notifier commented May 18, 2017

The inspection completed: No new issues

@laf laf merged commit 8565417 into librenms:master May 18, 2017

3 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@thecityofguanyu thecityofguanyu deleted the thecityofguanyu:newdevice_mrv_optidriver_support branch May 22, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 18, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 18, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.