net/vnstat: Handle multiple interfaces better #4443
Conversation
| @@ -9,7 +9,11 @@ | |||
| </enabled> | |||
| <interface type="InterfaceField"> | |||
| <Required>N</Required> | |||
| <multiple>Y</multiple> | |||
| <multiple>N</multiple> | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmm, yes, pretty sure this used to work.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -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> | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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”?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I've changed the default now.
| $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'])) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?)
I tried using vnstat to report on multiple interfaces (to see how much traffic is going over a backup LTE connection I'm experimenting with) and it didn't really work. (update: some of this was due to user error)
Selecting two interfaces was allowed but resulted in(this works fine, my problem was due to the point below)Interface foo+barinvnstat.confwhich appears to be a config directive only supporting one interface.vnstatwas not configured to pick up new interfaces added after it was first installed.The stat in the UI only show the interface configured on the general tab, and as noted, configuring more than one there doesn't work.(also not a problem)Putting my user error aside, it does work mostly, but doesn't pick up new interfaces, and can't show interface stats individually.
To improve this I have:
Restricted the interface option to one interface only, and renamed it to Default interfaceI've added a second 'Interfaces to display' option, which allows more than one to be selectedThe stats pages in the UI now show stats for all interfaces specified under 'Interfaces to display'AlwaysAddNewInterfacesto make it notice new interfaces added.