openwisp/openwrt-openwisp-monitoring

OpenWRT monitoring agent for openwisp-monitoring

openwrt-openwisp-monitoring's Issues

[lua-monitoring] Merge the two scripts

Merge the two scripts netjson-monitoring.lua and netjson-monitoring-wan.lua

[Openwrt] cannot find dependency openwisp-config for openwisp-monitoring

Hi,

openwisp-config has been working fine on OpenWrt-21.02.

Config:

netjson-monitoring=y
openwisp-config-openssl=y
openwisp-monitoring=y

Error:

Collected errors:
 * pkg_hash_check_unresolved: cannot find dependency openwisp-config for openwisp-monitoring
 * pkg_hash_fetch_best_installation_candidate: Packages for openwisp-monitoring found, but incompatible with the architectures configured
 * opkg_install_cmd: Cannot install package openwisp-monitoring.
 make[2]: *** [package/Makefile:69: package/install] Error 255

[openwrt] Store data temporarily if HTTP request is unsuccessful

If daemon fails to send data to the server, store data in temporary directory with its date and time

Update:-

• Break the procd service in two services, one to collect data and other to send data
• Have a retry mechanism in sending data (exponential backoff) link 1 link 2
• #45

[enhancement] Switch indentation from spaces to tabs

I know this is annoying, but in an embedded system every byte counts.
Because every byte must also be interpreted.
Therefore, tabs should always be used by shell scripts.
https://github.com/openwisp/openwrt-openwisp-monitoring/blob/gsoc21/openwrt-openwisp-monitoring/files/monitoring.agent
https://github.com/openwisp/openwrt-openwisp-monitoring/blob/gsoc21/openwrt-openwisp-monitoring/files/monitoring.init
This is also the way it is done throughout OpenWrt.

[change] Turn netjson_monitoring into proper shell tool

From my point of view, this is a script that only is used by openwisp-monitoring.
To avoid polluting the system path, it is usual to move such scripts to /usr/libexec.
I would create a separate subdirectory named openwisp in this location and would drop this file there.

See #59 (comment)

[bug] get_neighbors() may fail with "Interrupted system call"

lua: /usr/sbin/netjson-monitoring:67: Interrupted system call
stack traceback:
	[C]: in function '(for generator)'
	/usr/sbin/netjson-monitoring:67: in function 'get_ip_neigh'
	/usr/sbin/netjson-monitoring:84: in function 'get_neighbors'
	/usr/sbin/netjson-monitoring:181: in main chunk
	[C]: ?

I need to test this on more devices before ruling out if it's a system issue or something that has to be dealt with in the code.

[change] Switch to iwinfo to get associated wifi clients

The current method we use to get clients associated to a WiFi interface only works for access point mode, it doesn't work for mesh (802.11s), an most probably also not for adhoc mode.

I verified that ubus call iwinfo assoclist '{"device": "<interface_name>"}' works also in 802.11s, so we can switch to this instead.

[lua-monitoring] Reindent lua

Reindent lua code in a consistent way (4 spaces).

[bug] Bridge members shown even if they do not exist

In OpenWRT it's possible to define bridge members that do not exist, they'll be added only if they exist and be ignored otherwise.
This is pretty handy in OpenWISP because it's possible to define different bridge members which get added only if the right template is used.

@devkapilbansal can you change the code so that only existing bridge members are collected in the NetJSON monitoring output?

EG:

{
    "name": "br-wifi",
    "bridge_members": [
        "wlan0",
        "wlan1",
        "tap_tcp",
        "tap_tcp_nocryp",
        "tap_udp",
        "tap_udp_nocryp"
    ]
}

In the case above, these members do not exist and therefore should not be added:

• "tap_tcp_nocryp",
• "tap_udp",
• "tap_udp_nocryp"

[enhancement] Do not make openwisp lua script executable

As I see it, the scripts do not have to be executable that are under /usr/lib/lua/openwisp on the system.
Please replace INSTALL_BIN with INSTALL_DATA in the openwrt makefile install routine.

I also noticed that the openwsip-agent also has lua scripts under /usr/lib/lua/openwisp.
Does it make sense to use a separate directory?

[change] ModemManager device option needs to update to ctl_device

Hey,

Just letting you know, we are changing this from device to ctl_device upstream in the OpenWrt ModemManager protocol handler because of compatibility issues with DSA. I will link the PR here when it is in.

openwrt-openwisp-monitoring/openwrt-openwisp-monitoring/files/lib/openwisp_monitoring/interfaces.lua

Line 21 in 261239e

local modem = uci_cursor.get('network', interface['interface'], 'device')

[refactor] Refactor script with a main and help function

This change is requested here:-

Can you please refactoring the script that it has more have a look and feel of a C programm with a main and help function?
Have a look at https://github.com/openwrt/packages/blob/master/net/mwan3/files/usr/sbin/mwan3rtmon thats only an example.
From my point of view, this has certain advantages. If you later decide to make a C program out of it, then you already know what you need for functions. My experience as a maintainer has often shown me that.

Also add a valid license header

#25 (comment)

Avoid hard dependencies, document dependencies

Current dependencies:

• lua-cjson
• rpcd-mod-iwinfo

We may replace cjson with some other library already available on OpenWRT (asked for a suggestion in the OpenWRT forum).

Make sure that if the package rpcd-mod-iwinfo is not installed, these two lines won't fail:
https://github.com/openwisp/lua-monitoring/blob/master/netjson-monitoring.lua#L398-L399
Ideally we should check for the presence of the feature before the loop, and skip the loop if the feature is not available.

We should add a brief README which should tell users to install package X.

[ci] Upload checksum value for packages

Since we have already added a functionality to upload packages directly to downloads.openwisp.io, it will be beneficial to also upload checksums for built packages.

This way users will be able to verify the authenticity of downloaded packages.

[docs] Add info about verbose mode in README

Mention in Debugging section about verbose mode

[openwrt] Make wwan modem backend interchangeable

Up to now only the modem manager is supported to get information from them wwan modem. Not all system does have a modem manager instance running. There are the following other backends

• uqmi
• mbim
• 3g

So far, there is no uniform way to read the information from these different backends. The needed information are not all the same. We would have to agree on the smallest common set here.

I would suggest an ubus interface that provides the necessary information. Then there would be no need to go to the trouble of collecting this information from the different backends system. The ubus interface then takes over the collection of the information. The openwisp-monitoring then only calls the ubus endpoint and returns what is there.

[ci] Add Github CI workflow

• Add runbuild script

• Add LuaCov for test coverage

• Add an install script (to install openwrt dependencies in container)

[lua-monitoring] Find out ip address information of each interface

This issue depends on openwisp/openwisp-monitoring#30.

In this script:
https://github.com/openwisp/lua-monitoring/blob/master/netjson-monitoring.lua

For each monitored interface, we have to find out the ip addresses.
Once we get the list of IP, for each ip we can check the configuration of that interface to see if any static address is configured, and find out if the IP we got matches the static IP configured, if yes, that's a static address and we'll flag it as such, otherwise it's most probably an address got in DHCP client mode or manually configured, we can further check the configuration to see if the interface is flagged as proto dhcp, if yes we'll flag the address as dhcp, otherwise we'll flag it as static.

[bug] Agent exits if device hasn't registered to openwisp controller yet

If the device is not registered yet, the agent exits:

openwisp-monitoring: OpenWISP monitoring daemon stopping
openwisp-monitoring: uuid or key are not set, please add these to /etc/config/openwisp

This is wrong, because it means users will have to restart the agent manually each time they register a device, which is not what we want.

The agent needs to be smart enough and figure out that if there's a shared secret in the openwisp configuration, it means that openwisp-config has not registered yet but it should not exit like that.

We have to introduce a mechanism which allows the agent to wait for configuration changes and start automatically after the device registers.

I wonder if we can somehow force the openwisp-monitoring agent to automatically restart whenever the config of openwisp-config changes? @feckert do you know if this is possible?

Send data directly using SIGUSR1 signals

[bug] bridge_members, validator says:{} is not of type 'array'

This was a bug that I fixed in the early version of this package, it seems to have popped up again:

Invalid data in "#/interfaces/4/bridge_members", validator says:\n\n{} is not of type 'array'

This happens when there's an empty bridge, the validator expects an empty list but since in lua there's no difference between list and objects, both are represented with tables, so an empty table is converted to JSON as {} which is not accepted to the validator.

The solution I had used was: fa69556.

[ci] Create symbolic link "latest" after uploading compiled package

After uploading the compiled package to google cloud, we shall remove any latest symbolic link and create a new one pointing to the directory just uploaded.

PS: it looks like GC does not support symbolic links (yuck), we shall then just create a copy of the last compilation called "latest".

Write lua tests

We need to write tests for the lua functions.

I hope we can use mocking to simulate the calls to the network programs we use to get the data (ubus, arp, etc).

[bug] Agent crashes when using monitored_interfaces "wlan0 wlan1 br-lan"

How to reproduce the bug:

• configure monitored_interfaces to be something like wlan0 wlan1 lan1 lan2 lan3 eth0 wan, eg:

config monitoring 'monitoring'
        option interval '300'
        option verbose_mode '0'
        option required_memory '0.05'
        option max_retries '5'
        option monitored_interfaces 'wlan0 wlan1 lan1 lan2 lan3 eth0 wan mesh0 mesh1'

• restart the agent

Expected outcome: I expected the agent to work normally.

Actual outcome: the agent doesn't start, it gives this error:

Fri Apr 22 20:30:00 2022 daemon.err openwisp-monitoring[23774]: missing required --mode option
Fri Apr 22 20:30:00 2022 daemon.err openwisp-monitoring[23775]: root: No data file found to send.
Fri Apr 22 20:30:05 2022 daemon.err openwisp-monitoring[23802]: missing required --mode option
Fri Apr 22 20:30:10 2022 daemon.err openwisp-monitoring[23803]: missing required --mode option
Fri Apr 22 20:30:15 2022 daemon.err openwisp-monitoring[23804]: missing required --mode option
Fri Apr 22 20:30:20 2022 daemon.err openwisp-monitoring[23834]: missing required --mode option
Fri Apr 22 20:30:26 2022 daemon.err openwisp-monitoring[23843]: missing required --mode option
Fri Apr 22 20:30:26 2022 daemon.info procd: Instance openwisp-monitoring::openwisp-monitoring_collect_data s in a crash loop 6 crashes, 0 seconds since last crash

[lua-monitoring] Use POST for sending metrics

See openwisp/openwisp-monitoring#35

[feature] Allow recognizing wireguard interface as of type virtual

If the proto address of an interface is wireguard, we can consider its type to be "virtual".

[enhancement] Send offline data collected to server

Depends on #29 openwisp/openwisp-monitoring#298

• Check if temporary data is available to send
• Send data to server with explicit datetime
• Delete data once request is successfule

Update:-

• Specify in URL if data sent is old or new (no need to update device status at each point)

[refactor] Improve curl response mesage

This change was originally requested here:
#42 (review)

Logged response can be improved
Here's an example output: Data not sent successfully. Response code is {"detail":"Authentication credentials were not provided."}403.. The message is readable but it's weird. The response body is {"detail":"Authentication credentials were not provided."} while the response status code is 403. Can you turn this into something like:
The response received was: 403 {"detail":"Authentication credentials were not provided."}?

[optimize] Use gzip to compress data

We can reduce the use of memory of more than 70% thanks to gzip.

We should just do the following:

• a basic gzip is provided by busybox, this should be enough, I am not sure whether we should specify this in the dependencies or not, we should double check this
• gzip files after saving them, eg: gzip $path, this command also removes the original file
• decompress files before sending the data, eg: gzip -d $path

[bug] Data not sent successfully. Exit code is 127

While testing the daeamon I get the following:

Mon Jul  5 00:23:06 2021 daemon.info monitoring: Collecting NetJSoN Monitoring data
Mon Jul  5 00:23:06 2021 daemon.err monitoring: Data not sent successfully. Exit code is 127

First of all, can you please change any occurrence of output or docs mentioning "NetJSON" which is not spelled this way and end ensure follows this spelling?

Then, Data not sent successfully. Exit code is 127 is not super helpful in finding out what's wrong.

What steps can we take in order to improve the life of users which will be using this package?
Can we include the code line and command which are failing?

Document how to install

We are now using this template: https://gist.github.com/nemesisdesign/cb005dc80a75536cf8537105c3038a71

Steps:

1. Create template in admin page
2. Mark "enable by default"
3. Associate a openwisp device

Also document how to add the template to an already registered device

[lua-monitoring] Monitor addresses of all interfaces by default

Monitor addresses of all interfaces by default, but do not include traffic statistics unless explicitly configured.

I also want to make the monitoring information regarding addresses more complete by using:

nixio = require('nixio')
cjson = require('cjson')
print(cjson.encode(nixio.getifaddrs()))

Because ubus call network.interface dump only gives partial information, although it's very useful as well, so I think I will try to merge the two.

Possible new algorithm:

• call ubus call network.interface dump, build a dictionary of all the interfaces and their addresses
• call nixio.getifaddrs(), update the dictionary created previously and add only new information
• for each interface included in the list of interfaces passed in the first argument to netjson-monitoring.lua, call also ubus call network.device status '{"name": "<NAME>"}' to get additional information and traffic statistics
• prepare the netjson output, including statistic/traffic information only for the interfaces specified by configuration

[qa] Add quality assurance checks

[change] Invert upload/download on interfaces that are member of a bridge

In Linux, bridged interface show tx/rx packets in reverse: download is upload and upload is download.

I couldn't find a canonical reference on why this is the way it is, if anyone finds it, let me know.

Now the problem we have is that in the traffic graph, the upload is greater than the download on wifi interfaces that are part of a bridge, which most users will consider as a bug.

So I think we should do the following:

• whenever an interface is flagged as an interface which should be traffic monitored, we should check whether this interface is part of a bridge
• if it is, we should simply invert rx and tx

Addendum: I'm trying to do some more research on the subject before proceeding with this: https://forum.openwrt.org/t/inverted-tx-rx-packets-in-bridged-interfaces/85093.

[bug] get_cpus may fail sometimes

root@E4-C3-2A-90-84-97:/usr/sbin# ./netjson_monitoring '*'
lua: ./netjson_monitoring:226: bad argument #2 to 'tonumber' (integer expected, got string)
stack traceback:
	[C]: in function 'tonumber'
	./netjson_monitoring:226: in function 'get_cpus'
	./netjson_monitoring:260: in main chunk
	[C]: ?

Still not sure why this error is occuring, will try to test more until I got some satisfactory results

[docs] Add package installation instructions

Mention that pre-build packages can be downloaded and installed from https://downloads.openwisp.io

Convert into OpenWRT package

We need to convert this into an OpenWRT package with compilation instructions in the README.

It could be named openwrt-netjson-monitoring or openwrt-openwisp-monitoring.

[enhancement] Move logger call into subroutine

The logger shell call is used over and over again.
Which makes the code confusing.
I would suggest that you move the call to a subroutine.

https://github.com/openwisp/openwrt-openwisp-monitoring/blob/master/openwrt-openwisp-monitoring/files/monitoring.init
https://github.com/openwisp/openwrt-openwisp-monitoring/blob/master/openwrt-openwisp-monitoring/files/monitoring.agent

[lua-monitoring] Find out DCHP lease information

This issue depends on openwisp/openwisp-monitoring#33.

In this script:
https://github.com/openwisp/lua-monitoring/blob/master/netjson-monitoring.lua

Find out where the lease file is stored, (uci command is uci get dhcp.@dnsmasq[0].leasefile, this has to be translated into the lua way of doing it).

Parse the lease file to produce the output described in openwisp/openwisp-monitoring#33 (see also https://serverfault.com/a/786141/91152 for a good explanation of the contents of the lease file).

[lua-monitoring] Send ARP table information

This issue depends on openwisp/openwisp-monitoring#34.

In this script:
https://github.com/openwisp/lua-monitoring/blob/master/netjson-monitoring.lua

Execute the arp command and parse its output to produce the output described in openwisp/openwisp-monitoring#34

[tests] Add tests for shell scripts

I am not sure but there should be some ways to write unittests for monitoring agent or monitoring daemon too.

We can research about the possibilities and try to cover these too

[enhancement] Add debug mode in monitoring config

Yes, I remove -t flag when logging in verbose mode. In verbose_mode, it is automatically tagging the logged message by the procd service name and if I pass a tag, then the message is logged twice

What do you think about adding a new "debug mode" to turn this on?

openwrt-openwisp-monitoring/openwrt-openwisp-monitoring/files/monitoring.init

Line 89 in bac6d54

[ "$verbose_mode" -eq "1" ] && procd_set_param stdout 1 && procd_set_param stderr 1

In verbose mode we could avoid using it. In debug mode we would know (and document) that any output is sent to the logging facility, causing some output to be repeated, Or alternatively, we could even modify our log function to use echo instead of logger.

See #57 (comment)

[ci] Delete old builds

@devkapilbansal is there a way to remove the old builds?
I think we can keep stable releases (which we shall name differently), but we should highly limit the build results because space has a cost, I think we can keep the last 5 builds.

[qa] Add a linter/checker for shell scripts

An autolinter to lint the shell script codes should be added to maintain consistency

[enhancement] Spilt netjson-monitoring in small plugin parts

This change was requested here: 👇
#25 (comment)

In general I would split the netjson into small plugin parts. The problem I see here is that not all have all related information on the system, so we should not check on every run if we have a wlan or a cellular. I do not have for example a cellular on my system. So why should the openwisp-monitoring always try to check the cellular backend.

[qa] Format lua files

Format lua files to be consistent with openwisp-config in general and following lua best practices

• Whitespace around operators
• Search for a lua autolinter
• Add documentation about it in README

[change] Status: if bridge member is not present as interface, remove it

When collecting device status from OpenWrt, the bridge member list may include members of interfaces which are not active.

Before adding the interface to the bridge members, it would be great to ensure the interface is present and active.

[refactor] Use utility function for calling popen() properly

It would be good to create a utility function which does the read, close and returning of the output when using io.popen().

This would also ensure that whoever will put their hands on this code will keep reusing our function and avoid messing it up again by forgetting to close a pipe.

[enhancement] Delete old data if required memory is not available

In #34 we are storing data in temporary storage if specific percentage(required_memory) of total memory is available.

But it may be possible that the server is not reachable for a long time and memory is filled with the json data files.
In that case, previous data files should be deleted to store the new one.

[bug] Modem-manager: do not send "--"

I just found out some output from a device like the following:

"mobile": {
    "operator_name": "xxxx",
    "power_status": "on",
    "operator_code": "xxxx",
    "model": "xxxx",
    "signal": {
        "umts": {
            "ecio": -5,
            "rscp": "--",
            "rssi": -69
        }
    },
    "manufacturer": "xxxx",
    "imei": "xxxx",
    "connection_status": "connected"
},

I guess we should either set null instead of "--" or we shouldn't include the attribute at all.

The server returns error because it expects a number. It returns an error also with null.