Cluster lazy loading by nt · Pull Request #2740 · envoyproxy/envoy

nt · 2018-03-06T21:17:02Z

Lazy loading

This PR addresses #2500 for cluster lazy loading. Key design points are:

Exposes a new LazyLoader type that is owned by cluster manager.
LazyLoader allows the user to request for a specific cluster config.
LazyLoader is only available is there is a dynamic CDS config.
It will own its own separate XDS stream to not interfere with ClusterManager.
Callbacks can be added to ClusterManager to be notified of clusters updates or removal.

Missing in this initial PR. guidance requested:

At the moment there is no mechanism to deactivate the feature and avoid the extra XDS stream per envoy.
Tests.
I do not know if this PR should include a filter in source/common/http/filter/. Should I include one or let users implement their own using custom filters. Example implementation: https://gist.github.com/nt/e97410fd29583a340c4bb8ab4c41d9b8

Risk Level: Medium (new feature)

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

nt · 2018-03-06T21:24:35Z

cc: @htuch

nt · 2018-03-07T00:50:28Z

cc @mattklein123 for an initial review

htuch

Super excited to see this. Some comments to kick off the review.

htuch · 2018-03-07T16:16:13Z

include/envoy/upstream/cluster_manager.h

+public:
+  virtual ~ClusterUpdateCallbacks() {}
+
+  virtual void onClusterAddOrUpdate(const ThreadLocalCluster& cluster) PURE;


Needs Doxygen comments (see rest of file for examples to mimic style).

htuch · 2018-03-07T16:16:37Z

include/envoy/upstream/cluster_manager.h

+  virtual void addClusterUpdateCallbacks(ClusterUpdateCallbacks& callbacks) PURE;
+  virtual void removeClusterUpdateCallbacks(ClusterUpdateCallbacks& callbacks) PURE;
+
+  virtual LazyLoader* lazyLoader() const PURE;


All of these need comments as above.

htuch · 2018-03-07T16:17:59Z

include/envoy/upstream/lazy_loader.h

+
+/**
+ * Lazy loading is only available when using CDS, bootstrap with static clusters will not support
+ * lazy loading.


Is this true if we have independent lazy CDS config source in the bootstrap as in envoyproxy/data-plane-api#524?

htuch · 2018-03-07T16:18:17Z

include/envoy/upstream/lazy_loader.h

+public:
+  virtual ~LazyLoader() {}
+
+  virtual void loadCluster(const std::string& cluster) PURE;


Doxygenate.

htuch · 2018-03-07T16:19:46Z

include/envoy/upstream/upstream.h

   */
  virtual bool addedViaApi() const PURE;

+  virtual bool addedLazily() const PURE;


More so than my comment on envoyproxy/data-plane-api#524, I think this is where we might want to think about how to reconcile incremental and lazy loading. As a strawman, I'd suggest there is no difference between an incremental cluster addition on a regular CDS channel and a lazy loaded cluster. WDYT? @mattklein123 as well.

I agree. Ideally if we could extend the xDS API with incremental updates and incremental subscriptions (or hint) we could handle lazy loading with a single stream and reconcile quite a bit of this logic.

Has there been any work / discussion on this? Should I start a data-plane-api proposal?

I started a strawman proposal here: envoyproxy/data-plane-api#527

I need to spend heads down time with this next week, but at a high level if we can handle lazy loading w/ incremental lets just do incremental. We will get much bigger bang for the buck here. I will focus mostly on helping work through the issues in envoyproxy/data-plane-api#527.

htuch · 2018-03-07T16:22:00Z

source/common/upstream/BUILD

-    hdrs = ["cluster_manager_impl.h"],
+    srcs = [
+        "cluster_manager_impl.cc",
+        "lazy_loader_impl.cc",


Prefer to have lazy_loader_impl.* in their own lazy_load_lib target to keep fine grained dependencies for testing.

htuch · 2018-03-07T16:24:23Z

source/common/upstream/lazy_loader_impl.h

+  std::unique_ptr<Config::Subscription<envoy::api::v2::Cluster>> subscription_;
+  Event::Dispatcher& primary_dispatcher_;
+  ClusterManagerImpl& cluster_manager_;
+


Nit: superfluous blank space.

htuch · 2018-03-07T16:25:49Z

source/common/upstream/lazy_loader_impl.h

+                 Event::Dispatcher& primary_dispatcher,
+                 Runtime::RandomGenerator& random,
+                 ClusterManagerImpl& cluster_manager,
+                 Stats::Store& statsconst);


Nit: s/statsconst/stats.

htuch · 2018-03-07T16:28:58Z

source/common/upstream/lazy_loader_impl.cc

+};
+
+void LazyLoaderImpl::loadCluster(const std::string& cluster) {
+  subscription_->updateResources({cluster});


This is a bit strange use of the API for the subscription. Semantically, updateResources is saying "we now have a new set of resources we care about from this point forward". You want to say "send me an update for only this resource and forget about it when we're done". Or.. do we?

What should the behavior be for a lazy loaded cluster when it is modified at the management server? Do we want to have it pushed asynchronously? What are the plans for removal of clusters when they go away on the management server?

This could be solved using a single stream for the cds impl and the lazy loader if we extended the xds api to do incremental. See my comment here #2740 (comment)

htuch · 2018-03-07T16:29:53Z

source/common/upstream/lazy_loader_impl.cc

+                                 cluster_manager_(cm) {
+  ASSERT(bootstrap.dynamic_resources().has_cds_config());
+  if (bootstrap.dynamic_resources().has_ads_config()) {
+    // Create an ADS stream for the lazy loader. We can not reuse the cluster manager ADS stream.


I'm not sure if this is what we want for ADS. This will become clearer once we resolve envoyproxy/data-plane-api#524.

htuch · 2018-03-16T17:33:33Z

I'm going to close this out while we work on design. @nt is upstreaming individual PRs that are extracted from this.

Signed-off-by: Kuat Yessenov <kuat@google.com>

Lazy loading v0

1fee8e5

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

nt mentioned this pull request Mar 6, 2018

Lazy loading (on demand) for Routes, Clusters and Endpoints #2500

Closed

nt mentioned this pull request Mar 7, 2018

Add a lazy loading configuration field to bootstrap.proto envoyproxy/data-plane-api#524

Closed

htuch assigned mattklein123 and htuch Mar 7, 2018

htuch suggested changes Mar 7, 2018

View reviewed changes

htuch mentioned this pull request Mar 11, 2018

Add jwt auth filter config proto envoyproxy/data-plane-api#530

Merged

htuch closed this Mar 16, 2018

nt deleted the lazy-load branch July 22, 2019 21:21

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

add owners to telemetry extensions (envoyproxy#2740)

07efeed

Signed-off-by: Kuat Yessenov <kuat@google.com>

Conversation

nt commented Mar 6, 2018

Uh oh!

nt commented Mar 6, 2018

Uh oh!

nt commented Mar 7, 2018

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!

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!

htuch commented Mar 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants