perf(multiwan): derive the split and the tracker state instead of shelling out - #387
Conversation
gnacho
left a comment
There was a problem hiding this comment.
Thanks for the solid work here. The derivation matches mwan3 policies on your live box and the measured win is real (1313 ms to 264 ms). A few things before this can land:
Questions
-
Trackingis now hardcoded to"active". The UI does not consume the field today (MultiWanCardonly readsc.online), so there is no visible impact. Still, is the plan to keep the field, or should it go once nothing reports real tracker state? -
livenow covers every iface incfg.Ifaces. An iface present in the config but disabled (no entry under/var/run/mwan3/iface_state) would previously not appear, and now shows asonline: "offline", which paints a red dot / "failed" pill in the UI. Have you checked that case on your box, e.g. with one uplink disabled? If not, an easy fix is to skip ifaces with no state file instead of defaulting them to offline. -
policyOfActiveRulereturns the alphabetically-first policy referenced by any rule, not the onemwan3 policiesmarks in force. With a single policy they coincide (as on your router), but the name reads like it answers more than it does. Fine to keep if multi-policy setups are out of scope; a word in the comment would save the next reader the surprise.
Process
- Could you drop the
Co-Authored-By: Claude Opus 5trailer from the commit message (e.g.git commit --amend)? This repo keeps AI tooling out of the recorded history. - Same for #390, plus one small thing there: the inline comment added in
index.htmlis in Spanish, while repo convention is English comments (user-facing strings go through i18n). Mind switching it?
|
Heads-up: I rewrote |
061416a to
0ea25d9
Compare
gnacho
left a comment
There was a problem hiding this comment.
I verified this on a live router (mipsle, OpenWrt 24.10, mwan3 2.11) running this branch as a preview binary, against mwan3 interfaces/mwan3 policies as ground truth. Numbers first, then one regression I could reproduce.
The perf win is real here too: GET /api/multiwan went from ~3.4-4.9 s to ~0.31 s cold on this box (borky's 1313 ms → 264 ms measured on theirs). active_policy and the share split match mwan3 policies exactly for the stock config.
Reproduced regression: a disabled interface now reads "failed" instead of "standby".
With wan present in both /etc/config/network and mwan3, but option enabled '0' in mwan3 (so no state file under /var/run/mwan3/iface_state):
mwan3 interfaces:interface wan is unknown and tracking is down (31)- this branch: candidate
{"online": "offline", "tracking": "active"} - current main: candidate
{"online": "unknown", "tracking": "down"}
The UI maps online === "offline" to the danger pill ("failed") in stateLabel, while "unknown" falls through to the muted "standby" pill. So an uplink that mwan3 is simply not managing flips from neutral to red, and tracking: "active" says the opposite of what mwan3 interfaces reports. This is the case I flagged in the earlier review; it is not hypothetical.
Could the derivation keep the old semantics here? Concretely: only report online: "online" when the state file says so, keep online: "offline" for a state file that says offline, and go back to unknown/down when the interface has no state file (or enabled '0' in the mwan3 config). That preserves the win — no shelling out — while not crying failure at interfaces mwan3 is not tracking.
On the third point from the earlier review (policyOfActiveRule returning the alphabetically first policy): with the stock config all rules point at balanced, so it matches. I agree a single "active policy" is inherently a simplification since mwan3 policies are per rule; the alphabetical pick is fine for NetGrip-managed configs (single policy) — maybe just worth a comment saying so.
…lling out ProbeMultiWAN ran two mwan3 shell scripts to learn things it already had the ingredients for. Both walk the whole rule set, and on ipq40xx-class hardware they cost, measured over five runs each: mwan3 interfaces 408 ms mwan3 policies 302 ms uci show mwan3 8 ms That is 710 ms spent every time somebody opens the Internet page, for a tracker verdict that is already one word per file under /var/run/mwan3/iface_state and a split that follows from the member weights. The state reading inside MwanActiveUplink becomes mwanLiveState, so the probe and the resolver share it, and two pure functions derive what the scripts reported: policyOfActiveRule names the policy a rule actually points at, and mwanShares works out the split the way mwan3 does - only the lowest metric group with something online carries traffic, and inside it the weights decide. A policy with nothing derivable falls back to 100% on the active uplink rather than reporting an empty split. Measured end to end on the same hardware, the whole probe goes from 1313 ms to 264 ms. parseMwanInterfaces and parseMwanPolicies are left in place: nothing calls them now, but they are tested parsers of mwan3's own output and removing them is a separate decision.
Reading the tracker's state files instead of running `mwan3 interfaces` turned every interface without a file into "offline", with tracking "active". An uplink mwan3 is not tracking - enabled '0' in its config, or not started - has no state file, and the script reports it as "unknown and tracking is down"; the UI shows that as standby, but "offline" as a failed link, so a link nobody monitors turned red. The probe now reports what the script would: an interface that is disabled in the mwan3 config or has no state file is unknown and not tracked, and a tracked one gets the state the tracker recorded (online, offline, connecting, ...) with tracking active. Tracking is therefore real again, derived from whether mwan3 tracks the link. policyOfActiveRule also says in its comment what it returns with more than one policy in use: the alphabetically first one a rule points at, which is exact for the single-policy configurations NetGrip writes.
0ea25d9 to
fbf335d
Compare
gnacho
left a comment
There was a problem hiding this comment.
Verified on live hardware (mipsle, OpenWrt 24.10, mwan3 2.11): the disabled uplink now reports unknown/down, matching what mwan3 interfaces says, so the UI shows standby instead of the failed red we had before. Tracked uplinks take the tracker state words, latency holds at ~0.33 s against 3.4-4.9 s on main, and the new test covers the stale state file case too. Thanks for the quick turnaround.
ProbeMultiWANran twomwan3shell scripts to learn things it already hadthe ingredients for. Both walk the whole rule set. Measured over five runs
each on ipq40xx-class hardware:
mwan3 interfacesmwan3 policiesuci show mwan3That is 710 ms spent every time somebody opens the Internet page, for a
tracker verdict that is already one word per file under
/var/run/mwan3/iface_state, and a split that follows from the memberweights. Measured end to end on the same hardware, the probe goes from
1313 ms to 264 ms.
What changes
MwanActiveUplinkis factored out asmwanLiveState, so the probe and the uplink resolver share one cheap read(
uci showplus the state files) instead of each doing its own.policyOfActiveRulenames the policy a rule actually points at — whatmwan3 policieswould print as the one in force.mwanSharesderives the split the way mwan3 does it: only the lowest metricgroup with something online carries traffic, and within it the weights
decide. Deriving it also keeps the figure fresh on every read, where asking
the script cost a third of a second.
rather than reporting an empty split for a policy that is plainly steering.
Both new functions are pure and take the parsed config, so they are tested
without a router.
Verification
Checked against a live router, not only fixtures: its real
uci show mwan3was fed through the new derivations and the result compared with what
mwan3 policiesprinted on the same box at the same moment.mwan3 policiesprintedpolicyOfActiveRule→netgrip_polnetgrip_pol:mwanShares→{rds: 100}rds (100%)pickMwanActive→rdsSame answers, no subprocess. The second uplink sits at a higher metric and
correctly carries nothing.
go test ./...andtsc --noEmitpass, and the branch builds against thepublished
netpulse/agentmodule. The router is a two-uplink setup (PPPoE anda cellular modem) running
mwan32.12.0 on OpenWrt 25.12, withnetgrip_*policies loaded and steering.
One thing left alone deliberately
parseMwanInterfacesandparseMwanPolicieshave no callers after thischange. They are tested parsers of
mwan3's own output and a diagnostics viewmight still want them, so removing them looked like a separate decision rather
than part of a performance fix. Happy to drop them and their tests here if you
would rather not carry dead code.