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 Dlink MIBS, memory polling and temp polling for some devices #8076

Merged
merged 5 commits into from Jan 21, 2018

Conversation

Projects
None yet
5 participants
@hanserasmus
Contributor

hanserasmus commented Jan 12, 2018

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

I don't have all the devices and FW combinations that Dlink has to offer, so the tests were done on the following devices:

System info like FW version, HW version and Serial number now displaying properly:

Sucessful:
DGS-3620-28SC FW 3.00.004
DGS-3420-28SC FW 3.01.R003
DGS-3627G FW 3.00.B47
DGS-3420-28TC FW 3.01.R003 & FW 1.50.018
DES-3528 FW 3.00.012
DGS-3120-24TC FW 3.10.012

Not successful:
DES-3526 (OID returns empty)

Temperature info:

Successful:
DGS-3620-28SC FW 3.00.004
DGS-3420-28TC FW 1.50.018
DES-3528 FW 3.00.012
DGS-3120-24TC FW 3.10.012

Not successful:
DES-3526 & DGS-3627G (Will never support it, switch does not have environment capabilities)
All DGS-3420-28xx with FW 3.01.R003. Seems to be a FW issue which we will take up with the vendors. We expect fixes in the near future in the FW release.

Screenshots of how the data displays is attached as well.
temperature

device-details

@CLAassistant

This comment has been minimized.

CLAassistant commented Jan 12, 2018

CLA assistant check
All committers have signed the CLA.

@murrant

This comment has been minimized.

Member

murrant commented Jan 13, 2018

Looks good.

Can you add test data for these?

./scripts/save-tests-data -c -m mempools,processors,sensors -h HOSTNAME

(Use -v if you need multiple sets of data)

@hanserasmus

This comment has been minimized.

Contributor

hanserasmus commented Jan 14, 2018

On how many devices should I do this? One of each listed as successfully polled in my previous post?

@laf

This comment has been minimized.

Member

laf commented Jan 14, 2018

If you have the time to do that then yes that would be great. It ensures we don't break support for these devices in the future.

@laf laf added the Device 🖥 label Jan 14, 2018

@hanserasmus

This comment has been minimized.

Contributor

hanserasmus commented Jan 14, 2018

Will do that tomorrow morning and post it back here.

@hanserasmus

This comment has been minimized.

Contributor

hanserasmus commented Jan 15, 2018

OK so I have created the files you were looking for. Here they are. I have modified the filenames to resemble the Model number + FW version of the device. Hope this helps. Anything else?

SNMPrec-results.zip

@murrant

This comment has been minimized.

Member

murrant commented Jan 16, 2018

@hanserasmus I glanced at the data and I don't see any of the oids that hold the data .1.3.6.1.4.1.171.12.11.1.9.4.1.. Did you collect the data with your patch applied?

Also, if you install snmpsim, you can check the data for yourself. https://docs.librenms.org/Developing/os/Test-Units/

@hanserasmus

This comment has been minimized.

Contributor

hanserasmus commented Jan 16, 2018

@murrant
The method I used was:

  1. Install LibreNMS
  2. Change the files in the current LibreNMS install to resemble the submitted files in git.
  3. Keep track of all those files, and copy them to my local machine.
  4. On my local machine I cloned a forked repo of LibreNMS.
  5. Copied all of the changed files over to my local machine with the forked repo, and added them to the repo as per instruction on the site.

So in short, my current running system, on which I ran the save-test-data scripts, have all the files I have submitted, and is therefor "patched". Running ./validate.php results in:

./validate.php

Component Version
LibreNMS 1.35-97-g788c0bd
DB Schema 229
PHP 7.1.11
MySQL 5.5.56-MariaDB
RRDTool 1.4.8
SNMP NET-SNMP 5.7.2
====================================

[OK] Database connection successful
[OK] Database schema correct
[WARN] Your local git contains modified files, this could prevent automatic updates.
[FIX] You can fix this with ./scripts/github-remove
Modified Files:
includes/polling/os/dlink.inc.php

I am honest in saying I may be a bit out of my depth here. I am not sure why the OID's do not match. What I do know is, if you for example look at the DGS-3120-24TC file's OID's, none of the OID's there returns a Serial Number, or a Hardware Version or FW version. But the changes I have made does display data like HW version and FW version correctly, and the temperatures are graphing as expected (with values as expected and physically seen with spot checks on the devices). I know this may not be good enough for keeping the code up to standard, and I understand that, but at the moment I have no other explanation.

Sorry again.

value: swTemperatureCurrent
num_oid: .1.3.6.1.4.1.171.12.11.1.8.1.2.
descr: Current Sys Temp
index: '{{ $index }}'

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

This should be index: 'swTemperatureCurrent.{{ $index }}'

oid: swTemperatureTable
value: swTemperatureCurrent
num_oid: .1.3.6.1.4.1.171.12.11.1.8.1.2.
descr: Current Sys Temp

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

If you get more than one result back here then the descr will not be unique so just change it to: descr: 'Current Sys Temp {{ $index }}'

*
* @package LibreNMS
* @link http://librenms.org
* @copyright 2017 Thomas GAGNIERE

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

It's 2018 :)

$memory_oid = '.1.3.6.1.4.1.171.12.1.1.9.1.4.1';
$perc = snmp_get($device, $memory_oid, '-OvQ');
if (is_numeric($perc)) {
$mempool['perc'] = $perc;

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

You can delete this line, it's calculated by the code.

@laf

This comment has been minimized.

Member

laf commented Jan 17, 2018

You're doing really well with it :) I've made some inline comments for a few things that need to be changed.

For the test data if you have the device in LibreNMS then run: ./scripts/save-test-data.php -h ID - Change ID for the device ID.

@hanserasmus

This comment has been minimized.

Contributor

hanserasmus commented Jan 18, 2018

@laf Thank you for this clear and precise indications of what you needed. I have made all the changes to the code you have commented on. I have not submitted it yet, I have just "patched" my current install with all the changes, and then ran the tests against that. I have done the snmpsim install as well now, so I am adding the json file for that result as well.

Only thing that happened was, after I have made the changes to the files, and ran the test script, my historic temperature data (and ONLY temperature data) in the graphs disappeared for the device (ID 14) on which I ran the test. It now just shows current temperature, and "nan" in the historic graphs. Don't know if I broke something, or if it is just coincidence. All other devices are still running as expected and graphing as expected (with a glance over).

Thanks again for the help and the trouble, I appreciate it!
TestData.zip

hanserasmus added some commits Jan 18, 2018

Update dlink.yaml
Changed values as per laf's request.
Update dlink.inc.php
Updated date
Update dlink.inc.php
Deleted line as per laf's request.
@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Jan 18, 2018

The inspection completed: No new issues

@laf

This comment has been minimized.

Member

laf commented Jan 18, 2018

Only thing that happened was, after I have made the changes to the files, and ran the test script, my historic temperature data (and ONLY temperature data) in the graphs disappeared for the device (ID 14) on which I ran the test. It now just shows current temperature, and "nan" in the historic graphs. Don't know if I broke something, or if it is just coincidence. All other devices are still running as expected and graphing as expected (with a glance over).

Is it still showing NaN?

@hanserasmus

This comment has been minimized.

Contributor

hanserasmus commented Jan 19, 2018

@laf No it has been creating new historic data, so that is not a problem anymore.

@laf

This comment has been minimized.

Member

laf commented Jan 19, 2018

@hanserasmus Ok are you happy if we merge this now or are you working on anymore bits?

@hanserasmus

This comment has been minimized.

Contributor

hanserasmus commented Jan 19, 2018

I am happy at the moment. Will work on some extra bits as I go a long (but will still be a while). All the patches seem to work well, so I am happy. Thanks for the trouble @laf

@murrant

This comment has been minimized.

Member

murrant commented Jan 21, 2018

Thanks @hanserasmus. Congrats :)

@murrant murrant merged commit 99f8728 into librenms:master Jan 21, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@laf

This comment has been minimized.

Member

laf commented Jan 21, 2018

image

@hanserasmus

This comment has been minimized.

Contributor

hanserasmus commented Jan 22, 2018

Hehe thanks you guys for putting up with me! :-)

@lock

This comment has been minimized.

lock bot commented May 16, 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 16, 2018

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