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

dnsdist: add consistent hash builtin policy #6737

Merged
merged 6 commits into from Aug 24, 2018

Conversation

@chbruyand
Copy link
Member

@chbruyand chbruyand commented Jun 14, 2018

Short description

This adds a new load balancing policy that is based on consistent hashing. Unlike the whashed policy, this distribution will keep consistent over time. Adding or removing servers will only remap a small part of the queries.

For the record, this also adds auuid field to the DownstreamState class.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
pdns/dnsdist.cc Outdated

for (const auto& d: servers) {
if (d.second->isUp()) {
if (d.second->hashes.empty()) {
Copy link
Member

@rgacogne rgacogne Jun 14, 2018

We probably need a per-server mutex when accessing hashes.

Copy link
Member Author

@chbruyand chbruyand Jun 14, 2018

Definitely!

pdns/dnsdist.cc Outdated
if (d.second->isUp()) {
if (d.second->hashes.empty()) {
// computes server's points
// @todo check if server's weight can change over time
Copy link
Member

@rgacogne rgacogne Jun 14, 2018

I don't think so, but even if it does I think it might be cleaner to compute the points at the server's creation and when the weight changes, instead of doing it here?

Copy link
Member Author

@chbruyand chbruyand Jun 14, 2018

Yes, but I did it there because it prevents unnecessary computation when the policy isn't set to chashed and I got the feeling the policy handler could be called even if the different pools were not set to it.

Copy link
Member

@rgacogne rgacogne Jun 14, 2018

Right, I feel pre-computing the points might be worth it since it should happen very rarely but let's not rush that decision.

pdns/dnsdist.cc Outdated
}
for (const auto& h: d.second->hashes) {
// put server's hashes on the circle
circle.insert(std::make_pair(h, d.second));
Copy link
Member

@rgacogne rgacogne Jun 14, 2018

Do you think it would make sense to compute the circle when a server is added / removed or changes state?

Copy link
Member Author

@chbruyand chbruyand Jun 14, 2018

Hum, yes we could, but as the circle is pool dependent, it would require to change the signature of the policy handlers and pass the pool the query is assigned to. Didn't get the feeling it was an easy move as the policy system is quiet generic.

Copy link
Member Author

@chbruyand chbruyand Jun 14, 2018

I mean, to cache it, it would require to be aware of the context we are called in. Is that something we want ?

Copy link
Member

@rgacogne rgacogne Jun 14, 2018

Ah yes, it would lead to quite substantial changes so let's not go there until we have properly benchmarked that new policy.

@rgacogne rgacogne added this to the dnsdist-1.4.0 milestone Jun 14, 2018
@rgacogne rgacogne removed this from the dnsdist-1.4.0 milestone Jun 14, 2018
@rgacogne rgacogne added this to the dnsdist-1.3.x milestone Jun 14, 2018
@rgacogne
Copy link
Member

@rgacogne rgacogne commented Jun 14, 2018

Given that this PR has currently very little impact to the existing code I'm scheduling it for 1.3.x, we might need to rethink that if the impact grows.

@chbruyand
Copy link
Member Author

@chbruyand chbruyand commented Jun 15, 2018

Moved hashes computation to object initialization and added two setters for weight and id members.

@chbruyand
Copy link
Member Author

@chbruyand chbruyand commented Jun 15, 2018

It looks like the performances of this is comparable to other policies :
metronome

(from left to right, chashed, whashed, leastOutstanding and wrandom)

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Jun 18, 2018

It looks like the regression test "Routing: WRandom (overflow)" is failing with this PR, we need to check whether the test is testing something it shouldn't or if the PR actually is breaking something.

@chbruyand chbruyand changed the title wip: dnsdist: add consistent hash builtin policy dnsdist: add consistent hash builtin policy Jul 31, 2018
@chbruyand chbruyand requested a review from rgacogne Jul 31, 2018
Copy link
Member

@rgacogne rgacogne left a comment

One nit, looks good to me!

ReadLock rl(&(d.second->d_lock));
const auto& server = d.second;
// we want to keep track of the last hash
if (max < *(server->hashes.rbegin())) {
Copy link
Member

@rgacogne rgacogne Jul 31, 2018

Don't we have an issue here if the weight is set to zero, or even a negative since it's a signed integer,?

Copy link
Member Author

@chbruyand chbruyand Jul 31, 2018

Didn't consider that as the weight value is bounded when the server is added (see

if(weightVal < 1) {
)

Copy link
Member

@rgacogne rgacogne Jul 31, 2018

Right, although it looks like we have a Lua binding for the weight member that does not check anything :'(

Copy link
Member Author

@chbruyand chbruyand Jul 31, 2018

Just added some checks around the weight member binding

pdns/dnsdist.cc Outdated
@@ -2258,6 +2336,15 @@ try

auto todo=setupLua(false, g_cmdLine.config);

if (g_policy.getLocal()->name == "chashed") {
vinfolog("Pre-computing hashes for consistent hash load-balancing policy");
Copy link
Member

@rgacogne rgacogne Aug 6, 2018

I haven't tested but it looks like this only work for the global load-balancing policy, perhaps we should also test pool-specific load-balancing policies (set with setPoolServerPolicy())?

Copy link
Member Author

@chbruyand chbruyand Aug 16, 2018

https://github.com/PowerDNS/pdns/pull/6737/files#diff-194369649d922d5a26ab47ac9c895d33R770
Indeed, this only tests against global policy in order to decide whether or not to pre-compute hashes. However, they will be computed on demand if needed.

Copy link
Member

@rgacogne rgacogne Aug 16, 2018

Right, I agree this is not an issue, but I was just wondering if we might want to iterate over the pools there and check their policy as well? But that's not blocking either way.

Copy link
Member Author

@chbruyand chbruyand Aug 16, 2018

I'll update the check :)

@chbruyand chbruyand force-pushed the dnsdist-consistent-hashing branch from b720da2 to 636cc54 Aug 16, 2018
@rgacogne rgacogne merged commit c6420f2 into PowerDNS:master Aug 24, 2018
4 checks passed
@chbruyand chbruyand deleted the dnsdist-consistent-hashing branch Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants