LibreQoE / LibreQoS

A Quality of Experience and Smart Queue Management system for ISPs. Leverage CAKE to improve network responsiveness, enforce bandwidth plans, and reduce bufferbloat.
https://libreqos.io/
GNU General Public License v2.0
464 stars 50 forks source link

Uisp suspend #360

Open bile0026 opened 1 year ago

bile0026 commented 1 year ago

Add ability to shape clients down to a specified upload and download speed when account is not in an "active" state. Cleaned up a bunch of formatting and convered comments to docstrings to help with automated documentation down the road.

To do: [] Unit testing

Wanted to get this open for comment.

rchac commented 1 year ago

Hey sorry for the delay. I thought this over and had two concerns:

1) The PR doesn't have a toggle for this suspension option. In our case, I can see the default suspension option being a problem for our network. We do suspension by other means (a NAT redirect to a UISP suspension page) which runs on a different schedule. So if a client resumes service after being suspended, their first use of the service will often be limited to 2/2 Mbps or the chosen suspension speed for a while until the LibreQoS refresh takes effect. A customer who resumed billing will see very poor performance as their first impression for a few minutes, which could be bad. Perhaps this should be an optional feature that can be toggled with a variable in ispConfig.py?

2) I'm not certain that LibreQoS should handle the suspension in this way by limiting bandwidth. I think customers would just think things are slow rather than suspended. Requesting feedback though @thebracket @bile0026

bile0026 commented 1 year ago

Hey sorry for the delay. I thought this over and had two concerns:

  1. The PR doesn't have a toggle for this suspension option. In our case, I can see the default suspension option being a problem for our network. We do suspension by other means (a NAT redirect to a UISP suspension page) which runs on a different schedule. So if a client resumes service after being suspended, their first use of the service will often be limited to 2/2 Mbps or the chosen suspension speed for a while until the LibreQoS refresh takes effect. A customer who resumed billing will see very poor performance as their first impression for a few minutes, which could be bad. Perhaps this should be an optional feature that can be toggled with a variable in ispConfig.py?
  2. I'm not certain that LibreQoS should handle the suspension in this way by limiting bandwidth. I think customers would just think things are slow rather than suspended. Requesting feedback though @thebracket @bile0026

For #1, I agree. I thought I had included that but I see that I did not. I will get that worked out and included.

As far as #2, at least in the case of UISP, the customer should get an email from UISP that their service has been suspended. This is similar to how Preseem handles suspension and has worked well for me in other environments. That said, I'm open to other ideas. I suppose it might be fairly trivial to have LibreQoS host a basic service suspension page or something like that. Personally I generally prefer a more all-in-one solution, vs having to stand up some other system to handle this via NAT or other redirect. So if there's a way to do the redirect within LibreQoS itself that would be slick.

thebracket commented 1 year ago

On my slightly hacked-up version of the UISP integration, we simply don't add suspended users to ShapedDevices.csv at all. It's sadly common for a device to remain suspended for a while, and not be online - which can lead to the same IP appearing more than once in our UISP tree.

I have some temporary code in the long_term_stats branch (the intent being to nuke this temporary code and go with mainline once this PR is done) that kinda-sorta implements my preferred approach: https://github.com/LibreQoE/LibreQoS/blame/long_term_stats/src/integrationUISP.py (see line 465).

I added a uispSuspendedStrategy config key, and none will do nothing, ignore will completely ignore the existence of suspended users and slow sets them to 1mbps in each direction. I've not actually tested "slow" yet. (Like I said, this isn't intended to stick around in this form)

On a philosophical level, I'm wondering if the UISP integration is the right integration point for suspension, since other integrations probably have a similar concept. integrationCommon could have a "suspended" flag per entry, which resolves through a shared interface - allowing other integrations to tag suspended, also.

As for whether LibreQoS should handle suspension? That's a good question. Preseem lets you set a config key for the speed to give suspended customers (ours defaulted to full speed, not sure if thats normal). We played with it a bit, and and it was useful mostly because UISP responds to speed queries for suspended customers with "all of it" - so it gave a safety net for surprise suspend (which was a problem in early versions of UISP). It's not a great way to handle suspension, because the customer can't tell the difference between a service problem and a non-payment - and may well go on NextDoor (etc.) and whine about your service before they get around to submitting a ticket or calling you. On the other hand, that's an ISP policy issue. It's pretty easy to setup a Mikrotik suspension redirect.

Should LibreQoS redirect? I'm personally inclined to say "no" - because now we're adding another layer of checks on the way through that might branch into sending customers elsewhere. It probably wouldn't be that hard to write, I'm just not sure it's worth the overhead - when it really should be a router task.