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

Cambium ePMP CPU reporting fix #7182

Merged
merged 2 commits into from Aug 21, 2017

Conversation

Projects
None yet
7 participants
@neg2led
Contributor

neg2led commented Aug 18, 2017

See https://community.librenms.org/t/cambium-epmp-cpu-usage-not-reporting-correctly/1877 for more details, and the comment below.

Pre-commit output;

librenms@orwell:~$ ./scripts/pre-commit.php
Running unit tests... success
Running style check... success
Running lint check... success
Tests ok, submit away :)

This is a very small change; one number changed from 100 to 10, and a division removed.

  • Remove division by 10

Removes unnecessary division by 10 from poller file; this is handled by processor_precision in the main include.

  • Correct processor_precision

Adjusted processor precision divider from 100 to 10 (as sysCPUUsage.0 reports from 0-1000 as 0-100.0%)

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

Commit merge to branch for Cambium CPU fix (#1)
* Remove division by 10

Removes unnecessary division by 10 from poller file; this is handled by processor_precision in the main include.

* Correct processor_precision

Adjusted processor precision divider from 100 to 10 (as sysCPUUsage.0 reports from 0-1000 as 0-100.0%)
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Aug 18, 2017

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

mention-bot commented Aug 18, 2017

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

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Aug 18, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Aug 18, 2017

CLA assistant check
All committers have signed the CLA.

@@ -21,7 +21,7 @@
'0',
'cambium-cpu',
$descr,
'100',
'10',

This comment has been minimized.

@murrant

murrant Aug 18, 2017

Member

This should be 1 shouldn't it?

@murrant

murrant Aug 18, 2017

Member

This should be 1 shouldn't it?

This comment has been minimized.

@neg2led

neg2led Aug 18, 2017

Contributor

This value is the "processor_precision" value in the database; includes/polling/processors.inc.php divides whatever the CPU-specific polling module returns by this number.

The raw value presented by the OID ranges from 0-1000 - a value of 50 is 5% - so the raw value needs to be divided by 10.

Currently, includes/polling/processors/cambium-cpu.inc.php divides the raw value by 10, and passes that value back to includes/polling/processors.inc.php;

if (is_numeric($usage)) {
$proc = ($usage / 10);
}

processors.inc.php then divides the return value by processor_precision;

$proc = round(($proc / $processor['processor_precision']), 2);

Giving us a total divisor of 1000, which we see in a poller run;

Processor Processor... Cambium CPU UsageSNMP[/usr/bin/snmpget -v2c -c COMMUNITY -OvQ -M /opt/librenms/mibs:/opt/librenms/mibs/cambium:/opt/librenms/mibs/cambium udp:HOSTNAME:161 CAMBIUM-PMP80211-MIB::sysCPUUsage.0]
50
 
0.05%

(0.05 being 1/1000th of 50)

This gives us two options;

  1. Remove the division from cambium-cpu.inc.php, and set processor_precision to 10
  2. Leave the division in cambium-cpu.inc.php, and set processor_precision to 1

I've opted for option 1 - a search indicates processor_precision is only used by processors.inc.php, and only for dividing the raw value returned from a sub-module, so it seems a bit redundant to move it out to the device-specific sub-module when functionality is already in place.

If there's a compelling reason to choose option 2, please let me know - the only reason I can think of is that it's modifying two files instead of one, which doesn't seem like a great reason.

@neg2led

neg2led Aug 18, 2017

Contributor

This value is the "processor_precision" value in the database; includes/polling/processors.inc.php divides whatever the CPU-specific polling module returns by this number.

The raw value presented by the OID ranges from 0-1000 - a value of 50 is 5% - so the raw value needs to be divided by 10.

Currently, includes/polling/processors/cambium-cpu.inc.php divides the raw value by 10, and passes that value back to includes/polling/processors.inc.php;

if (is_numeric($usage)) {
$proc = ($usage / 10);
}

processors.inc.php then divides the return value by processor_precision;

$proc = round(($proc / $processor['processor_precision']), 2);

Giving us a total divisor of 1000, which we see in a poller run;

Processor Processor... Cambium CPU UsageSNMP[/usr/bin/snmpget -v2c -c COMMUNITY -OvQ -M /opt/librenms/mibs:/opt/librenms/mibs/cambium:/opt/librenms/mibs/cambium udp:HOSTNAME:161 CAMBIUM-PMP80211-MIB::sysCPUUsage.0]
50
 
0.05%

(0.05 being 1/1000th of 50)

This gives us two options;

  1. Remove the division from cambium-cpu.inc.php, and set processor_precision to 10
  2. Leave the division in cambium-cpu.inc.php, and set processor_precision to 1

I've opted for option 1 - a search indicates processor_precision is only used by processors.inc.php, and only for dividing the raw value returned from a sub-module, so it seems a bit redundant to move it out to the device-specific sub-module when functionality is already in place.

If there's a compelling reason to choose option 2, please let me know - the only reason I can think of is that it's modifying two files instead of one, which doesn't seem like a great reason.

This comment has been minimized.

@laf

laf Aug 18, 2017

Member

I do sometimes worry about changes like this as it could have been ok for earlier firmware for the device and now Cambium have changed it so this would break things.

@pheinrichs are you able to comment on this to check as you added the original support?

@neg2led if this is approved by pheinrichs then I think you can actually now get rid of the polling file as it shouldn't be needed.

@laf

laf Aug 18, 2017

Member

I do sometimes worry about changes like this as it could have been ok for earlier firmware for the device and now Cambium have changed it so this would break things.

@pheinrichs are you able to comment on this to check as you added the original support?

@neg2led if this is approved by pheinrichs then I think you can actually now get rid of the polling file as it shouldn't be needed.

This comment has been minimized.

@pheinrichs

pheinrichs Aug 18, 2017

Contributor

This looks good to me, option 1 seems like the best way to go here. I've noticed ePMP haven't been reporting right but was to busy to look into. Happy to see a fix.

@pheinrichs

pheinrichs Aug 18, 2017

Contributor

This looks good to me, option 1 seems like the best way to go here. I've noticed ePMP haven't been reporting right but was to busy to look into. Happy to see a fix.

This comment has been minimized.

@laf

laf Aug 19, 2017

Member

Thanks for confirming. @neg2led can you remove the polling file as I think it's not needed.

@laf

laf Aug 19, 2017

Member

Thanks for confirming. @neg2led can you remove the polling file as I think it's not needed.

This comment has been minimized.

@neg2led

neg2led Aug 20, 2017

Contributor

Just confirmed this on my own install, removing the file entirely works great. Will update this :)

@neg2led

neg2led Aug 20, 2017

Contributor

Just confirmed this on my own install, removing the file entirely works great. Will update this :)

Delete cambium-cpu.inc.php
With the change to the discovery file (processor_precision set to the correct value of 10) this file is not required.
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Aug 20, 2017

The inspection completed: No new issues

scrutinizer-notifier commented Aug 20, 2017

The inspection completed: No new issues

@neg2led

This comment has been minimized.

Show comment
Hide comment
@neg2led

neg2led Aug 20, 2017

Contributor

Removed the cambium-cpu.inc.php file after confirming it's no longer required at all. And hey, the automated checks didn't fail with an unrelated error this time! Always a plus.

Contributor

neg2led commented Aug 20, 2017

Removed the cambium-cpu.inc.php file after confirming it's no longer required at all. And hey, the automated checks didn't fail with an unrelated error this time! Always a plus.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Aug 21, 2017

Member

Thanks for the clarification, I thought the oid returned 0-100.

Member

murrant commented Aug 21, 2017

Thanks for the clarification, I thought the oid returned 0-100.

@neg2led

This comment has been minimized.

Show comment
Hide comment
@neg2led

neg2led Aug 21, 2017

Contributor

Originally it was specified as 0-100 - but an implementation bug made it report 0-1000. When the bug was found, it was decided to change the spec rather than fix the implementation, as the additional precision was considered a plus (though not really necessary)

More info here; http://community.cambiumnetworks.com/t5/ePMP-2000-and-1000/High-CPU-Usage/td-p/42240

Contributor

neg2led commented Aug 21, 2017

Originally it was specified as 0-100 - but an implementation bug made it report 0-1000. When the bug was found, it was decided to change the spec rather than fix the implementation, as the additional precision was considered a plus (though not really necessary)

More info here; http://community.cambiumnetworks.com/t5/ePMP-2000-and-1000/High-CPU-Usage/td-p/42240

@laf laf merged commit 0ba3722 into librenms:master Aug 21, 2017

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.

Show comment
Hide comment
@laf

laf Aug 21, 2017

Member

Thanks for the fix @neg2led :D

Member

laf commented Aug 21, 2017

Thanks for the fix @neg2led :D

@neg2led neg2led deleted the neg2led:cambium-cpufix branch Aug 21, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

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

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

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