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

Add module support for wrapper script calls #14055

Conversation

mwobst
Copy link
Contributor

@mwobst mwobst commented Jun 21, 2022

The scripts poller.php and discovery.php offer a module
option (-m), which may be used to specify specific modules
for polling/discovery, possibly for special (and thus faster) testing
or for example rediscovering the fdb table (on all hosts).

Until now, this was not possible with the python wrapper scripts.
Now they support a '-m' option, where comma separated module names
may be passed. This will currently only work with poller and discovery, though.

DO NOT DELETE THE UNDERLYING TEXT

Please note

Please read this information carefully. You can run ./lnms dev:check to check your code before submitting.

  • Have you followed our code guidelines?
  • If my Pull Request does some changes/fixes/enhancements in the WebUI, I have inserted a screenshot of it.
  • If my Pull Request makes discovery/polling/yaml changes, I have added/updated test data.

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.

The scripts poller.php and discovery.php offer a module
option (-m), which may be used to specify specific modules
for polling/discovery, possibly for special (and then faster) testing
or for example rediscovering the fdb table (on all hosts).

Until now, this was not possible with the python wrapper scripts.
Now they support a '-m' option, where comma separated module names
may be passed. This will currently only work with poller and discovery, though.
Also 'reduced' if-else-clause size at end of LibreNMS/wrapper.py
(do not call method at each branch, instead prepare a value for modules)
Also an additional comment sign before #
@murrant
Copy link
Member

murrant commented Jul 19, 2022

Not 100% sure of the use case here, wouldn't adding ./discover.php -h all -m fdb-tables to the cron work?

@mwobst
Copy link
Contributor Author

mwobst commented Jul 19, 2022

Not 100% sure of the use case here, wouldn't adding ./discover.php -h all -m fdb-tables to the cron work?

Working? Yes, all devices would be “asked” …
… but that would only start a single thread, which is way too slow when asking around 2000 devices. Please correct me if I’m wrong, but we want to start a python script for spawning multiple php processes at once, so that a single discovery run for e. g. the fdb-tables module finishes within a reasonable time.

@murrant
Copy link
Member

murrant commented Jul 19, 2022

Depending on how frequently you schedule the fdb discovery, it could end up with multiple running at the same time and should result in all being completed within that frequency depending on how consistent the time to run is.

Say you set fdb discovery to 5 minute intervals and it takes 30 minutes to complete all. This would result in 6 discovery processes running all the time and all devices being completed every 5 minutes.

I think the wrapper has a mechanism to prevent overlap, so if the process isn't completed within the interval, it is canceled. That is why I would suggest the php script directly for a rolling poll.

One difficulty with either approach is overloading the device CPUs. Multiple processes fetching SNMP can bring a device CPU down pretty easily. The easiest to trigger side effect of that is polling backing up and causing gaps in graphs.

If you think this is worthwhile, I have no problem with it. I just wanted to make sure you aware it isn't strictly necessary.

@mwobst
Copy link
Contributor Author

mwobst commented Jul 19, 2022

First an explanation:
I believe that the PHP scripts (both polling and discovery) work sequentially when using "-h all" … am I wrong about it? My motivation is parallelized processing of the devices.

Currently, we run the discovery for almost 2000 devices, with modules = arp-table, fdb-table + discovery-protocols, using 20 threads. It still takes around 18 minutes to finish. So a sequential attempt would take 20*18 minutes … okay, probably less, but still a lot of time. (The usual full discovery for newly added devices is still running, of course, in case you’re wondering)

Depending on how frequently you schedule the fdb discovery, it could end up with multiple running at the same time and should result in all being completed within that frequency depending on how consistent the time to run is.

Well, we also have a systemd unit (instead of using crontab) starting the "big" discovery, so overlapping is prevented that way. (Sadly, there doesn’t seem to be a distributed version of discovery like it is with polling.)

Say you set fdb discovery to 5 minute intervals and it takes 30 minutes to complete all. This would result in 6 discovery processes running all the time and all devices being completed every 5 minutes.

Yeah, with crontab this would be an issue, but alas, not with systemd (using timers).

I think the wrapper has a mechanism to prevent overlap, so if the process isn't completed within the interval, it is canceled. That is why I would suggest the php script directly for a rolling poll.

A "rolling poll"? :D

One difficulty with either approach is overloading the device CPUs. Multiple processes fetching SNMP can bring a device CPU down pretty easily. The easiest to trigger side effect of that is polling backing up and causing gaps in graphs.

With this you’re (sadly) right on track. Two of our pollers having long runtimes every hour (the current systemd unit interval), although often only two or one, and almost always the same ones (we use distributed polling) – there are some devices which are quite … challenged.

The thing, though, is that we need a regularly refreshed fdb table, and polling doesn’t seem to do this – at least this page doesn’t seem to list an appropiate option.

murrant
murrant previously approved these changes Jul 21, 2022
@murrant
Copy link
Member

murrant commented Jul 21, 2022

Systemd won't allow overlapping services via timer, but cron will.

Run 1: 1, 2, 3, 4, 5, 6, 7, 8
Run 2:          1, 2, 3, 4, 5, 6, 7, 8
Run 3:                   1, 2, 3, 4, 5, 6 ,7, 8
Run 4:                            1, 2, 3, 4, 5, 6, 7, 8
Run 5:                                     1, 2, 3, 4, 5, 6, 7, 8

So if you used cron, you would effectively have 3 parallel discoveries running. Because discovery doesn't update rrd data (thereby having no deadline), this is absolutely no problem.

The following cron entry would discover those three modules every 5 minutes. The process taking longer than 5 minutes is inconsequential for your particular use case.

*/5 * * * * librenms /opt/librenms/discovery.php -h all -m arp-table,fdb-table,discovery-protocols >> /dev/null 2>&1

Additionally, to improve your performance, you may want to disable those modules from the regular discovery process.
lnms config:set discovery_modules.arp-table false
lnms config:set discovery_modules.fdb-table false
lnms config:set discovery_modules.discovery-protocols false

A real fix for what you want may be more complex per-module scheduling. But that is currently a long way off.

@PipoCanaja
Copy link
Contributor

Hi @mwobst and @murrant
Shouldn't we add a little bit of documentation with this very good feature ?

@mwobst
Copy link
Contributor Author

mwobst commented May 26, 2023

I added some (short) documentation to doc/Support/Discovery Support.md – the option usage is the same as with discovery.php, so not many words are required.

@electrocret electrocret merged commit 0e952b9 into librenms:master Jul 20, 2023
9 checks passed
@mwobst mwobst deleted the tud-zih/22.4-feature_wrapper_with_module_option branch July 20, 2023 07:54
TheMysteriousX pushed a commit to TheMysteriousX/librenms that referenced this pull request Aug 9, 2023
* Add module support for wrapper script calls

The scripts poller.php and discovery.php offer a module
option (-m), which may be used to specify specific modules
for polling/discovery, possibly for special (and then faster) testing
or for example rediscovering the fdb table (on all hosts).

Until now, this was not possible with the python wrapper scripts.
Now they support a '-m' option, where comma separated module names
may be passed. This will currently only work with poller and discovery, though.

* Replace single quotation signs with double ones (empty strings only)

* Fix more code lines (quotation signs, indentation)

Also 'reduced' if-else-clause size at end of LibreNMS/wrapper.py
(do not call method at each branch, instead prepare a value for modules)

* Add commas after last parameter of dict+methode

Also an additional comment sign before #

* Fix two leftover single quot. signs …

* doc: Add documentation for module support
@librenms-bot
Copy link

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/23-8-0-changelog/22078/1

peejaychilds pushed a commit to peejaychilds/librenms that referenced this pull request Oct 26, 2023
* Add module support for wrapper script calls

The scripts poller.php and discovery.php offer a module
option (-m), which may be used to specify specific modules
for polling/discovery, possibly for special (and then faster) testing
or for example rediscovering the fdb table (on all hosts).

Until now, this was not possible with the python wrapper scripts.
Now they support a '-m' option, where comma separated module names
may be passed. This will currently only work with poller and discovery, though.

* Replace single quotation signs with double ones (empty strings only)

* Fix more code lines (quotation signs, indentation)

Also 'reduced' if-else-clause size at end of LibreNMS/wrapper.py
(do not call method at each branch, instead prepare a value for modules)

* Add commas after last parameter of dict+methode

Also an additional comment sign before #

* Fix two leftover single quot. signs …

* doc: Add documentation for module support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants