-
Notifications
You must be signed in to change notification settings - Fork 18
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 server-count support with random selection algorithm #63
Conversation
Hi, @denis-tingaikin |
Sorry, got some troubles with DCO, reopened this PR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the idea to get the policy for the fanout plugin. Many thanks for updating everything to the latest versions. Please feel free to open a separate PR with uplift; it will be merged instantly.
Just curious. Why just the load balancer server could not be used for the servers with the same records?
I found a few corner cases that at this moment are looking like blockers:
- Servers with the same records can be selected during the request.
- Server availability is not managed.
But if these problems are fine for your use-cases, we can merge this if a policy block is implemented.
README.md
Outdated
* `server-count` is the number of DNS servers to be requested. Equals to the number of specified IPs by default. If this parameter is lower than the number of specified IP addresses, servers are randomly selected based on the `load-factor` parameter. | ||
* `load-factor` - the probability of selecting a server. This is specified in the order of the list of IP addresses and takes values between 1 and 100. By default, all servers have an equal probability of 100. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think if we define a policy field to be consistent with the forward plugin?
fanout addr1, addr2, adddr3 {
policy weighted-rand{
server-count 2
load-factor 50 50 50
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Now it's possible to specify policy for fanout:
weighted-random:
example.org {
fanout . 127.0.0.1:9005 127.0.0.1:9006 127.0.0.1:9007
policy weighted-random {
server-count 2
load-factor 50 70 100
}
}
sequential (default):
example.org {
fanout . 127.0.0.1:9005 127.0.0.1:9006 127.0.0.1:9007
policy sequential
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
policy is going to be a separate Caddyfile block that may confuse Coredns users.
What if we make it flat?
example.org {
fanout . 127.0.0.1:9005 127.0.0.1:9006 127.0.0.1:9007 {
policy: weighted-random
weighted-random-server-count: 2
weighted-random-load-factor 50 70 100
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Example:
example.org {
fanout . 127.0.0.1:9005 127.0.0.1:9006 127.0.0.1:9007 {
policy weighted-random
weighted-random-server-count 2
weighted-random-load-factor 50 70 100
}
}
Signed-off-by: Gleb Kogtev <[email protected]>
* Set Go version to 1.21 --------- Signed-off-by: Gleb Kogtev <[email protected]>
- update error message in setup.go - add empty case in setup_test.go - make variables names more consistent - update copyright Signed-off-by: Gleb Kogtev <[email protected]>
Well, we expect this plugin to be further improved. It's just a first step. Load balancer is additional component, which might be a bottleneck in high-load scenarios |
- rename simple selector to sequential - update docs + add examples Signed-off-by: Gleb Kogtev <[email protected]>
@denis-tingaikin kindly ask you to take a look into this PR one more time |
@glebkin Looks good, only one comment and failed linter are remaining ;) |
* make policy configuration flat * update docs * fix linter issues --------- Signed-off-by: Gleb Kogtev <[email protected]>
Done. Please take a look. |
I've found a few confusing things in the README.md file; could you check if the configuration that I suggest is correct? |
Co-authored-by: Denis Tingaikin <[email protected]> Signed-off-by: Gleb Kogtev <[email protected]>
Co-authored-by: Denis Tingaikin <[email protected]> Signed-off-by: Gleb Kogtev <[email protected]>
Yep, thanks for help, fixed these typos Honestly, I didn't notice that I printed configuration wrong here, but actually it was like this:
So I had nested block with policy configuration. |
Revert flat policy configuration params. Fix README.md typos with configuration examples --------- Signed-off-by: Gleb Kogtev <[email protected]>
@denis-tingaikin I decided to rollback my latest commit with flat policy configuration, because I think this configuration is better:
than this:
Also I've update README.md examples to match actual setup settings. |
Since we're trying to use the syntax that, as you found, is not allowed by Caddyfile, I am going to ask CoreDNS contact to be sure that this is OK. |
@glebkin I just got a response that it's not recommended. Following the stict Caddyfile format is the best option. So I suggest returning to the flat configuration. |
This reverts commit 48958d5. --------- Signed-off-by: Gleb Kogtev <[email protected]>
Sad, but okay. I've reverted it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@glebkin Many thanks!
@denis-tingaikin is it possible to create new release? |
Sure done: https://github.com/networkservicemesh/fanout/releases/tag/v1.11.3 tag is v1.11.3 indicates which coredns we use. |
The Fanout plugin currently sends DNS requests to all servers specified in the configuration. However, this may be unnecessary in some cases. For example, if we have multiple instances of DNS servers scaled horizontally, each with the same copy of DNS records, it would be pointless to send requests to each server individually. Instead, we can choose to send requests to only some of the servers based on their
load-factor
. To address this, I have made some improvements to the plugin:server-count
andload-factor
:server-count
is the number of DNS servers to be requested. Equals to the number of specified IPs by default. If this parameter is lower than the number of specified IP addresses, servers are randomly selected based on theload-factor
parameter.load-factor
- the probability of selecting a server. This is specified in the order of the list of IP addresses and takes values between 1 and 100. By default, all servers have an equal probability of 100.Example of such configuration:
In this example, for each DNS request to
example.org
, only two of the specified servers will be selected. The servers are selected randomly based on the load-factor parameter of each server.2. Added the
internal/selector
package with two implementations for generic values selectionSimple
selector - useful when theserver-count
equals the number of available IPs. In this case, we don't need to spend time randomly selecting servers, so we simply return all of them, one by one.WeightedRand
selector which selects servers randomly based on theirload-factor
runWorkers
function for better readabilitymath/rand/v2
and generics