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

feat(kuma-cp) user specified load balancer type per route #1402

Merged
merged 5 commits into from
Jan 13, 2021

Conversation

nickolaev
Copy link
Contributor

@nickolaev nickolaev commented Jan 11, 2021

Summary

Extending the TrafficRoute to be able to specify the LoadBalancer type, reflecting Envoy's capabilities.
Supported types:

  • Round-robin
  • Least request - can set choice count
  • Ring hash - can set hash function, min/max ring size
  • Random
  • Maglev

A sample Universal resource would look like this:

type: TrafficRoute
name: route-all-default
mesh: default
sources:
  - match:
      kuma.io/service: '*'
destinations:
  - match:
      kuma.io/service: '*'
conf:
  loadBalancer:
    roundRobin: {}
  split:
    - destination:
        kuma.io/service: '*'
      weight: 100

Where the loadBalancer field can have the following values

  loadBalancer:
    leastRequest:
      choiceCount: 32
  loadBalancer:
    ringHash:
      hashFunction: "MURMUR_HASH_2"
      minRingSize: 64
      maxRingSize: 1024
  loadBalancer:
    random: {}
  loadBalancer:
    maglev: {}

Issues resolved

Fix #1030

Documentation

Nikolay Nikolaev added 3 commits January 12, 2021 01:08
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
@nickolaev nickolaev marked this pull request as ready for review January 12, 2021 08:59
@nickolaev nickolaev requested a review from a team as a code owner January 12, 2021 08:59
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

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

some nits, looks great!

switch lbConfig.HashFunction {
case "XX_HASH", "MURMUR_HASH_2":
default:
root := validators.RootedAt("conf.loadBalancer.ringHash")
Copy link
Contributor

Choose a reason for hiding this comment

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

conf.loadBalancer.ringHash.hashFunction

var _ ClusterConfigurer = &LbConfigurer{}

func (e *LbConfigurer) Configure(c *envoy_api.Cluster) error {
// default ot Round Robin
Copy link
Contributor

Choose a reason for hiding this comment

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

ot -> to

}),
Entry("least request", testCase{
clusterName: "backend",
lb: &v1alpha1.TrafficRoute_LoadBalancer{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: alias v1alpha1 to mesh_proto

@jakubdyszkiewicz
Copy link
Contributor

(and it's feat(kuma-cp))

@nickolaev nickolaev changed the title feat(*) user specified load balancer type per route feat(kuma-cp) user specified load balancer type per route Jan 12, 2021
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
@lobkovilya
Copy link
Contributor

I think it might be confusing that loadBalancer field is located on the top level of conf. It feels like this load balancer will be used to balance traffic across destinations, but in fact, we don't balance across the destinations, moreover, we have a NO_FALLBACK policy between these destinations.

I'd suggest renaming this top level loadBalancer to defaultLoadBalancer and have loadBalancer field for every destination:

type: TrafficRoute
name: route-all-default
mesh: default
sources:
  - match:
      kuma.io/service: '*'
destinations:
  - match:
      kuma.io/service: '*'
conf:
  defaultLoadBalancer:
    roundRobin: {}
  split:
    - destination:
        kuma.io/service: backend
        version: 1
      weight: 90
      loadBalancer: 
        maglev: {}
    - destination:
        kuma.io/service: backend
        version: 2
      weight: 5
      loadBalancer:
        random: {}
    - destination:
        kuma.io/service: backend
        version: 3
      weight: 5

What do you think about it?

@jakubdyszkiewicz
Copy link
Contributor

@lobkovilya
LB is placed on cluster, how would you like to place it on individual splits?

@lobkovilya
Copy link
Contributor

Sorry, I was confused by this line:

subset.Lb = route.Spec.GetConf().GetLoadBalancer()

nevermind

@nickolaev
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jan 13, 2021

Command update: success

Branch has been successfully updated

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ nickolaev
❌ mergify[bot]
You have signed the CLA already but the status is still pending? Let us recheck it.

@nickolaev nickolaev merged commit 8cffbaa into master Jan 13, 2021
@nickolaev nickolaev deleted the feat/traffic_route_lb branch January 13, 2021 09:54
mergify bot pushed a commit that referenced this pull request Jan 13, 2021
* feat(*) adding loadbalancer type to TrafficRoute

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
(cherry picked from commit 8cffbaa)
nickolaev pushed a commit that referenced this pull request Jan 13, 2021
)

* feat(*) adding loadbalancer type to TrafficRoute

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
(cherry picked from commit 8cffbaa)

Co-authored-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
austince pushed a commit to hvydya/kuma that referenced this pull request Jan 17, 2021
* feat(*) adding loadbalancer type to TrafficRoute

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
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.

Support for consistent hash based load balancing
4 participants