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: Configure LRS in CDS response #7953

Merged
merged 6 commits into from
Sep 20, 2019
Merged

cds: Configure LRS in CDS response #7953

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8d3285e
Add configuration of LRS in CDS response.
markdroth Aug 16, 2019
6dd7370
pacify spelling checker
markdroth Aug 16, 2019
cc3906d
Add comments.
markdroth Aug 19, 2019
1c66fa6
Merge remote-tracking branch 'upstream/master' into cds_lrs_config
markdroth Sep 5, 2019
a3e0810
Leave negotiation in the LRS stream.
markdroth Sep 17, 2019
6fa9e39
Update comment.
markdroth Sep 19, 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
17 changes: 16 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: 42]
// [#comment:next free field: 43]
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 @@ -657,6 +657,21 @@ message Cluster {
// :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>`.
LoadBalancingPolicy load_balancing_policy = 41;

// [#not-implemented-hide:]
// If present, tells the client where to send load reports via LRS. If not present, the
// client will fall back to a client-side default, which may be either (a) don't send any
// load reports or (b) send load reports for all clusters to a single default server.
//
// Note that if multiple clusters point to the same LRS server, the client may choose to
// create a separate stream for each cluster or it may choose to coalesce the data for
// multiple clusters onto a single stream. Either way, the client must make sure to send
Copy link
Member

Choose a reason for hiding this comment

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

Why is this the case? It would seem reasonable (but potentially a complication the client can opt into) to report on multiple streams for a given cluster. This basically depends on what the LRS server is asking for.

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 added this to mitigate the possibility of accidentally double-reporting data for the same cluster, since we're configuring the LRS server to use on a per-cluster basis but leaving the cluster negotiation in the LRS stream. If CDS returns two clusters pointing to the same LRS server and the client implementation chooses to create a separate LRS stream for each cluster rather than coalesce them into a single stream, then even if the LRS server asks for data for both clusters on both streams, each stream needs to report the data only for the cluster it was created for.

As far as I know, we don't have any use-case where we'd want to send the same data to multiple LRS servers, and that's not functionality that we provide today with LRS configured in the bootstrap file, so I'd prefer to not complicate this by trying to address that hypothetical case. But if we do need to do this for some reason, I would prefer to have a way to allow the user to explicitly specify that intention (e.g., by making this new field a repeated field, so that multiple LRS servers can be configured) rather than relaxing the restriction I've described here, because this is still not something that we want happening by accident.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, fair enough. I definitely want to see the CDS-LRS relationship cleaned up in the future.

// the data for any given cluster on no more than one stream.
//
// [#next-major-version: In the v3 API, we should consider restructuring this somehow,
// maybe by allowing LRS to go on the ADS stream, or maybe by moving some of the negotiation
// from the LRS stream here.]
core.ConfigSource lrs_server = 42;
}

// [#not-implemented-hide:] Extensible load balancing policy configuration.
Expand Down
1 change: 1 addition & 0 deletions tools/spelling_dictionary.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ LDS
LEV
LHS
LF
LRS
MB
MD
MGET
Expand Down