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

Add backend weight round robin select #34

Merged
merged 23 commits into from
Mar 9, 2019
Merged

Conversation

Sherlock-Holo
Copy link
Contributor

add

  • interface Selector
  • selector implement RandomSelector
  • selector implement WeightRoundRobinSelector

reason

dns-over-https allow us to set multi backends. However, sometimes some backends will be unavailable, we need a algorithm to reduce probability of thems. WRR algorithm is a good way.

    - random selector
    - weight random selector

random selector will choose upstream at random; weight random selector will choose upstream at random with weight

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
…in selector

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
…bin selector, the algorithm is nginx weight round robbin like

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
@m13253
Copy link
Owner

m13253 commented Mar 6, 2019

Thank you for the major contribution!

I guess it provides a solution for issue #24.
I will test it when I reach home. If it builds on all platforms and worked well, I will merge it.

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
@Sherlock-Holo
Copy link
Contributor Author

Actually I don't add any new dependenices, so it should work well for all paltforms as in the past

@Sherlock-Holo
Copy link
Contributor Author

I thinks we should let user know how to set upstream weight legitimately, and we may need to talk about:

  1. if upstream timeout, how much we should reduce the weight
  2. if upstream returns 5xx error, how much we should reduce the weight

1 and 2 are different, if upstream timeout, usually upstream will be unavailable for a long time, but if it returns 5xx, maybe it just a lot of users query it, it may return to normal quickly

@m13253
Copy link
Owner

m13253 commented Mar 6, 2019

We could set the penalty increase exponentially (like in power if around 1.6), but switch to linear after some number of fails, and cap it with a maximum value.

And we always make sure we have at least one server to query even if all servers seem unavailable.

@Sherlock-Holo
Copy link
Contributor Author

yes and we may can learn from some load b balance stuck as traefik

@Sherlock-Holo Sherlock-Holo changed the title Add backed weight round robin select Add backend weight round robin select Mar 6, 2019
Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
doh-client/doh-client.conf Outdated Show resolved Hide resolved
doh-client/doh-client.conf Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@m13253
Copy link
Owner

m13253 commented Mar 7, 2019

Please make some very small changes before I merge :-)

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
@Sherlock-Holo
Copy link
Contributor Author

The doh-client.conf actually is a toml config file, could we change the name suffix to toml? some text editor may only check name suffix, they won't highlight the content because they don't know what it is, such as Goland :-(

@Sherlock-Holo
Copy link
Contributor Author

I think I should rewrite the Selector interface because it doesn't provide a way to reduce weight in real time. I think reducing weight in real time is necessary.

@m13253
Copy link
Owner

m13253 commented Mar 7, 2019

The doh-client.conf actually is a toml config file, could we change the name suffix to toml? some text editor may only check name suffix, they won't highlight the content because they don't know what it is, such as Goland :-(

Actually I didn't intend to use a complex format like TOML to represent our configuration files.
What I expect is something like INI or systemd's configuration files. They are just lucky to be compatible with TOML.

Therefore the configuration is actually a simple format (until this pull request) which actually doesn't need any syntax highlighting.

Additionally systemd's configuration files also uses different suffixes (.conf, .service, .target, .socket, etc). I think Goland is to blame for not recognizing this.

Finally, if we break the compatibility of the configuration files, we need to bump the major version number.

@m13253
Copy link
Owner

m13253 commented Mar 7, 2019

I think I should rewrite the Selector interface because it doesn't provide a way to reduce weight in real time. I think reducing weight in real time is necessary.

I would really appreciate for your contributions!
I am sorry that I am too busy these months.

@Sherlock-Holo
Copy link
Contributor Author

This PR add Selector interface and wrr selector implement, so user should write every upsteam's weight, use TOML syntax will reudce compatible break. So we have to bump the major version number ^_^

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
…ion loop in real time

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
@Sherlock-Holo
Copy link
Contributor Author

I rewrite Selector interface and do a simple test in my PC, the wrr selector works well

@m13253
Copy link
Owner

m13253 commented Mar 7, 2019

I tested on my PC.

Thank you for the new deadline detection code.

But one more question: should we make Client.Selector private or public? I guess we don't need to share it with other packages?

Also, I hope all weight= 50 can be replaced by weight = 50, Url can be replaced by URL.

@Sherlock-Holo
Copy link
Contributor Author

Sherlock-Holo commented Mar 7, 2019 via email

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
@Sherlock-Holo
Copy link
Contributor Author

Change it pivate now

@m13253
Copy link
Owner

m13253 commented Mar 8, 2019

Change it pivate now

Thank you for the change!

Shall I make the last 2 changes for you, or you will do the changes before I merge?

@m13253
Copy link
Owner

m13253 commented Mar 8, 2019

Also, I hope all weight= 50 can be replaced by weight = 50, Url can be replaced by URL.

Add miss space for 'weight= 50'

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
…, report upstream ok in real time

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
@Sherlock-Holo
Copy link
Contributor Author

  • weight= 50 fixed
  • Url now is URL

@Sherlock-Holo
Copy link
Contributor Author

I upgrade Selector, now can report upstream works well. I add it because when I run it on our school DNS server, after reading logs, it seems wrr selector "drop" some upstreams because wrr selector reduce their weight too fast but icrease weight too slow when they are available, so we should increase weight when upstreams are available in real time

@m13253 m13253 merged commit fec1e84 into m13253:master Mar 9, 2019
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.

2 participants