envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
25.12k stars 4.82k forks source link

InitManager to support dependency based initialization #3831

Open qiwzhang opened 6 years ago

qiwzhang commented 6 years ago

Title: InitManager to support dependency based initialization

Description: When refactoring ClusterManager to use InitManager, (please see PR https://github.com/envoyproxy/envoy/pull/3700 for the reasons why such refactoring is needed), it will be much easier if InitManager supports dependency based initialization.

Cluster Manager has EDS cluster which may depends on other static clusters. Now it is using fairly complicated primary and secondary list to initialize.
With Added SDS (PR https://github.com/envoyproxy/envoy/pull/3700), it become even more complicated. Even some static clusters may need SDS to download the client secrets and SDS may need another static cluster.

If InitManager supports dependency based initialization, this will be much easier. Each target can specify its dependencies, InitManager will initialize these targets based on the dependency.

Here is the detail design:

Target Interface: { void initialize() PURE; // existing call string name(); // the unique target name; for cluster, it can be cluster name. vector depend_on(); // the depended target names };

InitManager initialize() call will sort the targets based on the dependency and start to initialize them.

ClusterManager can use InitManager in following way: each cluster (as InitTarget) can find out if it is using any other clusters, if so, use "depends_on()" function to return the depended cluster names.

qiwzhang commented 6 years ago

@mattklein123 what do you think?

mattklein123 commented 6 years ago

@qiwzhang at a high level I think this makes sense, though, given how complicated this is, I'm wondering if we can simplify as much as possible for now. For instance, could we say for now that SDS is only supported for EDS clusters (and fail config)? If we do that, could the InitManager live inside ClusterImplBase and not ClusterMananger? I know I said to do what you are trying to do, but having thought about it I agree it's complicated to do completely correctly and I'm wondering if we can punt on some of it for now until it's really asked for / needed?

Or do you think we must have SDS work right now for non-EDS clusters?

qiwzhang commented 6 years ago

Hi @mattklein123 let us do this in phases.

Phase one: it is what this PR https://github.com/envoyproxy/envoy/pull/3700 has: add an init_manager specific for sds in ClusterBaseImpl to support SDS initialization. SDS feature will have following restrictions, but we are OK.

Phase two: complete refractory of cluster manager initialization based on dependency based InitManager. With this SDS feature can remove about restrictions. This can be done whenever we have time. We can leave this issue open.

mattklein123 commented 6 years ago

@qiwzhang yup SGTM. I will review the other PR again. Thank you!