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

feature: add CPU and Memory pool BDCom Switchs #6523

Merged
merged 5 commits into from May 2, 2017

Conversation

Projects
None yet
6 participants
@kakopedreros
Contributor

kakopedreros commented Apr 28, 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.

Testers

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

Fixes: #6522

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 28, 2017

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

mention-bot commented Apr 28, 2017

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

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Apr 28, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Apr 28, 2017

CLA assistant check
All committers have signed the CLA.

@kakopedreros kakopedreros referenced this pull request Apr 28, 2017

Closed

feature: add CPU and Memory pool BDCom Switchs #6522

5 of 5 tasks complete
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Apr 28, 2017

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

@kakopedreros kakopedreros changed the title from Issue 6522 to feature: add CPU and Memory pool BDCom Switchs Apr 28, 2017

@laf

Some changes required but thanks for submitting the PR.

Show outdated Hide outdated includes/discovery/mempools/bdcom.inc.php
* @copyright 2017 - Carlos A. Pedreros Lizama <carlos.pedreros@gmail.com>
* @author Carlos A. Pedreros Lizama <carlos.pedreros@gmail.com>
.1.3.6.1.4.1.3320.9.48.1 nmsMemoryPoolUtilization.0 = [%][This is the memory pool utilization"]

This comment has been minimized.

@laf

laf Apr 28, 2017

Member

Please remove all the comments here, this is all in the mib mibs/pbn/NMS-MEMORY-POOL-MIB.MIB

@laf

laf Apr 28, 2017

Member

Please remove all the comments here, this is all in the mib mibs/pbn/NMS-MEMORY-POOL-MIB.MIB

This comment has been minimized.

@kakopedreros

kakopedreros Apr 28, 2017

Contributor

All the comments, including the license and author info?

@kakopedreros

kakopedreros Apr 28, 2017

Contributor

All the comments, including the license and author info?

This comment has been minimized.

@laf

laf Apr 28, 2017

Member

No, license and author are fine.

@laf

laf Apr 28, 2017

Member

No, license and author are fine.

This comment has been minimized.

@kakopedreros

kakopedreros Apr 29, 2017

Contributor

done.

@kakopedreros

kakopedreros Apr 29, 2017

Contributor

done.

Show outdated Hide outdated includes/discovery/mempools/bdcom.inc.php
if ($device['os'] == 'bdcom') {
echo 'BDCOM, NMS-MEMORY-POOL:';
$total = snmp_get($device, '.1.3.6.1.4.1.3320.9.48.2.0', '-OvQ');

This comment has been minimized.

@laf

laf Apr 28, 2017

Member

please use the named OID as we have the MIB available. It makes it easier to read

@laf

laf Apr 28, 2017

Member

please use the named OID as we have the MIB available. It makes it easier to read

This comment has been minimized.

@laf

laf Apr 28, 2017

Member

In fact you can also use snmp_get_multi or snmp_get_multi_oid in place of two gets.

@laf

laf Apr 28, 2017

Member

In fact you can also use snmp_get_multi or snmp_get_multi_oid in place of two gets.

Show outdated Hide outdated includes/discovery/processors/bdcom.inc.php
* @copyright 2017 - Carlos A. Pedreros Lizama <carlos.pedreros@gmail.com>
* @author Carlos A. Pedreros Lizama <carlos.pedreros@gmail.com>
.1.3.6.1.4.1.3320.9.109.1.1.1.1.1.1 nmspmCPUTotalIndex.1 = [Unsigned32][Index]

This comment has been minimized.

@laf

laf Apr 28, 2017

Member

Same comments for the comment data you have in here + using the named OID.

@laf

laf Apr 28, 2017

Member

Same comments for the comment data you have in here + using the named OID.

This comment has been minimized.

@kakopedreros

kakopedreros Apr 28, 2017

Contributor

if i use the named OID, i need to declare the mib_dir in the definition os file?

@kakopedreros

kakopedreros Apr 28, 2017

Contributor

if i use the named OID, i need to declare the mib_dir in the definition os file?

This comment has been minimized.

@laf

laf Apr 28, 2017

Member

Only if the mib dir is named differently from the os name, which it should if the vendor has multiple products so the mibs go in the vendor dir then.

@laf

laf Apr 28, 2017

Member

Only if the mib dir is named differently from the os name, which it should if the vendor has multiple products so the mibs go in the vendor dir then.

Show outdated Hide outdated includes/polling/mempools/bdcom.inc.php
* @copyright 2017 - Carlos A. Pedreros Lizama <carlos.pedreros@gmail.com>
* @author Carlos A. Pedreros Lizama <carlos.pedreros@gmail.com>
.1.3.6.1.4.1.3320.9.48.1 nmsMemoryPoolUtilization.0 = [%][This is the memory pool utilization"]

This comment has been minimized.

@laf

laf Apr 28, 2017

Member

Please remove comments again.

@laf

laf Apr 28, 2017

Member

Please remove comments again.

This comment has been minimized.

@kakopedreros

kakopedreros Apr 29, 2017

Contributor

done.

@kakopedreros

kakopedreros Apr 29, 2017

Contributor

done.

Show outdated Hide outdated includes/polling/mempools/bdcom.inc.php
*/
$total = snmp_get($device, '.1.3.6.1.4.1.3320.9.48.2.0', '-OvQ');

This comment has been minimized.

@laf

laf Apr 28, 2017

Member

Use named OIDs + multi get

@laf

laf Apr 28, 2017

Member

Use named OIDs + multi get

Show outdated Hide outdated includes/polling/mempools/bdcom.inc.php
echo 'BDCOM Memory Pool';
$mempool['total'] = snmp_get($device, '.1.3.6.1.4.1.3320.9.48.2.0', '-OvQ');

This comment has been minimized.

@laf

laf Apr 28, 2017

Member

You seemed to have duplicated the next two snmp calls.

@laf

laf Apr 28, 2017

Member

You seemed to have duplicated the next two snmp calls.

This comment has been minimized.

@kakopedreros

kakopedreros Apr 29, 2017

Contributor

done.

@kakopedreros

kakopedreros Apr 29, 2017

Contributor

done.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 1, 2017

Member

@kakopedreros I'm not seeing any of my requests having been changed yet, let us know if you are having issues.

Member

laf commented May 1, 2017

@kakopedreros I'm not seeing any of my requests having been changed yet, let us know if you are having issues.

@kakopedreros

This comment has been minimized.

Show comment
Hide comment
@kakopedreros

kakopedreros May 1, 2017

Contributor

I having problems with the named OID, with the mib files located in mibs/pbn folder doesnt work.
Maybe it is the mib files in this folder are incomplete or the MIB files from BDCom are in some way differents.

bdcom_nms_mibs.txt

What do you prefere, upload the NMs mibs provided by BDCom or only use numeric OID?

Contributor

kakopedreros commented May 1, 2017

I having problems with the named OID, with the mib files located in mibs/pbn folder doesnt work.
Maybe it is the mib files in this folder are incomplete or the MIB files from BDCom are in some way differents.

bdcom_nms_mibs.txt

What do you prefere, upload the NMs mibs provided by BDCom or only use numeric OID?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 2, 2017

Member

You can upload the mib as well, it's extremely useful and easier to read by using mibs.

Member

laf commented May 2, 2017

You can upload the mib as well, it's extremely useful and easier to read by using mibs.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented May 2, 2017

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

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 2, 2017

Member

@thanks for updating, I'll take a proper look later on but one thing I spotted was the OS logo has to be 32x32 if it's a png, can you please revert that unless you can swap it out to svg

Member

laf commented May 2, 2017

@thanks for updating, I'll take a proper look later on but one thing I spotted was the OS logo has to be 32x32 if it's a png, can you please revert that unless you can swap it out to svg

@kakopedreros

This comment has been minimized.

Show comment
Hide comment
@kakopedreros

kakopedreros May 2, 2017

Contributor

Replaced the png logo with a new logo in svg. Modified the processors discovery module. Detected an error in the variable type detection.

pre-commit.php: sucess

Contributor

kakopedreros commented May 2, 2017

Replaced the png logo with a new logo in svg. Modified the processors discovery module. Detected an error in the variable type detection.

pre-commit.php: sucess

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented May 2, 2017

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

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented May 2, 2017

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

@kakopedreros

This comment has been minimized.

Show comment
Hide comment
@kakopedreros

kakopedreros May 2, 2017

Contributor

one more thing...

This was tested in 136 BDCom switches... =)
captura de pantalla 2017-05-02 a la s 14 53 20

Contributor

kakopedreros commented May 2, 2017

one more thing...

This was tested in 136 BDCom switches... =)
captura de pantalla 2017-05-02 a la s 14 53 20

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier May 2, 2017

The inspection completed: No new issues

scrutinizer-notifier commented May 2, 2017

The inspection completed: No new issues

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 2, 2017

Member

I've just compressed the svg down so will be good after that.

Member

laf commented May 2, 2017

I've just compressed the svg down so will be good after that.

@laf

laf approved these changes May 2, 2017

@laf laf merged commit 233f8e8 into librenms:master May 2, 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

@kakopedreros kakopedreros deleted the kakopedreros:issue-6522 branch May 3, 2017

@kakopedreros kakopedreros restored the kakopedreros:issue-6522 branch May 3, 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.