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.

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

on-demand, upstream: on-demand cluster discovery #15523

Closed
wants to merge 13 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions api/envoy/extensions/filters/http/on_demand/v3/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ licenses(["notice"]) # Apache 2

api_proto_package(
deps = [
"//envoy/config/core/v3:pkg",
"//envoy/config/filter/http/on_demand/v2:pkg",
"@com_github_cncf_udpa//udpa/annotations:pkg",
],
Expand Down
26 changes: 24 additions & 2 deletions api/envoy/extensions/filters/http/on_demand/v3/on_demand.proto
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ syntax = "proto3";

package envoy.extensions.filters.http.on_demand.v3;

import "envoy/config/core/v3/config_source.proto";

import "udpa/annotations/status.proto";
import "udpa/annotations/versioning.proto";

Expand All @@ -10,11 +12,31 @@ option java_outer_classname = "OnDemandProto";
option java_multiple_files = true;
option (udpa.annotations.file_status).package_version_status = ACTIVE;

// [#protodoc-title: OnDemand]
// IP tagging :ref:`configuration overview <config_http_filters_on_demand>`.
// [#protodoc-title: On Demand Discovery]
// On Demand Discovery :ref:`configuration overview <config_http_filters_on_demand>`.
// [#extension: envoy.filters.http.on_demand]

// On Demand Discovery filter config.
message OnDemand {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.http.on_demand.v2.OnDemand";

// An optional configuration for on-demand cluster discovery
// service. If not specified, the on-demand cluster discovery will
// be disabled. When it's specified, the filter will pause a request
// to an unknown cluster and will begin a cluster discovery
// process. When the discovery is finished (successfully or not),
// the request will be resumed for further processing.
config.core.v3.ConfigSource odcds_config = 1;
mattklein123 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

If the only knob here is the presence of a ConfigSource, then how will we configure ODCDS when we're using new-style resource names and don't need a ConfigSource because we're using authority-based routing?

Copy link
Member

Choose a reason for hiding this comment

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

I think the presence of an OCDS PerRouteConfig would indicate we needed to treat the name as on-demand. The meaning of self becomes interesting as it might mean two different things for the filter-level config (received from LDS) and the PerRouteConfig (from RDS).

}

// Per-route configuration for On Demand Discovery.
message PerRouteConfig {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want an explicit disable flag in here? E.g. if it's on globally or at the vhost level but we don't want it at the route level? I think we can add that later but just wanted to point it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To disable ODCDS at the route level with current state of things, you could specify the empty config in route's "typed_per_filter_config":

typed_per_filter_config:
  envoy.filters.http.on_demand:
    "@type": type.googleapis.com/envoy.extensions.filters.http.on_demand.v3.PerRouteConfig

I'll add a test to make sure that disabling works that way.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need both the ability to disable this on a per-route basis and the ability to handle xdstp names for which no ConfigSource needs to be specified. In https://github.com/envoyproxy/envoy/pull/15523/files/b742a88722c205b9fa1d0b694ff010c3d994e7b2#r596505958, @htuch proposed that we use an empty PerRouteConfig message for xdstp names; here, you are proposing that we use the same thing to indicate disabling the feature. I don't think this can mean both things.

Copy link
Member

Choose a reason for hiding this comment

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

Technically we could have an empty message with an empty ConfigSource, but I think this gets confusing. Probably an explicit disable flag would be the way to go.

// An optional configuration for on-demand cluster discovery
// service. If not specified, the on-demand cluster discovery will
// be disabled. When it's specified, the filter will pause a request
// to an unknown cluster and will begin a cluster discovery
// process. When the discovery is finished (successfully or not),
// the request will be resumed for further processing.
config.core.v3.ConfigSource odcds_config = 1;
}
13 changes: 13 additions & 0 deletions api/envoy/extensions/filters/http/on_demand/v4alpha/BUILD

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

71 changes: 70 additions & 1 deletion include/envoy/upstream/cluster_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ namespace Envoy {
namespace Upstream {

/**
* ClusterUpdateCallbacks provide a way to exposes Cluster lifecycle events in the
* ClusterUpdateCallbacks provide a way to expose Cluster lifecycle events in the
* ClusterManager.
*/
class ClusterUpdateCallbacks {
Expand Down Expand Up @@ -72,6 +72,39 @@ class ClusterUpdateCallbacksHandle {

using ClusterUpdateCallbacksHandlePtr = std::unique_ptr<ClusterUpdateCallbacksHandle>;

/**
* Status enum for the result of an attempted cluster discovery.
*/
enum class ClusterDiscoveryStatus {
/**
* Cluster was not found during the discovery process.
*/
Missing,
/**
* Cluster found and currently available through ClusterManager.
*/
Available,
};

/**
* ClusterDiscoveryCallback is a callback called at the end of the on-demand cluster discovery
* process. The status of the discovery is sent as a parameter.
*/
using ClusterDiscoveryCallback = std::function<void(ClusterDiscoveryStatus)>;
using ClusterDiscoveryCallbackWeakPtr = std::weak_ptr<ClusterDiscoveryCallback>;
using ClusterDiscoveryCallbackSharedPtr = std::shared_ptr<ClusterDiscoveryCallback>;

/**
* ClusterDiscoveryCallbackHandle is a RAII wrapper for a ClusterDiscoveryCallback. Deleting the
* ClusterDiscoveryCallbackHandle will remove the callbacks from ClusterManager.
*/
class ClusterDiscoveryCallbackHandle {
public:
virtual ~ClusterDiscoveryCallbackHandle() = default;
};

using ClusterDiscoveryCallbackHandlePtr = std::unique_ptr<ClusterDiscoveryCallbackHandle>;

class ClusterManagerFactory;

// These are per-cluster per-thread, so not "global" stats.
Expand Down Expand Up @@ -119,6 +152,9 @@ struct ClusterConnectivityState {
int64_t connecting_stream_capacity_{};
};

class OdCdsApi;
using OdCdsApiSharedPtr = std::shared_ptr<OdCdsApi>;

/**
* Manages connection pools and load balancing for upstream clusters. The cluster manager is
* persistent and shared among multiple ongoing requests/connections.
Expand Down Expand Up @@ -309,6 +345,24 @@ class ClusterManager {
virtual const ClusterRequestResponseSizeStatNames&
clusterRequestResponseSizeStatNames() const PURE;
virtual const ClusterTimeoutBudgetStatNames& clusterTimeoutBudgetStatNames() const PURE;

/**
* Request an on-demand discovery of a cluster with a passed name. Passed ODCDS may be used to
* perform the discovery process in the main thread if there is no discovery going on for this
* cluster. The passed callback will be invoked when the cluster is added and warmed up. It is
* expected that the callback will be destroyed when it is invoked. To cancel the discovery,
* destroy the returned handle and the callback.
*
* This function is thread-safe.
*
* @param odcds is a pointer to ODCDS used for discovery. Must not be a nullptr.
* @param name is the name of the cluster to be discovered.
* @param callback will be called when the discovery is finished.
* @return ClusterDiscoveryCallbackHandlePtr the discovery process handle.
*/
virtual ClusterDiscoveryCallbackHandlePtr
requestOnDemandClusterDiscovery(OdCdsApiSharedPtr odcds, const std::string& name,
ClusterDiscoveryCallbackWeakPtr callback) PURE;
};

using ClusterManagerPtr = std::unique_ptr<ClusterManager>;
Expand Down Expand Up @@ -339,6 +393,21 @@ class CdsApi {

using CdsApiPtr = std::unique_ptr<CdsApi>;

/**
* Abstract interface for a On-Demand CDS API provider.
*/
class OdCdsApi {
public:
virtual ~OdCdsApi() = default;

/**
* File an on-demand request for a cluster.
*/
virtual void updateOnDemand(const std::string& cluster_name) PURE;
};

using OdCdsApiPtr = std::unique_ptr<OdCdsApi>;
Comment on lines +396 to +409
Copy link
Member

Choose a reason for hiding this comment

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

Somewhat related to @htuch comments, but why is this public interface needed? Conceptually, something asks the cluster manager to fetch a cluster, and gets a callback when complete or failed. I don't think any of this should be exposed "externally" and should be an internal implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the config source for the on-demand discovery is provided in the per-route config, which makes it a separate object than CM has from cds_config, I need to pass a "CDS" object to CM. It needs to be a public interface, because it's being used in ClusterManager interface. I had two options here: either extend the CdsApi interface with the updateOnDemand function and pass that to CM or create a new interface (OdCdsApi). I went with the latter, because I found the former to be awkward. It's because CdsApi provides initialize, setInitializedCb and versionInfo and for the on-demand discovery I would use none of it.


/**
* Factory for objects needed during cluster manager operation.
*/
Expand Down
20 changes: 20 additions & 0 deletions source/common/upstream/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,25 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "odcds_api_lib",
srcs = ["odcds_api_impl.cc"],
hdrs = ["odcds_api_impl.h"],
deps = [
":cds_api_helper_lib",
"//include/envoy/config:subscription_interface",
"//include/envoy/protobuf:message_validator_interface",
"//include/envoy/stats:stats_interface",
"//include/envoy/upstream:cluster_manager_interface",
"//source/common/common:minimal_logger_lib",
"//source/common/config:subscription_base_interface",
"//source/common/grpc:common_lib",
"//source/common/protobuf",
"@envoy_api//envoy/config/cluster/v3:pkg_cc_proto",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
],
)

envoy_cc_library(
name = "cluster_manager_lib",
srcs = ["cluster_manager_impl.cc"],
Expand All @@ -52,6 +71,7 @@ envoy_cc_library(
":cds_api_lib",
":load_balancer_lib",
":load_stats_reporter_lib",
":odcds_api_lib",
":ring_hash_lb_lib",
":subset_lb_lib",
"//include/envoy/api:api_interface",
Expand Down
2 changes: 1 addition & 1 deletion source/common/upstream/cds_api_helper.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class CdsApiHelper : Logger::Loggable<Logger::Id::upstream> {

private:
ClusterManager& cm_;
std::string name_;
const std::string name_;
std::string system_version_info_;
};

Expand Down
Loading