LEDS: Adding LEDS protos and initial implementation [WIP] #16487

adisuissa · 2021-05-13T18:02:41Z

Commit Message: LEDS: Adding LEDS protos and initial implementation
Additional Description:
Adding the API protos, and plugging into EDS configurations.
Added an integration test to test valid workflow.
Need to add unit tests and more integration tests.
Currently only works in delta-xDS mode, and using ADS.

Risk Level: Medium - but only if being configured.
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Adi Suissa-Peleg adip@google.com

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only · 2021-05-13T18:02:49Z

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

🐱

Caused by: #16487 was opened by adisuissa.

see: more, trace.

adisuissa · 2021-05-13T18:09:40Z

/cc @htuch
The PR can be broken into API and some plumbing PRs if needed.

markdroth · 2021-05-13T18:11:15Z

api/envoy/config/endpoint/v3/endpoint_components.proto

+  core.v3.ConfigSource leds_config = 1;
+
+  // The xDS transport protocol list or glob collection resource name.
+  // SotW mode supports only list collections, and delta xDS supports both.


I thought we said that initially, we were not going to support SotW.

markdroth · 2021-05-13T18:15:36Z

api/envoy/service/endpoint/v3/leds.proto

+// Locality-Endpoint discovery :ref:`architecture overview
+// <arch_overview_service_discovery_types_leds>`
+
+service LocalityEndpointDiscoveryService {


Out of curiosity, is there any case where we actually need a separate LEDS service, or will all known use-cases of LEDS actually use ADS?

My experience has been that endpoint data is different enough from regular config to merit special data plane treatment. The reasons include that endpoint data often changes faster that the rest of the config and loading endpoint data presents different risks than those seen while loading standard config. The most common failure modes when loading endpoint config is not validation errors but potential for excessive memory/cpu usage due to update size and frequency.

Endpoint distribution may also be one of the first possible targets for federation or sharding of control plane components in order to handle scaling better.

Yeah, I think as part of the general push towards supporting disaggregation, a separate service makes sense.

antoniovicente

It's great to see progress on LEDS.

antoniovicente · 2021-05-15T02:19:01Z

api/envoy/service/endpoint/v3/leds.proto

+// Locality-Endpoint discovery :ref:`architecture overview
+// <arch_overview_service_discovery_types_leds>`
+
+service LocalityEndpointDiscoveryService {


My experience has been that endpoint data is different enough from regular config to merit special data plane treatment. The reasons include that endpoint data often changes faster that the rest of the config and loading endpoint data presents different risks than those seen while loading standard config. The most common failure modes when loading endpoint config is not validation errors but potential for excessive memory/cpu usage due to update size and frequency.

Endpoint distribution may also be one of the first possible targets for federation or sharding of control plane components in order to handle scaling better.

antoniovicente · 2021-05-15T02:25:32Z

source/common/upstream/BUILD

+envoy_cc_library(
+    name = "leds_lib",
+    srcs = ["leds.cc"],
+    hdrs = ["leds.h"],


Seems a bit odd that the dependencies of this new leds library match the dependencies of what used to be the eds library. Is there potential for trimming down this dependency list?

htuch

Overall looks great, I think this is a nice architecture and this will support LEDS requirements.

Can you start splitting into smaller PR for review? I think this is pretty huge and some parts can be done individually, e.g. the LEDS subscription.
/wait

htuch · 2021-05-16T18:57:30Z

api/envoy/service/endpoint/v3/leds.proto

+// Locality-Endpoint discovery :ref:`architecture overview
+// <arch_overview_service_discovery_types_leds>`
+
+service LocalityEndpointDiscoveryService {


Yeah, I think as part of the general push towards supporting disaggregation, a separate service makes sense.

htuch · 2021-05-16T18:58:06Z

api/envoy/service/endpoint/v3/leds.proto

+
+  rpc FetchLocalityEndpoints(discovery.v3.DiscoveryRequest)
+      returns (discovery.v3.DiscoveryResponse) {
+    option (google.api.http).post = "/v3/discovery:locality_endpoints";


We might be able to get away with not supporting REST and SotW here, whatever simplifies and is efficient.

htuch · 2021-05-16T19:12:22Z

source/common/upstream/eds.cc

+            // TODO(adisuissa): here we know the exact locality that is now active, so we can
+            // optimize this process by moving the locality from warming set ot active set, and call
+            // the update if the warming set is empty.
+            if (validateAllLedsActive()) {


Does the design here support transitive warming flows?

E.g. when a new cluster is added, it will warm on its EDS update. We need to delay the warming of the EDS update until all the dependent LEDS updates have occurred.

htuch · 2021-05-16T19:13:41Z

source/common/upstream/eds.cc

+      // Create a new LEDS subscription and add it to the subscriptions map.
+      LedsSubscriptionPtr leds_locality_subscription = std::make_unique<LedsSubscription>(
+          leds_config, cluster_name_, factory_context_, /*nullptr,*/ [&]() {
+            // Called upon an update to the locality


Is there a timeout flow supported, where after some configurable period, e.g. 15s default, we can switch all LEDS to active and unblock initialization?

htuch · 2021-05-16T19:16:36Z

test/integration/ads_integration_test.cc

@@ -1666,6 +1666,75 @@ TEST_P(XdsTpAdsIntegrationTest, Basic) {
  makeSingleRequest();
 }

+// TODO(adisuissa): Add a test with static cluster and load assignment, but dynamic endpoints.


Yeah, we'll also need a bunch of unit tests exploring the corner case behaviors around warming and sharing of LEDS subscriptions across EDS subs.

htuch · 2021-05-16T19:20:44Z

source/common/upstream/eds.cc

+
+  // Add all the LEDS localities that are new.
+  for (const auto& leds_config : cla_leds_configs) {
+    if (leds_localities_.find(leds_config) == leds_localities_.end()) {


Did you consider making LEDS localities a cluster manager property to share across EDS subscriptions? I think it's probably overkill given that generally clusters won't share.

jmarantz · 2021-05-18T14:21:22Z

@jmarantz subscribing

github-actions · 2021-06-17T16:03:32Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 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!

github-actions · 2021-06-24T20:01:22Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

LEDS: Adding LEDS protos and initial implementation [WIP]

f20ce8a

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot added the api label May 13, 2021

repokitteh-read-only bot assigned htuch May 13, 2021

markdroth reviewed May 13, 2021

View reviewed changes

antoniovicente reviewed May 15, 2021

View reviewed changes

htuch requested changes May 16, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label May 16, 2021

htuch reviewed May 16, 2021

View reviewed changes

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 17, 2021

github-actions bot closed this Jun 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LEDS: Adding LEDS protos and initial implementation [WIP] #16487

LEDS: Adding LEDS protos and initial implementation [WIP] #16487

adisuissa commented May 13, 2021

repokitteh-read-only bot commented May 13, 2021

adisuissa commented May 13, 2021

markdroth May 13, 2021

markdroth May 13, 2021

antoniovicente May 15, 2021

htuch May 16, 2021

antoniovicente left a comment

antoniovicente May 15, 2021

antoniovicente May 15, 2021

htuch left a comment

htuch May 16, 2021

htuch May 16, 2021

htuch May 16, 2021

htuch May 16, 2021

htuch May 16, 2021

htuch May 16, 2021

jmarantz commented May 18, 2021

github-actions bot commented Jun 17, 2021

github-actions bot commented Jun 24, 2021

LEDS: Adding LEDS protos and initial implementation [WIP] #16487

LEDS: Adding LEDS protos and initial implementation [WIP] #16487

Conversation

adisuissa commented May 13, 2021

repokitteh-read-only bot commented May 13, 2021

adisuissa commented May 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniovicente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented May 18, 2021

github-actions bot commented Jun 17, 2021

github-actions bot commented Jun 24, 2021