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.

Sign up for GitHub

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

Jump to bottom

Fix an issue where destinations are compared against themselves #2065

Merged
merged 2 commits into from
Mar 9, 2023
Merged

Fix an issue where destinations are compared against themselves #2065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf816b3
Fix an issue where destinations are compared against themselves
felipeogarcia Mar 9, 2023
2abc0b8
Add power of two load balance tests
Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 15 additions & 4 deletions src/ReverseProxy/LoadBalancing/PowerOfTwoChoicesLoadBalancingPolicy.cs
View file
Open in desktop
Expand Up @@ -21,17 +21,28 @@ public PowerOfTwoChoicesLoadBalancingPolicy(IRandomFactory randomFactory)

public DestinationState? PickDestination(HttpContext context, ClusterState cluster, IReadOnlyList<DestinationState> availableDestinations)
{
if (availableDestinations.Count == 0)
var destinationCount = availableDestinations.Count;
if (destinationCount == 0)
{
return null;
}

if (destinationCount == 1)
{
return availableDestinations[0];
}

// Pick two, and then return the least busy. This avoids the effort of searching the whole list, but
// still avoids overloading a single destination.
var destinationCount = availableDestinations.Count;
var random = _randomFactory.CreateRandomInstance();
var first = availableDestinations[random.Next(destinationCount)];
var second = availableDestinations[random.Next(destinationCount)];
var firstIndex = random.Next(destinationCount);
int secondIndex;
do
{
secondIndex = random.Next(destinationCount);
} while (firstIndex == secondIndex);
Comment on lines +40 to +43
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix suffers from the same issue as the original, a high collision rate in small sets. I think it'd be better to do a deterministic shift in those cases.

int secondIndex = random.Next(destinationCount);
if (secondIndex == firstIndex)
{
    secondIndex = (irstIndex + 1) % destinationCount;
}

I'll send a PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MihaZupan convinced me that the +1 approach leads to non-trivial imbalance.

Even in the smallest case of two destinations where collisions will be common, the loop will only need to run a few times on average to break the tie. That's probably adequate.

var first = availableDestinations[firstIndex];
var second = availableDestinations[secondIndex];
return (first.ConcurrentRequestCount <= second.ConcurrentRequestCount) ? first : second;
}
}
41 changes: 34 additions & 7 deletions test/ReverseProxy.Tests/LoadBalancing/LoadBalancingPoliciesTests.cs
View file
Open in desktop
Expand Up @@ -72,7 +72,7 @@ public void PickDestination_Random_Works()
}

[Fact]
public void PickDestination_PowerOfTwoChoices_Works()
public void PickDestination_PowerOfTwoChoices_SkipBusiestConnection()
{
var destinations = new[]
{
Expand All @@ -82,24 +82,51 @@ public void PickDestination_PowerOfTwoChoices_Works()
};
destinations[0].ConcurrentRequestCount++;

const int Iterations = 10;
const int Iterations = 100;
var random = new Random(42);
RandomInstance.Sequence = Enumerable.Range(0, Iterations * 2).Select(_ => random.Next(destinations.Length)).ToArray();
RandomInstance.Sequence = Enumerable.Range(0, Iterations * Iterations).Select(_ => random.Next(destinations.Length)).ToArray();

var context = new DefaultHttpContext();
var loadBalancer = Create<PowerOfTwoChoicesLoadBalancingPolicy>();

for (var i = 0; i < Iterations; i++)
{
var result = loadBalancer.PickDestination(context, cluster: null, availableDestinations: destinations);
var first = destinations[RandomInstance.Sequence[i * 2]];
var second = destinations[RandomInstance.Sequence[i * 2 + 1]];
var expected = first.ConcurrentRequestCount <= second.ConcurrentRequestCount ? first : second;
Assert.Same(expected, result);

var groupByLoad = destinations.GroupBy(d => d.ConcurrentRequestCount);
var busiestGroup = groupByLoad.OrderByDescending(g => g.Key).First();
if (busiestGroup.Count() == 1)
{
Assert.True(result.ConcurrentRequestCount < busiestGroup.Key);
}

result.ConcurrentRequestCount++;
}
}

[Fact]
public void PickDestination_PowerOfTwoChoices_LeastLoaded()
{
var destinations = new[]
{
new DestinationState("d1") {ConcurrentRequestCount = 1000},
new DestinationState("d2")
};

const int Iterations = 100;
var random = new Random(42);
RandomInstance.Sequence = Enumerable.Range(0, Iterations * Iterations).Select(_ => random.Next(destinations.Length)).ToArray();

var context = new DefaultHttpContext();
var loadBalancer = Create<PowerOfTwoChoicesLoadBalancingPolicy>();

for (var i = 0; i < Iterations; i++)
{
var result = loadBalancer.PickDestination(context, cluster: null, availableDestinations: destinations);
Assert.Same(destinations[1], result);
}
}

[Fact]
public void PickDestination_LeastRequests_Works()
{
Expand Down