lb api: moving load balancing policy specific configuration to extension configuration by wbpcode · Pull Request #23967 · envoyproxy/envoy

wbpcode · 2022-11-12T13:38:44Z

Commit Message: lb api: load balancing policy specific configuration to extension configuration
Additional Description:
Risk Level: low. api only.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

…figuration Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

repokitteh-read-only · 2022-11-12T13:38:55Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #23967 was opened by wbpcode.

see: more, trace.

wbpcode · 2022-11-12T13:42:46Z

This is part of #20634.

This PR moved the zone_aware_config and locality_weighted_lb_config from common_lb_config. to least request, round rabin and random's extension configuration.

And consistent_hashing_lb_config is moved to maglev's extension configuration.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

wbpcode · 2022-11-12T13:53:23Z

cc @markdroth

is it possible for us to use ConsistentHashingLbConfig in the ring hash directly? Or to use envoy.extensions.load_balancing_policies.common.v3.SlowStartConfig in the configuration of least request or round ranbin directly?

I know we cann't update any API directly according to our API version policy even it's marked as not-implemented-hide.
But seems that it would be better to use envoy.extensions.load_balancing_policies.common.v3.SlowStartConfig rather than config.cluster.v3.Cluster.SlowStartConfig.
And the ring hash should shared the same ConsistentHashingLbConfig with the maglev.

So, if it's possible to change the API directly (as a special case)?

markdroth

Thanks for doing this!

markdroth · 2022-11-28T19:09:04Z

api/envoy/extensions/load_balancing_policies/least_request/v3/least_request.proto

@@ -56,4 +57,10 @@ message LeastRequest {
  // Configuration for slow start mode.
  // If this configuration is not set, slow start will not be not enabled.
  config.cluster.v3.Cluster.SlowStartConfig slow_start_config = 3;


I guess this should also switch to common.v3.SlowStartConfig. Technically, that's a breaking change, but I suspect no one is actually using this yet, so that's probably okay.

@temawi, please confirm that this change would not break anything for grpc-java.

markdroth · 2022-11-28T19:10:18Z

api/envoy/extensions/load_balancing_policies/least_request/v3/least_request.proto

 // This configuration allows the built-in LEAST_REQUEST LB policy to be configured via the LB policy
 // extension point. See the :ref:`load balancing architecture overview
 // <arch_overview_load_balancing_types>` for more information.
-// [#extension: envoy.load_balancing_policies]


Why remove these extension comments? I think they're needed to generate the right documentation.

These comments are wrong. The [#extension] comment should be complete name of extension.
I will ensure all these are correct when I implement these extensions.

api/envoy/extensions/load_balancing_policies/maglev/v3/maglev.proto

markdroth · 2022-11-28T19:12:42Z

api/envoy/extensions/load_balancing_policies/round_robin/v3/round_robin.proto

 message RoundRobin {
  // Configuration for slow start mode.
  // If this configuration is not set, slow start will not be not enabled.
  config.cluster.v3.Cluster.SlowStartConfig slow_start_config = 1;


Same comment as above w.r.t. changing this to use common.v3.SlowStartConfig.

markdroth · 2022-11-28T19:15:32Z

api/envoy/extensions/load_balancing_policies/least_request/v3/least_request.proto

  // If this configuration is not set, slow start will not be not enabled.
  config.cluster.v3.Cluster.SlowStartConfig slow_start_config = 3;
+
+  oneof locality_config_specifier {


Looks like we're repeating this oneof in 3 different places. Would it make sense to define a new message in common.proto that contains this oneof, and then just have a single field of that new message type in each of these three places?

Sounds great.

markdroth · 2022-11-28T19:25:56Z

Sorry, didn't see your questions above until after I'd reviewed. Looks like we're thinking along the same lines, though.

is it possible for us to use ConsistentHashingLbConfig in the ring hash directly?

In my review, I had actually suggested the opposite, which is just inlining the fields in the maglev config, just like we did in the ring hash config. That having been said, I am open to instead changing the ring_hash config to use ConsistentHashingLbConfig if we think it's likely that either (a) we will want to be able to add new fields to ConsistentHashingLbConfig in the future or (b) we think there will be more hash-based LB policies in the future that will want to share these fields.

In terms of API breakage policy, we can definitely add a new ConsistentHashingLbConfig field to the ring hash config if we want; the only question would be what to do about the existing inlined fields. Technically, we can't remove them, but we can mark them as deprecated. In practice, I suspect nothing would break if we simply removed them, since gRPC does not use those fields, but we don't really lose anything by leaving them there either.

Or to use envoy.extensions.load_balancing_policies.common.v3.SlowStartConfig in the configuration of least request or round ranbin directly?

As per my review comments, this one is a bit trickier. Technically, changing the type of the field is a breaking change. However, I don't think gRPC uses these fields at all, and Envoy hasn't implemented these new extensions yet, so I don't think anything would break if we just made this change.

I'd be okay with allowing this as a one-off exception to our breakage policy, if none of the other @envoyproxy/api-shepherds objects.

wbpcode · 2022-11-29T01:17:06Z

Thanks for you review comments. @markdroth

(a) we will want to be able to add new fields to ConsistentHashingLbConfig in the future or (b) we think there will be more hash-based LB policies in the future that will want to share these fields.

This is what I am worried about.

I will create new commit to address your comments.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

…ore-lb-api-to-extension

markdroth

This looks great to me!

api/envoy/extensions/load_balancing_policies/common/v3/common.proto

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

markdroth · 2022-11-30T15:48:14Z

/lgtm api

wbpcode · 2022-12-01T01:26:12Z

/assign-from @envoyproxy/senior-maintainers

A LGTM is necessary for this API only PR.

repokitteh-read-only · 2022-12-01T01:26:17Z

@envoyproxy/senior-maintainers assignee is @htuch

🐱

Caused by: a #23967 (comment) was created by @wbpcode.

see: more, trace.

wbpcode · 2022-12-07T01:32:41Z

/assign-from @envoyproxy/senior-maintainers

@htuch is out for vacation

repokitteh-read-only · 2022-12-07T01:32:47Z

@envoyproxy/senior-maintainers assignee is @snowp

🐱

Caused by: a #23967 (comment) was created by @wbpcode.

see: more, trace.

wbpcode · 2022-12-07T01:33:36Z

cc @snowp could you give a LGTM to this API only PR when you have free time? Thanks.

snowp

Makes sense to me, just one comment

snowp · 2022-12-07T01:36:07Z

api/envoy/extensions/load_balancing_policies/ring_hash/v3/ring_hash.proto

+    (envoy.annotations.deprecated_at_minor_version) = "3.0"
+  ];
+
+  common.v3.ConsistentHashingLbConfig consistent_hashing_lb_config = 6;


Docs for this here and for the other lib configs?

Most docs are comments of inner fields of common.v3.ConsistentHashingLbConfig. And simple comments to consistent_hashing_lb_config are added.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

…ore-lb-api-to-extension

snowp

Thanks!

….h-into-multiple-header-files * origin/main: Add setRequestDecoder to ResponseEncoder interface (#24368) downstream: refactoring code to remove listener hard deps (#24394) lb api: moving load balancing policy specific configuration to extension configuration (#23967) ci: Skip docker/examples verification for docs or mobile only changes (#24417) ci: run mobile GitHub Actions on every PR (#24407) mobile: remove `bump_lyft_support_rotation.sh` script (#24404) Add file size to DirectoryEntry (#24176) bazel: update to 6.0.0rc4 (#24235) bazel: update rules_rust (#24409) Ecds config dump recommit (#24384) bazel: add another config_setting incompatible flag (#24270) listeners: moving listeners to extension directory (#24248) mobile: build Swift with whole module optimization (#24396) ci: update `actions/setup-java` from v1 to v3.8 (#24393) Signed-off-by: JP Simard <jp@jpsim.com>

…-cpp-to-latest-version * origin/main: (23 commits) Reduce Route memory utilization by avoiding RuntimeData instances when not needed (#24327) build: fix compile error for mac (#24429) postgres: support for upstream SSL (#23990) iOS: split `EnvoyEngine.h` into multiple header files (#24397) mobile: check for pending exceptions after JNI call (#24361) Remove uneccessary `this->` from mobile engine builder (#24389) Add setRequestDecoder to ResponseEncoder interface (#24368) downstream: refactoring code to remove listener hard deps (#24394) lb api: moving load balancing policy specific configuration to extension configuration (#23967) ci: Skip docker/examples verification for docs or mobile only changes (#24417) ci: run mobile GitHub Actions on every PR (#24407) mobile: remove `bump_lyft_support_rotation.sh` script (#24404) Add file size to DirectoryEntry (#24176) bazel: update to 6.0.0rc4 (#24235) bazel: update rules_rust (#24409) Ecds config dump recommit (#24384) bazel: add another config_setting incompatible flag (#24270) listeners: moving listeners to extension directory (#24248) mobile: build Swift with whole module optimization (#24396) ci: update `actions/setup-java` from v1 to v3.8 (#24393) ... Signed-off-by: JP Simard <jp@jpsim.com>

lb api: load balancing policy specific configuration to extension con…

ed5a03b

…figuration Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

repokitteh-read-only bot added the api label Nov 12, 2022

wbpcode assigned markdroth Nov 12, 2022

repokitteh-read-only bot assigned adisuissa Nov 12, 2022

fix format

06b27ae

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

wbpcode changed the title ~~lb api: load balancing policy specific configuration to extension configuration~~ lb api: moving load balancing policy specific configuration to extension configuration Nov 13, 2022

markdroth reviewed Nov 28, 2022

View reviewed changes

wbpcode added 2 commits November 29, 2022 12:37

address review comments

0b54dfc

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-m…

b24c064

…ore-lb-api-to-extension

markdroth reviewed Nov 29, 2022

View reviewed changes

api/envoy/extensions/load_balancing_policies/common/v3/common.proto Outdated Show resolved Hide resolved

address comment and fix format

cea20a7

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

repokitteh-read-only bot removed the api label Nov 30, 2022

repokitteh-read-only bot assigned htuch Dec 1, 2022

wbpcode unassigned htuch Dec 7, 2022

repokitteh-read-only bot assigned snowp Dec 7, 2022

snowp suggested changes Dec 7, 2022

View reviewed changes

address comments

181a140

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

repokitteh-read-only bot added the api label Dec 7, 2022

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-m…

c17327e

…ore-lb-api-to-extension

snowp approved these changes Dec 7, 2022

View reviewed changes

snowp merged commit 50ac04b into envoyproxy:main Dec 7, 2022

Conversation

wbpcode commented Nov 12, 2022

Uh oh!

repokitteh-read-only bot commented Nov 12, 2022

Uh oh!

wbpcode commented Nov 12, 2022

Uh oh!

wbpcode commented Nov 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markdroth 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!

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!

markdroth commented Nov 28, 2022

Uh oh!

wbpcode commented Nov 29, 2022

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markdroth commented Nov 30, 2022

Uh oh!

wbpcode commented Dec 1, 2022

Uh oh!

repokitteh-read-only bot commented Dec 1, 2022

Uh oh!

wbpcode commented Dec 7, 2022

Uh oh!

repokitteh-read-only bot commented Dec 7, 2022

Uh oh!

wbpcode commented Dec 7, 2022

Uh oh!

snowp 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!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wbpcode commented Nov 12, 2022 •

edited

Loading