api: Add send_all_clusters field to LRS response. by markdroth · Pull Request #10613 · envoyproxy/envoy

markdroth · 2020-04-01T18:11:57Z

Signed-off-by: Mark D. Roth roth@google.com

Description: Add a send_all_clusters field to LRS response, triggered by a new client capability. This avoids the need for the server to enumerate the full list of clusters if it always wants data for all clusters.
Risk Level: Low
Testing: N/A
Docs Changes: Included in PR.
Release Notes: N/A

As discussed with @htuch and @jaychenatr.

Signed-off-by: Mark D. Roth <roth@google.com>

repokitteh-read-only · 2020-04-01T18:12:02Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10613 was opened by markdroth.

see: more, trace.

htuch

Looks good, needs fix_format.
/wait

htuch · 2020-04-01T22:47:05Z

+  // Not populated if send_all_clusters is true.
  repeated string clusters = 1 [(validate.rules).repeated = {min_items: 1}];
+  // If true, the client should send all clusters it knows about.
+  // The server may only specify this if the client advertises the


I think it's safe to set this regardless of the capability, you just might not get back the expected result.

Reworded to indicate that clients will not honor the field unless they advertise that capability.

htuch · 2020-04-01T22:47:16Z

 // [#not-implemented-hide:] Not configuration. TBD how to doc proto APIs.
 message LoadStatsResponse {
  // Clusters to report stats for.
+  // Not populated if send_all_clusters is true.


Nit: *send_all_clusters*

htuch · 2020-04-01T22:48:03Z

  :ref:`overprovisioning_factor<envoy_api_field_ClusterLoadAssignment.Policy.overprovisioning_factor>`
  field. If graceful failover functionality is required, it must be supplied by the management
  server.
+- **envoy.lrs.supports_send_all_clusters**: This feature indicates that the client supports


Do you plan on implementing this in Envoy? It's not a ton of work.

If it's something I can do fairly quickly, I can take a stab at it. Can you point me at the code that would need to be changed?

It would mean changing the loop logic in https://github.com/envoyproxy/envoy/blob/master/source/common/upstream/load_stats_reporter.cc#L48, instead of looping over known clusters from LRS and looking them up in cluster manager, we would iterate over all the clusters in cluster manager in the wildcard case.

Thanks for implementing; have you added the client feature capability announcement yet?

Done. Also added a bullet to the release notes about this.

Signed-off-by: Mark D. Roth <roth@google.com>

markdroth · 2020-04-02T14:47:38Z

I don't understand what's causing the docs CI failure. The ref I used looks like the right format -- in fact, it exactly matches the form used in a couple of places in load_report.proto itself. Is it possible that lrs.proto is simply not being loaded into RST when client_features.rst is being formatted?

htuch · 2020-04-02T18:39:20Z

@markdroth yeah, https://www.envoyproxy.io/docs/envoy/latest/api-v2/service/load_stats/v2/lrs.proto is empty. This is due to a known issue, we don't have 1st class support for generating docs for service APIs, see #3091.

I'd suggest just writing it out in natural language and leaving a [#comment:] referencing this bug.

Signed-off-by: Mark D. Roth <roth@google.com>

htuch · 2020-04-07T14:50:49Z

Please merge master to pick up #10672. We no longer accept changes to v2 (without explicit exception), so any API modifications should happen in v3. If this PR is adding a new proto, please follow the updated instructions in https://github.com/envoyproxy/envoy/blob/master/api/STYLE.md#adding-an-extension-configuration-to-the-api.

Signed-off-by: Mark D. Roth <roth@google.com>

…ters Signed-off-by: Mark D. Roth <roth@google.com>

stale · 2020-04-25T07:45:57Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Mark D. Roth <roth@google.com>

markdroth · 2020-04-27T22:35:48Z

@htuch I think everything is working here now, but the coverage CI is still failing. It's not obvious to me from the error message what the problem is. Can you please take a look and advise? Thanks!

htuch · 2020-04-28T15:16:05Z

+  // If true, the client should send all clusters it knows about.
+  // Only clients that advertise the "envoy.lrs.supports_send_all_clusters" capability in their
+  // :ref:`client_features<envoy_api_field_core.Node.client_features>` field will honor this field.
+  bool send_all_clusters = 4;


I think this should be v3-only, but I imagine this is going to cause you folks pain? @envoyproxy/api-shepherds

Yeah, unfortunately, this is something that we need in v2 for product reasons.

I'm going to have to recuse myself and ask the other @envoyproxy/api-shepherds to weigh in on whether an exception can be made here.

@lizan can we get your opinion on this one as well? Thanks.

htuch · 2020-04-28T15:16:53Z

  :ref:`overprovisioning_factor<envoy_api_field_ClusterLoadAssignment.Policy.overprovisioning_factor>`
  field. If graceful failover functionality is required, it must be supplied by the management
  server.
+- **envoy.lrs.supports_send_all_clusters**: This feature indicates that the client supports


Thanks for implementing; have you added the client feature capability announcement yet?

htuch · 2020-04-28T15:22:43Z

@markdroth you might want to merge master before/after making the suggested changes above. Odds are CI will be fixed by then.

…ters Signed-off-by: Mark D. Roth <roth@google.com>

Signed-off-by: Mark D. Roth <roth@google.com>

mattklein123

Small API comment, thank you!

Signed-off-by: Mark D. Roth <roth@google.com>

htuch

LGTM modulo v2 modificaiton discussion.

markdroth · 2020-04-29T13:39:02Z

@mattklein123 Any objections to making this change in v2 as well? gRPC is working on a design to migrate to v3, but we'll need this before we can make that happen.

mattklein123 · 2020-04-29T19:31:09Z

@mattklein123 Any objections to making this change in v2 as well? gRPC is working on a design to migrate to v3, but we'll need this before we can make that happen.

I'm OK with it.

markdroth · 2020-04-30T13:38:46Z

@lizan Can we get your approval on making this change in v2? Thanks!

Add send_all_clusters field to LRS response.

7b9dea1

Signed-off-by: Mark D. Roth <roth@google.com>

repokitteh-read-only Bot added the api label Apr 1, 2020

htuch self-assigned this Apr 1, 2020

htuch suggested changes Apr 1, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Apr 1, 2020

code review changes

3d0d18d

Signed-off-by: Mark D. Roth <roth@google.com>

repokitteh-read-only Bot removed the waiting label Apr 2, 2020

fix_format

5357600

Signed-off-by: Mark D. Roth <roth@google.com>

Work around doc bug.

c83f277

Signed-off-by: Mark D. Roth <roth@google.com>

markdroth added 3 commits April 9, 2020 08:58

Attempt to implement support for send_all_clusters.

835192b

Signed-off-by: Mark D. Roth <roth@google.com>

fix lambda capture

3b4fe1e

Signed-off-by: Mark D. Roth <roth@google.com>

Merge remote-tracking branch 'upstream/master' into lrs_send_all_clus…

b32c19c

…ters Signed-off-by: Mark D. Roth <roth@google.com>

stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 25, 2020

fix test

93fa970

Signed-off-by: Mark D. Roth <roth@google.com>

stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 27, 2020

fix-format

cb0f1a0

Signed-off-by: Mark D. Roth <roth@google.com>

htuch reviewed Apr 28, 2020

View reviewed changes

markdroth added 5 commits April 28, 2020 09:45

Merge remote-tracking branch 'upstream/master' into lrs_send_all_clus…

e7677e5

…ters Signed-off-by: Mark D. Roth <roth@google.com>

code review changes

845f773

Signed-off-by: Mark D. Roth <roth@google.com>

fix doc link

0aa5db8

Signed-off-by: Mark D. Roth <roth@google.com>

fix format

b4126cb

Signed-off-by: Mark D. Roth <roth@google.com>

fix test

fdabe58

Signed-off-by: Mark D. Roth <roth@google.com>

mattklein123 reviewed Apr 28, 2020

View reviewed changes

Comment thread api/envoy/service/load_stats/v2/lrs.proto Outdated

remove stale validation options

b229528

Signed-off-by: Mark D. Roth <roth@google.com>

htuch reviewed Apr 29, 2020

View reviewed changes

mattklein123 approved these changes Apr 29, 2020

View reviewed changes

repokitteh-read-only Bot removed the api label Apr 29, 2020

lizan approved these changes May 1, 2020

View reviewed changes

htuch merged commit b0f45ed into envoyproxy:master May 1, 2020

markdroth deleted the lrs_send_all_clusters branch May 1, 2020 21:20

Conversation

markdroth commented Apr 1, 2020

Uh oh!

repokitteh-read-only Bot commented Apr 1, 2020

Uh oh!

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

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 commented Apr 2, 2020

Uh oh!

htuch commented Apr 2, 2020

Uh oh!

htuch commented Apr 7, 2020

Uh oh!

stale Bot commented Apr 25, 2020

Uh oh!

markdroth commented Apr 27, 2020

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!

Uh oh!

Uh oh!

htuch commented Apr 28, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth commented Apr 29, 2020

Uh oh!

mattklein123 commented Apr 29, 2020

Uh oh!

markdroth commented Apr 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants