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.

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

Uisp suspend #360

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

Uisp suspend #360

wants to merge 16 commits into from

Conversation

bile0026
Copy link
Contributor

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.

Copy link
Member

@rchac rchac left a comment

Choose a reason for hiding this comment

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

Everything looks good except lines 212 and 220 of LibreQoS.py. With those changed back to if a < 1: we should be set.

src/LibreQoS.py Outdated Show resolved Hide resolved
src/LibreQoS.py Outdated Show resolved Hide resolved
@rchac rchac requested a review from thebracket May 23, 2023 17:07
@bile0026 bile0026 requested a review from rchac June 14, 2023 17:51
@rchac
Copy link
Member

rchac commented Jun 14, 2023

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
Copy link
Contributor Author

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants