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 15 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 all commits
Commits
Show all changes
15 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
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
44 changes: 32 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->getStats('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->getStats('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->getStats('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->getStats('yearly');
}

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

/**
* Get stats of the given type
* @param string $type Which type of statistics to return (hourly, daily, weekly, or monthly)
* @return array
*/
private function getStats(string $type) {
$config = Config::getInstance()->toArray();
$backend = new Backend();

if (isset($config['OPNsense']['vnstat']['general']['separate_stats']) && !$config['OPNsense']['vnstat']['general']['separate_stats']) {
// Separate stats are not wanted, just get totals - that's the default in vnstat itself,
// no need to specify anything here.
$response = $backend->configdpRun("vnstat", [ $type ]);
return array("response" => $response);
}

// Loop over configured interfaces, concatenating the output of each.
$result = '';
foreach (explode(',', $config['OPNsense']['vnstat']['general']['interface']) 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
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);
}
}
9 changes: 8 additions & 1 deletion 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 @@ -9,6 +9,13 @@
<id>general.interface</id>
<label>Interface</label>
<type>select_multiple</type>
<help>Set the interface to listen on.</help>
<help>Set the interface(s) to display stats for.</help>
Copy link
Author

Choose a reason for hiding this comment

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

Open to suggestions on this but I think the previous text was misleading, vnstat always gathers stats for all known interfaces as far as I can tell, this config option just controls the default for command line queries, and therefore what OPNsense shows in the UI pages.

Copy link
Member

Choose a reason for hiding this comment

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

If this is true it might be better to flip the default to the new behaviour.

wouldn’t that also mean querying stats works using “dev1+dev2”?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure what you mean by 'new behaviour' - do you mean the separation of interface stats? That seems a bad default change for people used to the current behaviour.

Querying for dev1+dev2 does work (other than my problems due to a 'missing' interface earlier) but just provides the total, not separate stats per interface as I'm trying to achieve - perhaps I've misunderstood what you're asking though.

Copy link
Member

Choose a reason for hiding this comment

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

If this merely pertains to stats fetch and you can fetch all interfaces using your code… changing the default to separate interface stats for multi select is not a bad thing. People can flip back to the old behaviour of the summarized query?

Copy link
Author

Choose a reason for hiding this comment

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

I've changed the default now.

</field>
<field>
<id>general.separate_stats</id>
<label>Separate interface stats</label>
<type>checkbox</type>
<help>If checked, separate stats are shown for each configured interface. If unchecked,
a single set of stats is shown containing the sum of traffic on all configured interfaces.</help>
</field>
</form>
8 changes: 6 additions & 2 deletions 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 @@ -4,12 +4,16 @@
<version>0.0.1</version>
<items>
<enabled type="BooleanField">
<default>0</default>
<Default>0</Default>
<Required>Y</Required>
</enabled>
<interface type="InterfaceField">
<Required>N</Required>
<multiple>Y</multiple>
<Multiple>Y</Multiple>
</interface>
<separate_stats type="BooleanField">
<Default>1</Default>
<Required>Y</Required>
</separate_stats>
</items>
</model>
8 changes: 4 additions & 4 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 @@ -24,25 +24,25 @@ message:request Vnstat status

[hourly]
command:/usr/local/bin/vnstat -h
parameters:
parameters:%s
type:script_output
message:request Vnstat hourly status

[daily]
command:/usr/local/bin/vnstat -d
parameters:
parameters:%s
type:script_output
message:request Vnstat daily status

[monthly]
command:/usr/local/bin/vnstat -m
parameters:
parameters:%s
type:script_output
message:request Vnstat monthly status

[yearly]
command:/usr/local/bin/vnstat -y
parameters:
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