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

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 7 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
118 changes: 112 additions & 6 deletions api/envoy/api/v2/cds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ service ClusterDiscoveryService {
// [#protodoc-title: Clusters]

// Configuration for a single upstream cluster.
// [#comment:next free field: 40]
// [#comment:next free field: 41]
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,7 +175,14 @@ message Cluster {
}
// The :ref:`load balancer type <arch_overview_load_balancing_types>` to use
// when picking a host in the cluster.
LbPolicy lb_policy = 6 [(validate.rules).enum.defined_only = true];
//
// .. attention::
//
// **This field is deprecated**. Set the
// :ref:`load_balancing_policy<envoy_api_field_Cluster.load_balancing_policy>`
// field instead.
//
LbPolicy lb_policy = 6 [(validate.rules).enum.defined_only = true, deprecated = true];

// If the service discovery type is
// :ref:`STATIC<envoy_api_enum_value_Cluster.DiscoveryType.STATIC>`,
Expand Down Expand Up @@ -448,6 +455,10 @@ message Cluster {
// The number of random healthy hosts from which the host with the fewest active requests will
// be chosen. Defaults to 2 so that we perform two-choice selection if the field is not set.
google.protobuf.UInt32Value choice_count = 1 [(validate.rules).uint32.gte = 2];
// Populated only when the LB policy is configured via the new
// :ref:`load_balancing_policy<envoy_api_field_Cluster.load_balancing_policy>`
// field.
CommonLbConfig common_fields = 2;
}

// Specific configuration for the :ref:`RingHash<arch_overview_load_balancing_types_ring_hash>`
Expand Down Expand Up @@ -479,6 +490,11 @@ message Cluster {
// to further constrain resource use. See also
// :ref:`minimum_ring_size<envoy_api_field_Cluster.RingHashLbConfig.minimum_ring_size>`.
google.protobuf.UInt64Value maximum_ring_size = 4 [(validate.rules).uint64.lte = 8388608];

// Populated only when the LB policy is configured via the new
// :ref:`load_balancing_policy<envoy_api_field_Cluster.load_balancing_policy>`
// field.
CommonLbConfig common_fields = 5;
}

// Specific configuration for the
Expand All @@ -495,6 +511,11 @@ message Cluster {
// route traffic to arbitrary hosts and/or ports, which may have serious security
// consequences.
bool use_http_header = 1;

// Populated only when the LB policy is configured via the new
// :ref:`load_balancing_policy<envoy_api_field_Cluster.load_balancing_policy>`
// field.
CommonLbConfig common_fields = 2;
}

// Optional configuration for the load balancing algorithm selected by
Expand All @@ -504,13 +525,20 @@ message Cluster {
// has additional configuration options.
// Specifying ring_hash_lb_config or least_request_lb_config without setting the corresponding
// LbPolicy will generate an error at runtime.
//
// .. attention::
//
// **These fields are deprecated**. Set the
// :ref:`load_balancing_policy<envoy_api_field_Cluster.load_balancing_policy>`
// field instead.
//
oneof lb_config {
// Optional configuration for the Ring Hash load balancing policy.
RingHashLbConfig ring_hash_lb_config = 23;
RingHashLbConfig ring_hash_lb_config = 23 [deprecated = true];
// Optional configuration for the Original Destination load balancing policy.
OriginalDstLbConfig original_dst_lb_config = 34;
OriginalDstLbConfig original_dst_lb_config = 34 [deprecated = true];
// Optional configuration for the LeastRequest load balancing policy.
LeastRequestLbConfig least_request_lb_config = 37;
LeastRequestLbConfig least_request_lb_config = 37 [deprecated = true];
}

// Common configuration for all load balancer implementations.
Expand Down Expand Up @@ -583,7 +611,14 @@ message Cluster {
}

// Common configuration for all load balancer implementations.
CommonLbConfig common_lb_config = 27;
//
// .. attention::
//
// **This field is deprecated**. Set the
// :ref:`load_balancing_policy<envoy_api_field_Cluster.load_balancing_policy>`
// field instead.
//
CommonLbConfig common_lb_config = 27 [deprecated = true];

// Optional custom transport socket implementation to use for upstream connections.
core.TransportSocket transport_socket = 24;
Expand Down Expand Up @@ -632,6 +667,77 @@ message Cluster {
// If this flag is not set to true, Envoy will wait until the hosts fail active health
// checking before removing it from the cluster.
bool drain_connections_on_host_removal = 32;

// New mechanism for LB policy configuration. If set and understood
// by the client, replaces the
// :ref:`lb_policy<envoy_api_field_Cluster.lb_policy>` field, the
// :ref:`common_lb_config<envoy_api_field_Cluster.common_lb_config>`
// field, and the fields in the
// :ref:`lb_config<envoy_api_field_Cluster.lb_config>` oneof. For backward
// compatibility, management servers should populate both sets of fields.
htuch marked this conversation as resolved.
Show resolved Hide resolved
LoadBalancingPolicy load_balancing_policy = 40;
}

// Load balancing policy selection.
//
// 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.
//
// Envoy supports the following LB policies, each of which has its own
// configuration message. Refer to
// :ref:`load balancer type <arch_overview_load_balancing_types>` architecture
// overview section for information on each type.
//
// "io.envoyproxy.weighted_round_robin":
htuch marked this conversation as resolved.
Show resolved Hide resolved
// Config type: :ref:`CommonLbConfig<envoy_api_msg_Cluster.CommonLbConfig>`
// Refer to the :ref:`round robin load balancing
// policy<arch_overview_load_balancing_types_round_robin>`
// for an explanation.
//
// "io.envoyproxy.least_request":
// Config type: :ref:`LeastRequestLbConfig<envoy_api_msg_Cluster.LeastRequestLbConfig>`
// Refer to the :ref:`least request load balancing
// policy<arch_overview_load_balancing_types_least_request>`
// for an explanation.
//
// "io.envoyproxy.ring_hash":
// Config type: :ref:`RingHashLbConfig<envoy_api_msg_Cluster.RingHashLbConfig>`
// Refer to the :ref:`ring hash load balancing
// policy<arch_overview_load_balancing_types_ring_hash>`
// for an explanation.
//
// "io.envoyproxy.random":
// Config type: :ref:`CommonLbConfig<envoy_api_msg_Cluster.CommonLbConfig>`
// Refer to the :ref:`random load balancing
// policy<arch_overview_load_balancing_types_random>`
// for an explanation.
//
// "io.envoyproxy.original_dst":
// Config type: :ref:`OriginalDstLbConfig<envoy_api_msg_Cluster.OriginalDstLbConfig>`
// Refer to the :ref:`original destination load balancing
// policy<arch_overview_load_balancing_types_original_destination>`
// for an explanation.
//
// "io.envoyproxy.maglev":
// Config type: :ref:`CommonLbConfig<envoy_api_msg_Cluster.CommonLbConfig>`
// Refer to the :ref:`Maglev load balancing
// policy<arch_overview_load_balancing_types_maglev>` for an explanation.
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