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.

Sign up for GitHub

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

net/vnstat: Handle multiple interfaces better #4443

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

net/vnstat: Handle multiple interfaces better #4443

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
4995dcb
add setting to scan for new interfaces
bensmithurst Jan 4, 2025
c8697dc
allow single interface only
bensmithurst Jan 4, 2025
2903e63
multiple interface support
bensmithurst Jan 4, 2025
0d849ef
remove vim marker
bensmithurst Jan 4, 2025
b59f9b0
use configdpRun for consistency here
bensmithurst Jan 4, 2025
81b8215
revert interface option changes
bensmithurst Jan 5, 2025
ced7a58
define new separate_stats option
bensmithurst Jan 5, 2025
371b95d
handle the separate_stats option
bensmithurst Jan 5, 2025
c217fbb
rewrite to avoid needing the stats script
bensmithurst Jan 5, 2025
aac8ec4
fix typo, add back full path to vnstat
bensmithurst Jan 5, 2025
5f53d28
improve comments, add basic phpdoc
bensmithurst Jan 5, 2025
17978c9
tweak help text
bensmithurst Jan 5, 2025
790583a
run "make style-model"
bensmithurst Jan 5, 2025
f733359
default to true for separate_stats
bensmithurst Jan 5, 2025
2bd5355
update config check to handle the default being true
bensmithurst Jan 5, 2025
dead6b0
Merge branch 'master' into vnstat-multi-interface
bensmithurst Jan 25, 2025
b49ae5c
fix XML formatting
bensmithurst Jan 25, 2025
027ec79
Merge remote-tracking branch 'origin/master' into vnstat-multi-interface
bensmithurst Jan 27, 2025
1b75246
Merge remote-tracking branch 'origin/master' into vnstat-multi-interface
bensmithurst Feb 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 26 additions & 12 deletions net/vnstat/src/opnsense/mvc/app/controllers/OPNsense/Vnstat/Api/ServiceController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

use OPNsense\Base\ApiMutableServiceControllerBase;
use OPNsense\Core\Backend;
use OPNsense\Core\Config;
use OPNsense\Vnstat\General;

/**
Expand All @@ -51,9 +52,7 @@ class ServiceController extends ApiMutableServiceControllerBase
*/
public function hourlyAction()
{
$backend = new Backend();
$response = $backend->configdRun("vnstat hourly");
return array("response" => $response);
return $this->getStatsForDisplayInterfaces('hourly');
}

/**
Expand All @@ -62,9 +61,7 @@ public function hourlyAction()
*/
public function dailyAction()
{
$backend = new Backend();
$response = $backend->configdRun("vnstat daily");
return array("response" => $response);
return $this->getStatsForDisplayInterfaces('daily');
}

/**
Expand All @@ -73,9 +70,7 @@ public function dailyAction()
*/
public function monthlyAction()
{
$backend = new Backend();
$response = $backend->configdRun("vnstat monthly");
return array("response" => $response);
return $this->getStatsForDisplayInterfaces('monthly');
}

/**
Expand All @@ -84,9 +79,7 @@ public function monthlyAction()
*/
public function yearlyAction()
{
$backend = new Backend();
$response = $backend->configdRun("vnstat yearly");
return array("response" => $response);
return $this->getStatsForDisplayInterfaces('yearly');
}

/**
Expand All @@ -99,4 +92,25 @@ public function resetdbAction()
$response = $backend->configdRun("vnstat resetdb");
return array("response" => $response);
}

private function getStatsForDisplayInterfaces(string $type) {
$config = Config::getInstance()->toArray();
$backend = new Backend();

if (!isset($config['OPNsense']['vnstat']['general']['interface_display'])) {
// no interface configured, use script default (i.e. don't specify interface)
$response = $backend->configdpRun("vnstat", [ $type ]);
return array("response" => $response);
}

// loop over configured interfaces, combining the output
$result = '';
foreach (explode(',', $config['OPNsense']['vnstat']['general']['interface_display']) as $interface) {
// map the OPNsense interface name to the kernel interface name that vnstat uses
if (isset($config['interfaces'][$interface]['if'])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is only correct for IPv4 or the easy interface cases. Ping me on Monday for how to gather complete stats across split IPv4/6 interface assignments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fichtner so, about this, what's a better way to do it? I checked the logic for how the vnstat.conf is generated and it seems to use similar logic to this, but using a macro

{%     for interface in OPNsense.vnstat.general.interface.split(',') %}
{%       do interfaces.append(physical_interface(interface)) %}
{%     endfor %}

where the macro is doing:

{% if helpers.exists('interfaces.'+name+'.if')
%}{{
      helpers.getNodeByTag('interfaces.'+name+'.if')
}}{%
else %}{{name}}{%
endif

.. which is largely the same as I'm doing, I think? If my logic in this PR is wrong, is this config logic also wrong? (I find it odd that the config logic falls back to the input value for unknown interfaces, rather than just ignoring them? Or is that needed for.. reasons?)

$result .= $backend->configdpRun("vnstat", [ $type, $config['interfaces'][$interface]['if'] ]);
}
}
return array("response" => $result);
}
}
10 changes: 8 additions & 2 deletions net/vnstat/src/opnsense/mvc/app/controllers/OPNsense/Vnstat/forms/general.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,14 @@
</field>
<field>
<id>general.interface</id>
<label>Interface</label>
<label>Default interface</label>
<type>dropdown</type>
<help>Set the default interface.</help>
</field>
<field>
<id>general.interface_display</id>
<label>Interfaces to display</label>
<type>select_multiple</type>
<help>Set the interface to listen on.</help>
<help>Set the interface(s) to display stats for.</help>
</field>
</form>
6 changes: 5 additions & 1 deletion net/vnstat/src/opnsense/mvc/app/models/OPNsense/Vnstat/General.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@
</enabled>
<interface type="InterfaceField">
<Required>N</Required>
<multiple>Y</multiple>
<multiple>N</multiple>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allegedly this worked in the past with multiple interfaces given as a single interface, but only up to a limit of 16/32 characters which may be the reason it doesn’t work for you. This is an upstream issue. Breaking this here for people who use it ok is not an option, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. My interface string was just igb0+ue0 (possibly reversed) so well within that limit. I'll dig into exactly how vnstat handles that config option later hopefully.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yes, pretty sure this used to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, some of this may have been caused by me trying to fix it before understanding that the missing AlwaysAddNewInterfaces was part of the problem, so that's my bad. Sorry for the noise. If the Interface directive contains an interface that doesn't exist in vnstat's DB, retrieving stats returns an error. So just adding AlwaysAddNewInterfaces would be enough to fix that.

However what I was hoping for (and what my change does) is to show stats for different interfaces separately. Getting a combined total for WAN+LTE isn't useful for my use case.

Would a PR which just adds AlwaysAddNewInterfaces and adds a new option called something like 'Separate interface stats' - implemented along the lines as I have done here, and defaulting off of course - and otherwise leaving the interface options alone, be acceptable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that sounds like a plan 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll get right on it. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR. I've removed the stats.sh wrapper I added, that was only needed because I'd misunderstood how the Interface option worked, so I've binned it.

</interface>
<interface_display type="InterfaceField">
<Required>N</Required>
<multiple>Y</multiple>
</interface_display>
</items>
</model>
11 changes: 11 additions & 0 deletions net/vnstat/src/opnsense/scripts/OPNsense/Vnstat/stats.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#!/bin/sh

type="$1"
interface="$2"

if [ -z "$interface" ]; then
# Interface not specified, use default interface for backward compatibility
interface=`awk '/^Interface/ { print $2 }' < /usr/local/etc/vnstat.conf`
fi

vnstat -$type $interface
16 changes: 8 additions & 8 deletions net/vnstat/src/opnsense/service/conf/actions.d/actions_vnstat.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,26 +23,26 @@ type:script_output
message:request Vnstat status

[hourly]
command:/usr/local/bin/vnstat -h
parameters:
command:/usr/local/opnsense/scripts/OPNsense/Vnstat/stats.sh h
parameters:%s
type:script_output
message:request Vnstat hourly status

[daily]
command:/usr/local/bin/vnstat -d
parameters:
command:/usr/local/opnsense/scripts/OPNsense/Vnstat/stats.sh d
parameters:%s
type:script_output
message:request Vnstat daily status

[monthly]
command:/usr/local/bin/vnstat -m
parameters:
command:/usr/local/opnsense/scripts/OPNsense/Vnstat/stats.sh m
parameters:%s
type:script_output
message:request Vnstat monthly status

[yearly]
command:/usr/local/bin/vnstat -y
parameters:
command:/usr/local/opnsense/scripts/OPNsense/Vnstat/stats.sh y
parameters:%s
type:script_output
message:request Vnstat yearly status

Expand Down
2 changes: 2 additions & 0 deletions net/vnstat/src/opnsense/service/templates/OPNsense/Vnstat/vnstat.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
Interface {{ interfaces|join('+') }}
{% endif %}

# scan for new interfaces and add to DB
AlwaysAddNewInterfaces 1

# location of the database directory
DatabaseDir "/var/lib/vnstat"
Expand Down