This looks like what we'd need for our use case. Since some details are missing, I presume the final implementation would have a failure pathway of sorts, but otherwise it appears that its goal is to provide a somewhat simple interface for establishing a callback for when the cluster resolves.

At a very high level this looks right to me, however, I think like VHDS the actual lazy loading should be in its own HTTP or network filter that runs before the other filters. I think this would be a cleaner (and less risky design). My advice would be to first build the future API into CM, and then we can make the filters that use it?

+1 on separating the lazy load to a filter. By default, CM should not have to make decisions and we should have non-lazy EDS.

@bryce-anderson Thank you for the feedback! The goal of this PR is to explain the necessary changes to the data path including CM beyond http request. I absolutely agree tthat the eventual on-demand CDS/EDS needs a fallback strategy.

@htuch @mattklein123
There are 3 arguments

1. Should CM provide a ClusterManager::futureThreadLocalCluster() API?
   My answer is Yes. Also I want to add that this API is independent of whether the cluster is lazy-loadable. See also question 3. futureThreadLocalCluster() along with FutureCluster could work when the cluster is not lazy-loadable or the cluster is lazy-loaded already. The FutureCluster::isReady() returns the state of whether the cluster is available and save the effort to await.

if (future->isReady()) {
  onClusterReady(*future);
} else {
  future_cluster_handle_ =
      future->await(read_callbacks_->connection().dispatcher(),
                    [this](Upstream::FutureCluster& f) { onClusterReady(f); });
}

2. What component should initiate the lazy load attempt if the cluster is lazy-loadable?
   In Http conn manager we could use a preload http filter as in the VHDS flow because VHDS filter know the cluster name prior to create the connection in the http router filter.
   In this PR which address the tcp_proxy. TcpProxy is the trigger because only TcpProxy filter knows the target cluster name. See TcpProxy::pickRoute Not all the network filters have the luxury to fire the lazy load attempt prior to request the upstream connection.
   futureThreadLocalCluster() provides the minimum API to enable a lazyload filter.

3. Is non lazy-loadable EDS cluster allowed? Which API should decide if a known cluster is lazy-loadable?
   This PR doesn't address this issue but I have two cents.
   Let's exclude some answers. Filter should not decide whether a cluster is lazy-loadable. The filter could use FutureCluster::isReady() and not call FutureCluster::await() to avoid blindly wait.
   I prefer to add a field in message Cluster to indicate whether CdsApi should sending EDS request put the cluster in a warming state.
   For the on-demand CDS, because there is no cluster field before lazy-load the cluster (chicken-egg problem). We probably need to provide a matcher for CM to decide whether we should add a cluster name to watcher in on-demand CDS stream. Luckily lazy-loadable EDS cluster doesn't have this problem because we assume CDS must provide the cluster info that include is lazy-loadable.

@lambdai I think one thing that we would want to make sure is possible is receiving a CDS update, storing the cluster config locally in proto but not instantiating a full Cluster (with stats) and then only on-demand doing the instantiation. This would potentially save a ton of memory and CPU on updates for rarely uses clusters. We might even fetch the EDS resources, but leave them uninstantiated.

re: (1), yes, agreed, let's add this functionality and test it as an independent PR. It's not very complicated as all the parts are already there. I would note that we don't explicitly need this to be part of the cluster manager API, since with the callback mechanisms in place I think this could be implemented on top. I don't feel strongly about it but you might consider a wrapper class.

re: (2) for HTTP I agree this should be a filter. For TCP, I'm not crazy about making the tcp_proxy filter even more complicated, so it might be worth it to see if we can refactor somehow to allow TCP route selection to be done by a previous filter? I think maybe this is possible? It's worth investigating.

re: (3) I agree with what @htuch said, but this part is complicated enough that I would recommend doing an independent design doc on this portion to make sure we are all on the same page.

Thank you for the feedback! I am drafting and I will post in the linked issue

Also design doc here https://docs.google.com/document/d/1-QgiqOCq8EcQO1cO22dRFKpuUglQp_pKZVXFCPmMSJQ/edit?usp=sharing

I think we can move lazy local expansion to a separate issue, but the API (and ideally implementation) should be forward thinking with this use case in mind, as it's something likely to come up in MT scenarios where on-demand doesn't work for reliability reasons.

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!

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!