dnsdist: add route policy of first ordered then weighted #15670
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
User may require two levels of routing policy to select downstream servers. First choose the least ordered, then distribute queries according to weights among the same ordered servers. It also added special filtering on selecting servers for query restart. If user sets the required tag in the restarted query then the policy will not select server(s) that had been tried before.
Pull Request Test Coverage Report for Build 15697060781Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
miodvallat
reviewed
Jun 13, 2025
| if (svr.second->isUp() && svr.second->d_config.order <= curOrder && (!dnsq->ids.qTag || dnsq->ids.qTag->count(svr.second->getNameWithAddr()) == 0)) { | ||
| if (svr.second->d_config.order < curOrder) { | ||
| curOrder = svr.second->d_config.order; | ||
| startIndex = candidates.end() - candidates.begin(); |
There was a problem hiding this comment.
Wouldn't that be
Suggested change
| startIndex = candidates.end() - candidates.begin(); | |
| startIndex = candidates.size(); |
| @@ -2079,7 +2079,7 @@ load_balancing_policies: | |||
| lua-name: "setWeightedBalancingFactor" | |||
| internal-field-name: "d_weightedBalancingFactor" | |||
| runtime-configurable: false | |||
| description: "Set the maximum imbalance between the number of outstanding queries intended for a given server, based on its weight, and the actual number, when using the ``whashed`` or ``wrandom`` load-balancing policy. Default is 0, which disables the bounded-load algorithm" | |||
| description: "Set the maximum imbalance between the number of outstanding queries intended for a given server, based on its weight, and the actual number, when using the ``whashed`` or ``wrandom`` or ``orderedWrandUntag`` load-balancing policy. Default is 0, which disables the bounded-load algorithm" | |||
There was a problem hiding this comment.
Suggested change
| description: "Set the maximum imbalance between the number of outstanding queries intended for a given server, based on its weight, and the actual number, when using the ``whashed`` or ``wrandom`` or ``orderedWrandUntag`` load-balancing policy. Default is 0, which disables the bounded-load algorithm" | |
| description: "Set the maximum imbalance between the number of outstanding queries intended for a given server, based on its weight, and the actual number, when using the ``whashed``, ``wrandom`` or ``orderedWrandUntag`` load-balancing policy. Default is 0, which disables the bounded-load algorithm" |
| ``orderedWrandUntag`` is another weighted policy with additional server filtering: | ||
|
|
||
| - select the group of server(s) with the lowest ``order`` passed to :func:`newServer`. | ||
| - filter out server(s) that were tagged with key string of :func:`Server:getNameWithAddr` in the query that was set by :func:`DNSQuestion:setTag`. This can be useful to restart a query with a different server, the user is responsible to set the required tag in lua action before calling :func:`DNSResponse:restart`. Initial queries are not impacted by this filtering if no other intentional lua action to set the tag. |
There was a problem hiding this comment.
I suppose you mean
Suggested change
| - filter out server(s) that were tagged with key string of :func:`Server:getNameWithAddr` in the query that was set by :func:`DNSQuestion:setTag`. This can be useful to restart a query with a different server, the user is responsible to set the required tag in lua action before calling :func:`DNSResponse:restart`. Initial queries are not impacted by this filtering if no other intentional lua action to set the tag. | |
| - filter out server(s) that were tagged with key string of :func:`Server:getNameWithAddr` in the query that was set by :func:`DNSQuestion:setTag`. This can be useful to restart a query with a different server, the user is responsible to set the required tag in lua action before calling :func:`DNSResponse:restart`. Initial queries are not impacted by this filtering if there is no other intentional lua action to set the tag. |
| @@ -300,7 +309,7 @@ Functions | |||
| .. versionadded: 1.5.0 | |||
|
|
|||
| Set the maximum imbalance between the number of outstanding queries intended for a given server, based on its weight, | |||
| and the actual number, when using the ``whashed`` or ``wrandom`` load-balancing policy. | |||
| and the actual number, when using the ``whashed`` or ``wrandom`` or ``orderedWrandUntag`` load-balancing policy. | |||
There was a problem hiding this comment.
Suggested change
| and the actual number, when using the ``whashed`` or ``wrandom`` or ``orderedWrandUntag`` load-balancing policy. | |
| and the actual number, when using the ``whashed``, ``wrandom`` or ``orderedWrandUntag`` load-balancing policy. |
| @@ -631,7 +631,7 @@ Setting for load-balancing policies | |||
| - **default_policy**: String ``(leastOutstanding)`` - Set the default server selection policy | |||
| - **servfail_on_no_server**: Boolean ``(false)`` - If set, return a ServFail when no servers are available, instead of the default behaviour of dropping the query | |||
| - **round_robin_servfail_on_no_server**: Boolean ``(false)`` - By default the roundrobin load-balancing policy will still try to select a backend even if all backends are currently down. Setting this to true will make the policy fail and return that no server is available instead | |||
| - **weighted_balancing_factor**: Double ``(0.0)`` - Set the maximum imbalance between the number of outstanding queries intended for a given server, based on its weight, and the actual number, when using the ``whashed`` or ``wrandom`` load-balancing policy. Default is 0, which disables the bounded-load algorithm | |||
| - **weighted_balancing_factor**: Double ``(0.0)`` - Set the maximum imbalance between the number of outstanding queries intended for a given server, based on its weight, and the actual number, when using the ``whashed`` or ``wrandom`` or ``orderedWrandUntag`` load-balancing policy. Default is 0, which disables the bounded-load algorithm | |||
There was a problem hiding this comment.
Suggested change
| - **weighted_balancing_factor**: Double ``(0.0)`` - Set the maximum imbalance between the number of outstanding queries intended for a given server, based on its weight, and the actual number, when using the ``whashed`` or ``wrandom`` or ``orderedWrandUntag`` load-balancing policy. Default is 0, which disables the bounded-load algorithm | |
| - **weighted_balancing_factor**: Double ``(0.0)`` - Set the maximum imbalance between the number of outstanding queries intended for a given server, based on its weight, and the actual number, when using the ``whashed``, ``wrandom`` or ``orderedWrandUntag`` load-balancing policy. Default is 0, which disables the bounded-load algorithm |
|
Thanks for the comments, addressed in the followed commit. |
rgacogne
requested changes
Jun 16, 2025
| return {}; | ||
| } | ||
|
|
||
| ServerPolicy::NumberedServerVector selected(candidates.begin() + startIndex, candidates.end()); |
There was a problem hiding this comment.
Is there a reason you decided to copy to a new vector rather than erasing1 from candidates? In theory I think it would be slightly more efficient to erase but I have not measured it. Actually, I don't think this is needed because of my comment above.
|
|
||
| for (const auto& svr : servers) { | ||
| if (svr.second->isUp() && svr.second->d_config.order <= curOrder && (!dnsq->ids.qTag || dnsq->ids.qTag->count(svr.second->getNameWithAddr()) == 0)) { | ||
| if (svr.second->d_config.order < curOrder) { |
There was a problem hiding this comment.
This should never happen because servers are ordered on their order inside a pool. And if it did happen you would have an issue because the index that is stored in svr.second would be completely wrong, which could cause an exception to be thrown in getValRandom. So I think it would make sense to remove this test, perhaps adding a comment explaining why it is not needed.
| end | ||
| end | ||
| """ | ||
|
|
There was a problem hiding this comment.
Unless I'm mistaken this doesn't test the case where a server is not used because a tag? It would be nice to test this case as well.
|
that will be good if the pool servers are already sorted by its order. I'll checkout and make changes accordingly. Yes, tag is not tested as I thought it is the same condition as server down. But yes, rigorously, separate testing for that condition is better. I'll work it out. |
rgacogne
approved these changes
Jun 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Short description
User may require two levels of routing policy to select downstream servers. First choose the least ordered, then distribute queries according to weights among the same ordered servers. It also added special filtering on selecting servers for query restart. If user sets the required tag in the restarted query then the policy will not select server(s) that had been tried before.
Checklist
I have: