Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor cluster manager init sequence. #33221

Merged
merged 7 commits into from
Apr 11, 2024
Merged

Refactor cluster manager init sequence. #33221

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
69325cf
Refactor cluster manager init sequence.
yanjunxiang-google Mar 29, 2024
0555ec2
fixing format
yanjunxiang-google Apr 1, 2024
35c94ad
fixing CI issue
yanjunxiang-google Apr 2, 2024
50abdf3
addressing comments and fixing CI issue
yanjunxiang-google Apr 5, 2024
666a0fe
fix test
yanjunxiang-google Apr 5, 2024
106e9f6
Empty-Commit
yanjunxiang-google Apr 8, 2024
1fb625e
Merge branch 'main' of https://github.com/envoyproxy/envoy into refac…
yanjunxiang-google Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions envoy/upstream/cluster_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,18 @@ class ClusterManager {

virtual ~ClusterManager() = default;

// API to initializes the ClusterManagerImpl instance based on the given Bootstrap config.
//
// This method *must* be called prior to invoking any other methods on the class and *must* only
// be called once. This method should be called immediately after ClusterManagerImpl construction
// and from the same thread in which the ClusterManagerImpl was constructed.
//
// The initialization is separated from the constructor because lots of work, including ADS
// initialization, is done in this method. If the contents of this method are invoked during
// construction, a derived class cannot override any of the virtual methods and have them invoked
// instead, since the base class's methods are used when in a base class constructor.
virtual absl::Status init(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) PURE;
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved

/**
* Add or update a cluster via API. The semantics of this API are:
* 1) The hash of the config is used to determine if an already existing cluster has changed.
Expand Down Expand Up @@ -491,6 +503,8 @@ class ClusterManagerFactory {

/**
* Allocate a cluster manager from configuration proto.
* The cluster manager init() method needs to be called right after this method.
* Please check https://github.com/envoyproxy/envoy/issues/33218 for details.
*/
virtual ClusterManagerPtr
clusterManagerFromProto(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) PURE;
Expand Down
1 change: 0 additions & 1 deletion source/common/upstream/cluster_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2224,7 +2224,6 @@ ClusterManagerPtr ProdClusterManagerFactory::clusterManagerFromProto(
context_.accessLogManager(), context_.mainThreadDispatcher(), context_.admin(),
context_.messageValidationContext(), context_.api(), http_context_, context_.grpcContext(),
context_.routerContext(), server_)};
THROW_IF_NOT_OK(cluster_manager_impl->init(bootstrap));
return cluster_manager_impl;
}

Expand Down
12 changes: 1 addition & 11 deletions source/common/upstream/cluster_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,17 +246,7 @@ class ClusterManagerImpl : public ClusterManager,
public MissingClusterNotifier,
Logger::Loggable<Logger::Id::upstream> {
public:
// Initializes the ClusterManagerImpl instance based on the given Bootstrap config.
//
// This method *must* be called prior to invoking any other methods on the class and *must* only
// be called once. This method should be called immediately after ClusterManagerImpl construction
// and from the same thread in which the ClusterManagerImpl was constructed.
//
// The initialization is separated from the constructor because lots of work, including ADS
// initialization, is done in this method. If the contents of this method are invoked during
// construction, a derived class cannot override any of the virtual methods and have them invoked
// instead, since the base class's methods are used when in a base class constructor.
absl::Status init(const envoy::config::bootstrap::v3::Bootstrap& bootstrap);
absl::Status init(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) override;

std::size_t warmingClusterCount() const { return warming_clusters_.size(); }

Expand Down
3 changes: 3 additions & 0 deletions source/server/configuration_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,10 @@ absl::Status MainImpl::initialize(const envoy::config::bootstrap::v3::Bootstrap&
}

ENVOY_LOG(info, "loading {} cluster(s)", bootstrap.static_resources().clusters().size());

// clusterManagerFromProto() and init() have to be called consecutively.
cluster_manager_ = cluster_manager_factory.clusterManagerFromProto(bootstrap);
RETURN_IF_NOT_OK(cluster_manager_->init(bootstrap));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this completely resolve our problem. This ensure the server context could always return a valid reference to the cluster manager. But what will happen when the upstream network filter or upstream http filter try to access the actual content of the cluster manager when doing the initialization?

Copy link
Contributor Author

@yanjunxiang-google yanjunxiang-google Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments!

  1. When router filter is trying to call processFilters() for upstream filters:

    envoy/source/common/router/router.cc

    Line 108 in 63bc9b5

    context.serverFactoryContext().clusterManager(), *upstream_ctx_, prefix);

    it does access context.serverFactoryContext().clusterManager(). IIUC, this is in the much later stage, so the cluster manager instance is already initialized.

  2. In the ClusterInfoImpl c'tor, which is called during cluster_manager_->init()

    envoy/source/common/upstream/upstream_impl.cc

    Line 1376 in 63bc9b5

    factory_context.clusterManager(), upstream_context_, prefix);

envoy/source/common/upstream/cluster_manager_impl.cc

Line 905 in 63bc9b5

factory_.clusterFromProto(cluster, *this, outlier_event_logger_, added_via_api);

It's accessing the cluster_manager_, i.e, *this, before it is initialized. This means ClusterInfoImpl c'tor only needs an instance of cluster manager, and does not require it is initialized.

Unfortunately, for composite filter in upstream chain case, it needs to access the cluster_manager_ from from outside here:

envoy/source/extensions/filters/http/composite/action.cc

Line 43 in 63bc9b5

factory_context, server_factory_context.clusterManager(), false, "http", nullptr);

That's the reason we need to make sure the server_factory_context.clusterManager() is setup before it is initialized.

BTW, for composite filter in upstream case, it also only needs an instance of cluster manager, and does not require it is initialized.


const auto& listeners = bootstrap.static_resources().listeners();
ENVOY_LOG(info, "loading {} listener(s)", listeners.size());
Expand Down
1 change: 1 addition & 0 deletions test/mocks/upstream/cluster_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class MockClusterManager : public ClusterManager {
void initializeThreadLocalClusters(const std::vector<std::string>& cluster_names);

// Upstream::ClusterManager
MOCK_METHOD(absl::Status, init, (const envoy::config::bootstrap::v3::Bootstrap& bootstrap));
MOCK_METHOD(bool, addOrUpdateCluster,
(const envoy::config::cluster::v3::Cluster& cluster,
const std::string& version_info));
Expand Down
Loading