add maglev load balancer #2434

ramaraochavali · 2022-07-30T09:04:01Z

As discussed istio/istio#40142 adding support for MagLev loadbalancer.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2022-07-30T09:07:01Z

/test release-notes_api

howardjohn · 2022-08-01T17:20:14Z

networking/v1alpha3/destination_rule.proto

+        // considerations on choosing an algorithm.
+        oneof hash_algorithm {
+          // The ring/modulo hash load balancer implements consistent hashing to backend hosts.
+          RingHashConfig ring_hash = 6;


should we deprecate the minimum_ring_size on L626?

Yes. We can deprecate. Is it just doc update or any thing else?

howardjohn · 2022-08-01T17:20:20Z

networking/v1alpha3/destination_rule.proto

+        // Please refer to https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/load_balancers#ring-hash
+        // and https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/load_balancers#maglev for
+        // considerations on choosing an algorithm.
+        oneof hash_algorithm {


What is the default?

Default is RingHash. I will update.

howardjohn · 2022-08-01T17:20:40Z

networking/v1alpha3/destination_rule.proto

+          // The ring/modulo hash load balancer implements consistent hashing to backend hosts.
+          RingHashConfig ring_hash = 6;
+          // The Maglev load balancer implements consistent hashing to backend hosts. 
+          MagLevConfig maglev = 7;


I can set maglev without configuring the ring size? If so, how?

maglev: {}

?

yes. you can set maglev:{} and it uses the default table size.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ericvn · 2022-08-03T14:55:13Z

/test gencheck_api

howardjohn

LGTM, just two nits

cc @nmittler

howardjohn · 2022-08-04T00:01:09Z

networking/v1beta1/destination_rule.proto

+      // load distributions. If the number of hosts in the load balancing
+      // pool is larger than the ring size, each host will be assigned a
+      // single virtual node.
+      uint64 minimum_ring_size = 8 [deprecated=true];


why is this one deprecated? isn't this the new one?

yeah.. copy pasted at wrong place

howardjohn · 2022-08-04T00:01:45Z

networking/v1beta1/destination_rule.proto

+      // The table size for Maglev hashing. This helps in controlling the 
+      // disruption when the backend hosts change.
+      // Increasing the table size reduces the amount of disruption. 
+      uint64 table_size = 9;


Suggested change

uint64 table_size = 9;

uint64 table_size = 1;

new message, new number scheme (I think? or does nesting change it?). Same above

No you are right. This has to be follow new number

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

…pport

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

nmittler · 2022-08-04T15:51:57Z

networking/v1alpha3/destination_rule.proto

    // The minimum number of virtual nodes to use for the hash
    // ring. Defaults to 1024. Larger ring sizes result in more granular
    // load distributions. If the number of hosts in the load balancing
    // pool is larger than the ring size, each host will be assigned a
    // single virtual node.
-    uint64 minimum_ring_size = 4;
+    uint64 minimum_ring_size = 4 [deprecated=true];


Replace the comment with text indicating that it's deprecated and where to find the replacement value.

nmittler · 2022-08-04T15:53:31Z

networking/v1beta1/destination_rule.proto

    // The minimum number of virtual nodes to use for the hash
    // ring. Defaults to 1024. Larger ring sizes result in more granular
    // load distributions. If the number of hosts in the load balancing
    // pool is larger than the ring size, each host will be assigned a
    // single virtual node.
-    uint64 minimum_ring_size = 4;
+    uint64 minimum_ring_size = 4 [deprecated=true];


same comment here.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

hzxuzhonghu · 2022-08-05T01:30:48Z

networking/v1alpha3/destination_rule.proto

+        // Please refer to https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/load_balancers#ring-hash
+        // and https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/load_balancers#maglev for
+        // considerations on choosing an algorithm.


We should give a brief compare here, rather than letting users searching and decide

I think it is difficult to give concise description in a protobuf comment. That is why Envoy wrote that separate section.

ramaraochavali · 2022-08-05T09:49:15Z

@howardjohn @nmittler addressed comments. PTAL

linsun · 2022-08-05T15:42:30Z

should this have some type of release notes? Else LGTM

ramaraochavali · 2022-08-06T03:21:43Z

should this have some type of release notes? Else LGTM

I will add release notes when it is implemented in Istio.

ramaraochavali · 2022-08-09T04:15:46Z

@linsun @howardjohn can you please review this when you get chance?

ericvn

LGTM.

add maglev load balancer

5a0c690

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali requested review from linsun, louiscryan, nrjpoddar, howardjohn and ericvn as code owners July 30, 2022 09:04

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 30, 2022

ramaraochavali added the release-notes-none Indicates a PR that does not require release notes. label Jul 30, 2022

howardjohn reviewed Aug 1, 2022

View reviewed changes

deprecate old and rename new message

874d7b2

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn reviewed Aug 4, 2022

View reviewed changes

ramaraochavali added 3 commits August 4, 2022 20:09

fix issues

9121777

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Merge remote-tracking branch 'upstream/master' into fix/add_maglev_su…

4b7e6f1

…pport

make gen

b9bd496

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

nmittler reviewed Aug 4, 2022

View reviewed changes

update comments

8c19b65

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

hzxuzhonghu reviewed Aug 5, 2022

View reviewed changes

ericvn approved these changes Aug 9, 2022

View reviewed changes

howardjohn approved these changes Aug 9, 2022

View reviewed changes

istio-testing merged commit 5bf1a9b into istio:master Aug 9, 2022

ramaraochavali deleted the fix/add_maglev_support branch August 9, 2022 14:56

costinm mentioned this pull request Aug 11, 2022

Implement stateful session affinity to a backend istio/istio#39740

Closed

hzxuzhonghu mentioned this pull request Aug 16, 2022

add support for maglev for consistent hash load balancer istio/istio#40142

Merged

10 tasks

add maglev load balancer #2434

add maglev load balancer #2434

Uh oh!

Conversation

ramaraochavali commented Jul 30, 2022

Uh oh!

ramaraochavali commented Jul 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericvn commented Aug 3, 2022

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Aug 5, 2022

Uh oh!

linsun commented Aug 5, 2022

Uh oh!

ramaraochavali commented Aug 6, 2022

Uh oh!

ramaraochavali commented Aug 9, 2022

Uh oh!

ericvn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!