Skip to content
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 Wireless discovery to Huawei Vrp #9516

Merged
merged 16 commits into from May 19, 2019

Conversation

Projects
None yet
2 participants
@PipoCanaja
Copy link
Contributor

commented Dec 3, 2018

A large set of Huawei VRP devices (switches) can become Wireless Lan Controllers, as well as the AC (Access Controller) family.
This PR detects any answer in the WLAN mibs of Huawei and gather some data.

Please let me know if I am going the right direction. I tried to use the LibreNMS/OS/xxxx with classes and co, and was partly successfull. WirelessSensors are discovered that way.
But doing so for accesspoints polling would result in multiple time polling the same OIDs without caching capabilities (I use snmpwalk_group which does not cache) for every class, which would be very expensive in time.

So I went back to the ciscowlc route where polling is done in includes/polling/os/xxxx.inc.php. If a cleaner way is possible without rewriting all the access_points part of the code, I am interested ;)

PipoCanaja

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
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@PipoCanaja PipoCanaja changed the title WIP Vrp Wireless Lan Controller WIP Huawei Vrp - Wireless Lan Controller Dec 4, 2018

@murrant

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

FYI, the access points code is due for a rewrite to bring it in line with the Wireless code. But considering I don't have a controller like this, I did not have working knowledge and lost interest and never completed it.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2018

@murrant
It makes sense. I'll go with current philosophy first to get something working, and will improve it as it goes.
My questions for the moment are :

  • The snmpwalk_group() function is great whenever you get data indexed by mac-address or other funny types but it does not seems to be cached. Any other way to get the same kind of walk with caching enabled ?
  • So everytime you call it, it costs time in the discovery (not too bad) or polling (bad 🤒 ). In order to avoid this, I try to do it only once, which is not compatible with the multiple "interfaces" and methods of the Wireless code. A way to get around this could be to have some "pre_cache" method called for the device, where the snmpwalk_group is called, and that creates data freely accessible from all the interface methods later.
@murrant

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

@PipoCanaja you can cache data on the OS, it persists. Check the getCacheByIndex() function to see. Feel free to implement that on the specific OS class, and we can pull it up if it is useful to others.

Basically the goal is to get rid of pre_cache as it is unconditional, so even if the data isn't needed, it is fetched. getCacheByIndex() the first one that calls it fetches the data and caches it, next one(s) get the cached.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

I'll try to see how it goes with the snmpwalk_group part, because the fact that OIDs are converted into MAC Addresses and back, makes it more difficult to cache. This job is done directly by snmpwalk (with the proper option).

@murrant murrant added the Device 🖥 label Dec 12, 2018

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

Hello @murrant,

The function "public function getCacheTable($oid, $mib = null)" seems the be very close to my needs. I only miss the capacity to give it the depth parameter to influence the indexing, parameter that I can give to snmpwalk_group. Exemple :

$vapInfoTable = snmpwalk_group($this->getDevice(), 'hwWlanVapInfoTable', 'HUAWEI-WLAN-VAP-MIB', 3, array());

The "3" at the end allow snmpwalk_group to properly index a table with multiple indexes (depth).

And upgrading "getCacheTable" to pass the option is also not so easy, cause we should also add a way in the cache hash to store the value of the option. If we don't, then the same OID being accessed twice in the code, but with different depth values in snmpwalk_group parameters, would certainely break.

@murrant

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

Indeed, but I think when you access a table the "group" parameter is determined by the structure of that data, not how you want to use it. So it should be common that the group depth is the same for the same data.

What I'm trying to say is maybe we don't add that hashing complexity until it is needed.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

Theoretically, you are right ;)
The only question I have is how to troubleshoot the issue that would appear in case we have somebody accessing the same OID with the group but different depths.
I'll give it a try, sometimes during the Xmas break. And the discussion will continue afterward.

@murrant

This comment has been minimized.

Copy link
Member

commented Dec 22, 2018

Another option is not create a helper function. You can cache any data you want in the OS object. It should stay there for the duration of the poll.

@PipoCanaja PipoCanaja force-pushed the PipoCanaja:VRP_WirelessLanController branch from 831a4d1 to 8396f02 Jan 9, 2019

@PipoCanaja PipoCanaja force-pushed the PipoCanaja:VRP_WirelessLanController branch from 8396f02 to 6e50f56 Jan 21, 2019

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

Hello @murrant
I am right now testing a variant where the cache hash of the OS object is also hashed with the type of data (type of snmpwalk function and depth if applicable).
What do you think of this approach?

(it should probably be done in a cleaner way but testing the idea for the moment).

@PipoCanaja PipoCanaja force-pushed the PipoCanaja:VRP_WirelessLanController branch from 2ff59f8 to 8fdc7b6 Jan 23, 2019

@PipoCanaja PipoCanaja force-pushed the PipoCanaja:VRP_WirelessLanController branch from 8fdc7b6 to 263e0b5 Mar 1, 2019

* @return array array indexed by the snmp index with the value as the data returned by snmp
*/
public function getCacheTable($oid, $mib = null)
public function getCacheTable($oid, $mib = null, $depth = null)
{
if (str_contains($oid, '.')) {
echo "Error: don't use this with numeric oids!\n";
return null;
}
if (!isset($this->cache[$oid])) {

This comment has been minimized.

Copy link
@murrant

murrant Mar 14, 2019

Member

This isset doesn't match the lines below.
You shouldn't need $depth as a key right? Snmp tables have a fixed number of items in the index of a table.

This comment has been minimized.

Copy link
@PipoCanaja

PipoCanaja Apr 1, 2019

Author Contributor

Unless you can be sure that nobody will ever call the function twice with different $depth ...
The point here is that for complex tables, you may have a situation where you want to see them with different values at different places of the code (to loop over it in a different way). Having this cached without $depth as a key, means that a piece of code my ask for a depth and receive a cached value with another depth (and fail ...).
I agree this is probably not so often, but it would be fairly difficult to troubleshoot. As a security and for coherence of the result of the cached value compared to the "non cached" equivalent, a nested hastable does not cost that much CPU (LibreNMS waits for IO 99% of the time (Network IO, Disk IO ...)).

Nice catch for the isset, missed it ! :(

This comment has been minimized.

Copy link
@murrant

murrant Apr 9, 2019

Member

Depth is dictated by the how many values are in the index of the snmp table (you can see this in the mib). This really can't change.

This comment has been minimized.

Copy link
@PipoCanaja

PipoCanaja Apr 9, 2019

Author Contributor

Caching the depths values acts as a check. The same reason we check parameters in a function call. Useless until somebody calls the function with the wrong parameters :)
$depth as key can't be wrong and does not cost much of memory/performance. Not caching it exposes a risk if the function is misused (on purpose or by mistake).

If you prefer that I remove it, I will of course.

@PipoCanaja PipoCanaja self-assigned this Mar 15, 2019

@PipoCanaja PipoCanaja force-pushed the PipoCanaja:VRP_WirelessLanController branch 2 times, most recently from 68ca90a to addaf51 Apr 1, 2019

@PipoCanaja PipoCanaja force-pushed the PipoCanaja:VRP_WirelessLanController branch from addaf51 to 27668a3 Apr 9, 2019

@PipoCanaja PipoCanaja changed the title WIP Huawei Vrp - Wireless Lan Controller Added Wireless discovery to Huawei Vrp Apr 9, 2019

Show resolved Hide resolved LibreNMS/OS.php Outdated

PipoCanaja and others added some commits Apr 13, 2019

@murrant
Copy link
Member

left a comment

Looks good

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

OK. I'll probably update that when my devices (currently in the lab and testing phase) are moved to production, but this will take another couple of months so let's go.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

@murrant OK for me to merge.

@murrant
Copy link
Member

left a comment

Looks good :)

@murrant murrant merged commit 393b0e6 into librenms:master May 19, 2019

5 of 6 checks passed

codeclimate 9 issues to fix
Details
Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

spencerbutler added a commit to spencerbutler/librenms that referenced this pull request May 21, 2019

Added Wireless discovery to Huawei Vrp (librenms#9516)
* HUAWEI Wireless MIB files

* Adding support for Wireless Controller capabilities of Huawei VRP Switches

* CodeClimate Fixes

* Add clientPerRadio polling for VRP

* Cache snmpwalk_group and  snmpwalk_cache_oid separately

* Travis

* Travis

* Cleaning sensors not available

* isset corrected

* tests

* clean

* default value for depth

* Update Vrp.php

* Update Vrp.php

* Update vrp.inc.php

* Update vrp_ac6605-26.json

@lock lock bot locked as resolved and limited conversation to collaborators Jul 18, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.