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

Add Consistent Hashing Load Balancing Strategy #592

Merged
merged 6 commits into from
Aug 19, 2024

Conversation

sinadarbouy
Copy link
Collaborator

Ticket(s)

#398

Description

This pull request introduces a new consistent hashing strategy to our load balancing system. The consistent hashing algorithm routes client connections to specific proxies based on a hash of the client’s IP address or full connection address. This addition enhances the load balancing mechanism by providing a more deterministic way to distribute traffic, especially useful in scenarios where persistence or even distribution is crucial.

Changes:

  1. New Consistent Hashing Implementation:
    • Added a new ConsistentHash struct to implement the consistent hashing strategy.
    • Added methods for creating a ConsistentHash instance and selecting proxies based on the hash of the client’s IP or connection address.
    • Utilizes the MurmurHash3 algorithm for hashing.
  2. Configuration Updates:
    • Extended the LoadBalancer struct to include optional consistent hashing configuration.
    • Updated the configuration file (gatewayd.yaml) to support the new consistentHash field.
  3. Integration with Existing Strategies:
    • Updated the NewLoadBalancerStrategy function to wrap existing strategies with consistent hashing if configured.
    • Adjusted the NextProxy method in existing strategies to accept connection parameters, aligning with the new interface.
  4. Tests:
    • Added comprehensive tests for the ConsistentHash strategy, including cases where hashing is based on source IP and full connection address.
    • Updated existing tests to work with the new IConnWrapper interface.
  5. Code Adjustments:
    • Refactored related components to support the new consistent hashing strategy.
    • Added a mock implementation for IConnWrapper to facilitate testing.

Related PRs

Development Checklist

  • I have added a descriptive title to this PR.
  • I have squashed related commits together.
  • I have rebased my branch on top of the latest main branch.
  • I have performed a self-review of my own code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added docstring(s) to my code.
  • I have made corresponding changes to the documentation (docs).
  • I have updated docs using make gen-docs command.
  • I have added tests for my changes.
  • I have signed all the commits.

Legal Checklist

@sinadarbouy
Copy link
Collaborator Author

@mostafa
The issue suggests using Murmur, and the more popular package is https://github.com/spaolacci/murmur3. I added that package; do you think it’s okay if I include it in the Depguard rules?
We also have other alternatives like cespare/xxhash and google/cityhash..

@mostafa
Copy link
Member

mostafa commented Aug 19, 2024

@mostafa

The issue suggests using Murmur, and the more popular package is https://github.com/spaolacci/murmur3. I added that package; do you think it’s okay if I include it in the Depguard rules?

We also have other alternatives like cespare/xxhash and google/cityhash..

I suggested Murmur as an example and the best hashing algorithm should be chosen in terms of 1) performance and 2) least amount of or no collisions. If you think that Murmur3 outperforms the others and is well-maintained, we can use it. (Optionally) In the future we can give the users the choice of the algorithm.

return &ConsistentHash{
originalStrategy: originalStrategy,
useSourceIP: server.LoadbalancerConsistentHash.UseSourceIP,
hashMap: make(map[uint64]IProxy),
Copy link
Member

Choose a reason for hiding this comment

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

I think you could use sync.Map, which handles locking behind the scenes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At first, I messed up by just locking the load and store process. Now, I’ve added a sync.Mutex to lock the whole NextProxy connection. also added a test case to check for concurrency. (so I guess we can not use sync.Map here anymore)

@mostafa mostafa merged commit 54d0c69 into gatewayd-io:main Aug 19, 2024
5 checks passed
@mostafa mostafa mentioned this pull request Oct 6, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants