Add rotate option for recursors

[munali]

Closes #8807

[dnephin]

Thank you for the PR! I think this is a good idea, and I have a suggestion for the implementation.

[dnephin]

I think there may be a problems here. cfg.Recursors.RoundRobinIndex does not appear to be reset to 0 when it reaches the end of the list. I think this will panic after a few requests.

I believe cfg.Recursors.RoundRobinIndex += 1 is not safe for concurrent use either, so this could return incorrect results when there are multiple handlers running at the same time in different goroutines.

Instead of round robin, would random selection be ok? Something like this would remove the need to track the Index, and I think should also be safe for concurrent use because we only read concurrently, and the recursors list does not change.

for _, idx := range rand.Perm(len(cfg.Recursors)) {
    recursor := cfg.Recursors[idx]
}

[munali]

@dnephin Doh! I forgot to modulo.

I am sure this is not safe for concurrent use either, but I believe something like this could work as well? What do you think.

cfg.Recursors.RoundRobinIndex = (atomic.AddInt32(cfg.Recursors.RoundRobinIndex, 1)%len(cfg.Recursors))

Either way, I like your suggestion because it does a good job of distributing the load. Whereas my attempt adds complexity to increment the index and store it as an extra variable.

Would something like this be good/safe golang style?

for idx, random_idx := range rand.Perm(len(cfg.Recursors)) {
    recursor := cfg.Recursors[idx]
    if cfg. RecursorRotate {
       recursor = cfg.Recursors[random_idx]
   }
}

[munali]

@dnephin any thoughts on the changes?

[munali]

Hi @dnephin I am not sure if you so my comment above or my recent push. Does that seem reasonable? Thank you for your help! Much appreciated.

[kyhavlov]

Hi @munali, sorry for the lack of attention on this PR after so long - how would you feel about changing the config field to something like dns_recursor_strategy or similar and having the valid options be default/randomize, instead of a boolean? I think the approach taken with rand.Perm() makes sense, just a thought about the wording of the config field.

[blake]

Hi @munali, we never heard back from you, but we were interested in adding this feature to Consul so I added this in #10611, and gave you credit as one of the authors. I hope you don't mind.

Thank you for your contribution.