Prefer the load_balancing_policy cluster field over lb_policy by temawi · Pull Request #18419 · envoyproxy/envoy

temawi · 2021-10-04T23:07:31Z

This change updates Envoy to consider the load balancing configuration
set in load_balancing_policy, regardless of what is set in lb_policy.
Previously the load_balancing_policy field was only considered if lb_policy
was set to LOAD_BALANCING_POLICY_CONFIG.

Going forward the approach is to only use the load_balancing_policy field
and the extensible configuration mechanism it provides to configure
all load balancing policies, which makes lb_policy deprecated.

This change still preserves backward compatibility, allowing old clients
to continue using the lb_policy field and new ones to switch to just
considering load_balancing_policy.

Commit Message: Prefer the load_balancing_policy cluster field over lb_policy
Additional Description: This change updates Envoy to consider the load
balancing configuration set in load_balancing_policy, regardless of what
is set in lb_policy.
Risk Level: Low
Testing: New unit tests for upstream_impl.cc
Docs Changes: Documentation to follow once consensus on this
change is reached.
Release Notes: Not currently user impacting as backward compatibility
is maintained.
Platform Specific Features: None.
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2021-10-04T23:07:33Z

Hi @temawi, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #18419 was opened by temawi.

see: more, trace.

repokitteh-read-only · 2021-10-04T23:07:34Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #18419 was opened by temawi.

see: more, trace.

repokitteh-read-only · 2021-10-04T23:07:38Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #18419 was opened by temawi.

see: more, trace.

api/envoy/config/cluster/v3/cluster.proto

temawi · 2021-10-04T23:11:45Z

api/envoy/config/cluster/v3/cluster.proto

Naming this is hard! This is the best I could think of, but it's still a bit confusing as it's somewhat overloaded with the LOAD_BALANCING_POLICY_CONFIG enum value.

Yeah, this name is probably fine.

Please add a [#not-implemented-hide:] annotation here to hide this from the Envoy docs until it is actually implemented in Envoy.

I'd like to push back on the name a bit. I feel it's just too easy to confuse this with the existing same-typed field.

Why can't we just change semantics of the existing field such that if a non-LOAD_BALANCING_POLICY_CONFIG policy is set, and load_balancing_policy is set as well, that we have the intended fallback behaviors?

This would have the same behavior as today if LOAD_BALANCING_POLICY_CONFIG and load_balancing_policy are set and still provide the desired fallback behavior for older clients?

That sounds okay to me. The only real impact of that would be that if the control plane populates the load_balancing_policy field but does not set the enum to LOAD_BALANCING_POLICY_CONFIG, then very recent versions of Envoy that include #17400 would react as if they did not support load_balancing_policy at all.

@pianiststickman, does that sound okay to you?

I believe we have consensus on reusing the existing load_balancing_config field. Old clients will look at the field if LOAD_BALANCING_POLICY_CONFIG is set in lb_policy, but this approach is getting marked as deprecated in this PR. The approach going forward is to only consider load_balancing_config for all LB configuration. Please take a look if the changes I made look ok.

Sorry for the late reply - yes, that sounds fine to me. AIUI there is no change if the management server still populates load_balancing_policy and continues to set the enum to LOAD_BALANCING_POLICY_CONFIG.

Of course that raises a different question - if eventually the client will just look at load_balancing_policy and not at the enum value, then does LOAD_BALANCING_POLICY_CONFIG really have a purpose? Should it be deprecated?

No, LOAD_BALANCING_POLICY_CONFIG is no longer needed. We're deprecating it as part of this PR.

markdroth

Thanks for doing this!

Note that you'll need to run ./ci/run_envoy_docker.sh './ci/do_ci.sh fix_format' to generate the shadow proto files and add the changes to this PR.

Please let me know if you have any questions. Thanks!

api/envoy/config/cluster/v3/cluster.proto

markdroth · 2021-10-07T16:52:51Z

api/envoy/config/cluster/v3/cluster.proto

Yeah, this name is probably fine.

Please add a [#not-implemented-hide:] annotation here to hide this from the Envoy docs until it is actually implemented in Envoy.

api/envoy/config/cluster/v3/cluster.proto

htuch · 2021-10-11T15:45:49Z

api/envoy/config/cluster/v3/cluster.proto

No value at all of what?

markdroth

I'm wondering if we can make the code change to Envoy to always prefer load_balancing_policy to lb_policy as part of this PR. Ideally, that should be fairly simple to do, but if we let this go for a long time before making that change, it will make life much harder for control planes.

markdroth · 2021-10-12T15:19:26Z

api/envoy/config/cluster/v3/cluster.proto

I think we should just change this comment to say:

If this field is set and is supported by the client, it will supercede the value of :ref:`lb_policy<envoy_v3_api_field_config.cluster.v3.Cluster.lb_policy>`.

htuch · 2021-10-13T03:24:22Z

To @markdroth's point, the change to do this in Envoy would be very small, probably a few lines in upstream_impl.cc where load_balancing_policy is referenced and upstream_impl_test.cc unit tests.

repokitteh-read-only · 2021-10-18T23:06:22Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #18419 was synchronize by temawi.

see: more, trace.

temawi · 2021-10-21T21:38:23Z

I'm wondering if we can make the code change to Envoy to always prefer load_balancing_policy to lb_policy as part of this PR. Ideally, that should be fairly simple to do, but if we let this go for a long time before making that change, it will make life much harder for control planes.

I made the change and added some unit tests.

htuch · 2021-10-22T04:46:02Z

@lizan can you provide non-Googler API and implementation review coverage on this one? Thanks.

markdroth · 2021-10-25T17:33:50Z

Thanks for making that change -- this looks great!

It looks like there are some CI failures to be resolved.

lizan · 2021-10-25T21:43:34Z

Changes looks good, can you fix DCO?

This adds a new LB policy mechanism over the existing lb_policy and load_balancing_policy fields, which it deprecates. This change allows for a more flexible configuration of defining a heterogenous list of built-in and custom load balancers while still preserving backward compatibility, allowing the old fields to be used by older clients . Signed-off-by: Terry Wilson <tmwilson@google.com>

Signed-off-by: Terry Wilson <tmwilson@google.com>

Not supported in enum values. Signed-off-by: Terry Wilson <tmwilson@google.com>

Signed-off-by: Terry Wilson <tmwilson@google.com>

temawi · 2021-10-26T03:45:10Z

@lizan DCO is now taken care of but I ended up dismissing your approval by making some formatting changes. Can you re-approve, please?

repokitteh-read-only bot added the api label Oct 4, 2021

repokitteh-read-only bot assigned markdroth Oct 4, 2021

temawi commented Oct 4, 2021

View reviewed changes

api/envoy/config/cluster/v3/cluster.proto Outdated Show resolved Hide resolved

temawi commented Oct 4, 2021

View reviewed changes

api/envoy/config/cluster/v3/cluster.proto Outdated Show resolved Hide resolved

temawi commented Oct 4, 2021

View reviewed changes

markdroth suggested changes Oct 7, 2021

View reviewed changes

htuch reviewed Oct 11, 2021

View reviewed changes

api/envoy/config/cluster/v3/cluster.proto Outdated

Copy link
Copy Markdown

Member

htuch Oct 11, 2021

Choose a reason for hiding this comment

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

No value at all of what?

markdroth reviewed Oct 12, 2021

View reviewed changes

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Oct 18, 2021

temawi force-pushed the new_lb_policy branch from 29066c0 to 09441b9 Compare October 19, 2021 20:40

temawi marked this pull request as ready for review October 21, 2021 21:39

htuch assigned lizan Oct 22, 2021

temawi changed the title ~~config: add new field for load balancing policy.~~ Prefer the load_balancing_policy cluster field over lb_policy Oct 25, 2021

lizan previously approved these changes Oct 25, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Oct 25, 2021

temawi dismissed lizan’s stale review via de45fa2 October 25, 2021 22:38

repokitteh-read-only bot added the api label Oct 25, 2021

temawi added 4 commits October 26, 2021 00:24

Deprecate LOAD_BALANCING_POLICY_CONFIG.

6bb7e25

Signed-off-by: Terry Wilson <tmwilson@google.com>

Update load_balancing_policy comment.

dc533e3

Signed-off-by: Terry Wilson <tmwilson@google.com>

temawi added 5 commits October 26, 2021 00:24

Remove deprecation annotation from enum value.

bcd89e4

Not supported in enum values. Signed-off-by: Terry Wilson <tmwilson@google.com>

load_balancing_policy to be preferred over lb_policy.

3aa9dc0

Signed-off-by: Terry Wilson <tmwilson@google.com>

Fix typo and missing last line

df7fab2

Signed-off-by: Terry Wilson <tmwilson@google.com>

Remove trailing whitespace.

bd2fe73

Signed-off-by: Terry Wilson <tmwilson@google.com>

Fix formatting.

4f41342

Signed-off-by: Terry Wilson <tmwilson@google.com>

temawi force-pushed the new_lb_policy branch from de45fa2 to 4f41342 Compare October 26, 2021 00:24

lizan approved these changes Oct 27, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Oct 27, 2021

lizan merged commit bc641db into envoyproxy:main Oct 27, 2021

markdroth mentioned this pull request Apr 1, 2022

xds: Envoy should support its built-in LB policies as extensions #20634

Open

puertomontt mentioned this pull request Aug 14, 2025

Refactor load balancer plugin to use loadBalancingPolicy kgateway-dev/kgateway#12000

Merged

Conversation

temawi commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Oct 4, 2021

Uh oh!

repokitteh-read-only bot commented Oct 4, 2021

Uh oh!

repokitteh-read-only bot commented Oct 4, 2021

Uh oh!

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!

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!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Oct 13, 2021

Uh oh!

repokitteh-read-only bot commented Oct 18, 2021

Uh oh!

temawi commented Oct 21, 2021

Uh oh!

htuch commented Oct 22, 2021

Uh oh!

markdroth commented Oct 25, 2021

Uh oh!

lizan commented Oct 25, 2021

Uh oh!

temawi commented Oct 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

temawi commented Oct 4, 2021 •

edited

Loading