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.

Sign up for GitHub

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

Jump to bottom

cds: Add general-purpose LB policy configuration #7744

Merged
merged 15 commits into from
Sep 5, 2019
Merged

cds: Add general-purpose LB policy configuration #7744

Changes from 11 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a23fd06
cds: Add general-purpose LB policy configuration.
markdroth Jul 27, 2019
405f55e
Fix field number conflict.
markdroth Jul 27, 2019
b4e1249
Update next-free-field comment.
markdroth Jul 27, 2019
9c53fc2
Fix formatting.
markdroth Jul 27, 2019
e9efa81
Add locality-picking policy and update comments.
markdroth Aug 16, 2019
7ed94eb
Don't explicitly separate locality and endpoint picking. Use nesting …
markdroth Aug 16, 2019
820cc92
Code review changes.
markdroth Aug 19, 2019
3eb2cbe
Trigger use of new mechanism via a new enum value in lb_policy.
markdroth Aug 21, 2019
c2ff7a2
Remove now-unnecessary duplication of CommonLbConfig.
markdroth Aug 21, 2019
7a31c45
Change enum name and add not-implemented-yet comment.
markdroth Aug 26, 2019
5d61da6
Merge remote-tracking branch 'upstream/master' into cds_lb_policy
markdroth Aug 26, 2019
0adc2d4
Fix not-implemented documentation tag.
markdroth Aug 27, 2019
1e2958e
Add note about hierarchical LB policies.
markdroth Aug 30, 2019
593d145
Fix doc reference.
markdroth Aug 30, 2019
c233712
address review comments
markdroth Sep 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion api/envoy/api/v2/cds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ service ClusterDiscoveryService {
// [#protodoc-title: Clusters]

// Configuration for a single upstream cluster.
// [#comment:next free field: 41]
// [#comment:next free field: 42]
message Cluster {
// Supplies the name of the cluster which must be unique across all clusters.
// The cluster name is used when emitting
Expand Down Expand Up @@ -175,6 +175,15 @@ message Cluster {
// specific load balancer. Consult the configured cluster's documentation for whether to set
// this option or not.
CLUSTER_PROVIDED = 6;

// Use the new :ref:`load_balancing_policy
// <envoy_api_field_Cluster.load_balancing_policy>` field to
// determine the LB policy.
// [#next-major-version: In the v3 API, we should consider
// deprecating the lb_policy field and instead using the new
// load_balancing_policy field as the one and only mechanism for
// configuring this.]
LOAD_BALANCING_POLICY_CONFIG = 7;
}
// The :ref:`load balancer type <arch_overview_load_balancing_types>` to use
// when picking a host in the cluster.
Expand Down Expand Up @@ -645,6 +654,35 @@ message Cluster {
// outgoing connections made by the cluster. Order matters as the filters are
// processed sequentially as connection events happen.
repeated cluster.Filter filters = 40;

// New mechanism for LB policy configuration. Used only if the
// :ref:`lb_policy<envoy_api_field_Cluster.lb_policy>` field has the value
// :ref:`LOAD_BALANCING_POLICY_CONFIG
// <envoy_api_enum_value_Cluster.LbPolicy.LOAD_BALANCING_POLICY_CONFIG>`.
// [#not-implemented-yet: This is not yet implemented in Envoy.]
htuch marked this conversation as resolved.
Show resolved Hide resolved
LoadBalancingPolicy load_balancing_policy = 41;
}

// New Load balancing policy selection.
Copy link
Member

Choose a reason for hiding this comment

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

[#not-implemented-hide:] Extensible load balancing policy configuration. would be better here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

//
// Every LB policy defined via this mechanism will be identified via a
Copy link
Member

Choose a reason for hiding this comment

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

One other question: should we tag this as experimental with https://github.com/envoyproxy/envoy/blob/master/tools/protodoc/protodoc.py#L55, providing freedom to do backwards compat breaking changes while you iterate on this with gRPC, or are you highly confident we're good to go with what is here as the stable version for v2 at least?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the probability of needing changes here is fairly low, but I agree that it doesn't hurt to call it experimental for now.

Done.

// unique name using reverse DNS notation. If the policy needs configuration
// parameters, it must define a message for its own configuration, which will
// be stored in the config field. The name of the policy will tell clients
// which type of message they should expect to see in the config field.
message LoadBalancingPolicy {
message Policy {
// Required. The name of the LB policy.
string name = 1;
// Optional config for the LB policy.
// No more than one of these two fields may be populated.
google.protobuf.Struct config = 2;
google.protobuf.Any typed_config = 3;
htuch marked this conversation as resolved.
Show resolved Hide resolved
}
// Each client will iterate over the list in order and stop at the first
// policy that it supports. This provides a mechanism for starting to use
// new LB policies that are not yet supported by all clients.
Copy link
Member

Choose a reason for hiding this comment

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

This seems very strange. Its almost saying here is an API way of fixing backward compatibility issues that any control plane should take care of. IOW, you are pushing what should be the control plane's responsibility in detecting the envoy version and shipping the right config down into the data plane. There is nothing wrong with that, except it has to be consistent throughout the API. Everywhere else in the API, we strive for backward compatibility, not forward compatibility. If we take the approach of reducing control plane overhead, then we should start apply it equally and not just in LB selection.

Copy link
Member

Choose a reason for hiding this comment

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

As a general API principle, we want to move towards a model where the control plane has to do relatively little per-client processing, so to the extent we can specify sensible fallbacks, this works. It certainly doesn't work everywhere, but for LB policy it's sensible. I agree with your concern over consistency; directionally we want to move here and will aim to do that in future major versions.

Copy link
Member

Choose a reason for hiding this comment

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

but its not that simple. Imagine control plane shipping a 2x set of listeners (set 1 with old field formats/algorithm choices, etc., set 2 with newer ones), where new ones have backwards incompatible fields. Per your (new) principle, older Envoy should ignore those erroneous listeners and only take in listeners that actually work, because this older Envoy has "forward compatibility". Now, the newer Envoy also has backwards compatibility. So, this newer Envoy will be able to process both the older set and the newer set, both of which have potentially the same settings (like binding to same addresses, or what not). So, which version is the newer envoy going to pick? Old or new or merge both ?

this is a construed scenario but you get the idea. This forward compatibility and backward compatibility has far reaching implications than just the LB. If we cannot allow forward compatibility (other than simply ignoring unknown fields), then we shouldn't be allowing it here either. If we can allow forward compatibility across the board, then we should figure out the backward compatibility issue before taking this big step. Else, this will become one of those futuristic api additions that never reaches reality, and forever confuses users as to the utility of the field.

Copy link
Member

Choose a reason for hiding this comment

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

Put another way, to sovle this forward+backward compat issue, the control plane will have to know the version of the client to ship it the only latest possible configuration. At that point, the goal of this complexity introduced in the data plane API becomes moot as the primary target user (the control plane) cannot use it in reliable fashion.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the point about backwards incompatible fields. We're planning on ending completely backwards incompatible changes within a major version, see #6271, this is what we're doing in v3.

Copy link
Member

Choose a reason for hiding this comment

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

@rshriram are you OK with us merging this as is for v2?

repeated Policy policies = 1;
}

// An extensible structure containing the address Envoy should bind to when
Expand Down