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

Enable load balancing policy extensions #17400

Merged
merged 10 commits into from
Aug 14, 2021
16 changes: 9 additions & 7 deletions api/envoy/config/cluster/v3/cluster.proto
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ message Cluster {
// this option or not.
CLUSTER_PROVIDED = 6;

// [#not-implemented-hide:] Use the new :ref:`load_balancing_policy
// Use the new :ref:`load_balancing_policy
// <envoy_v3_api_field_config.cluster.v3.Cluster.load_balancing_policy>` field to determine the LB policy.
// [#next-major-version: In the v3 API, we should consider deprecating the lb_policy field
// and instead using the new load_balancing_policy field as the one and only mechanism for
Expand Down Expand Up @@ -720,8 +720,7 @@ message Cluster {

// The :ref:`load balancer type <arch_overview_load_balancing_types>` to use
// when picking a host in the cluster.
// [#comment:TODO: Remove enum constraint :ref:`LOAD_BALANCING_POLICY_CONFIG<envoy_v3_api_enum_value_config.cluster.v3.Cluster.LbPolicy.LOAD_BALANCING_POLICY_CONFIG>` when implemented.]
LbPolicy lb_policy = 6 [(validate.rules).enum = {defined_only: true not_in: 7}];
LbPolicy lb_policy = 6 [(validate.rules).enum = {defined_only: true}];

// Setting this is required for specifying members of
// :ref:`STATIC<envoy_v3_api_enum_value_config.cluster.v3.Cluster.DiscoveryType.STATIC>`,
Expand Down Expand Up @@ -1008,7 +1007,7 @@ message Cluster {
// servers of this cluster.
repeated Filter filters = 40;

// [#not-implemented-hide:] New mechanism for LB policy configuration. Used only if the
// New mechanism for LB policy configuration. Used only if the
// :ref:`lb_policy<envoy_v3_api_field_config.cluster.v3.Cluster.lb_policy>` field has the value
// :ref:`LOAD_BALANCING_POLICY_CONFIG<envoy_v3_api_enum_value_config.cluster.v3.Cluster.LbPolicy.LOAD_BALANCING_POLICY_CONFIG>`.
LoadBalancingPolicy load_balancing_policy = 41;
Expand Down Expand Up @@ -1073,7 +1072,7 @@ message Cluster {
bool connection_pool_per_downstream_connection = 51;
}

// [#not-implemented-hide:] Extensible load balancing policy configuration.
// Extensible load balancing policy configuration.
pianiststickman marked this conversation as resolved.
Show resolved Hide resolved
//
// Every LB policy defined via this mechanism will be identified via a unique name using reverse
// DNS notation. If the policy needs configuration parameters, it must define a message for its
Expand Down Expand Up @@ -1104,9 +1103,12 @@ message LoadBalancingPolicy {
reserved "config";

// Required. The name of the LB policy.
string name = 1;
string name = 1 [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"];

google.protobuf.Any typed_config = 3;
google.protobuf.Any typed_config = 3
[deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"];
Copy link
Member

Choose a reason for hiding this comment

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

I thought we can delete them instead of deprecating since the whole message was not-implemented-hide before, and the implementation doesn't support this either.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be okay with fully removing these fields too, unless that causes problems. But I think there's very little cost to leaving them here and marking them deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. I tried to remove the whole Policy message from v3 only earlier and it failed the presubmit because it was expecting some v3 message to correspond to the v2 version of the same message. (That's why I ended up deprecating v2 fields in an earlier commit, which I've since undone.)

Removing these fields seems to be fine, though, so I've done that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. While there's little cost to leaving them here, but then the doc will be confusing as we never use those field in implementation.


core.v3.TypedExtensionConfig typed_extension_config = 4;
}

// Each client will iterate over the list in order and stop at the first policy that it
Expand Down
18 changes: 7 additions & 11 deletions api/envoy/config/cluster/v4alpha/cluster.proto

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

21 changes: 21 additions & 0 deletions envoy/upstream/load_balancer.h
Original file line number Diff line number Diff line change
Expand Up @@ -173,5 +173,26 @@ class ThreadAwareLoadBalancer {

using ThreadAwareLoadBalancerPtr = std::unique_ptr<ThreadAwareLoadBalancer>;

/**
* Factory for (thread-aware) load balancers. To support a load balancing policy of
* LOAD_BALANCING_POLICY_CONFIG, at least one load balancer factory corresponding to a policy in
* load_balancing_policy must be registered with Envoy. Envoy will use the first policy for which
* it has a registered factory.
*/
class TypedLoadBalancerFactory : public Config::UntypedFactory {
public:
~TypedLoadBalancerFactory() override = default;

/**
* @return ThreadAwareLoadBalancerPtr a new thread-aware load balancer.
*/
virtual ThreadAwareLoadBalancerPtr
create(const PrioritySet& priority_set, ClusterStats& stats, Stats::Scope& stats_scope,
Runtime::Loader& runtime, Random::RandomGenerator& random,
const ::envoy::config::cluster::v3::LoadBalancingPolicy_Policy& lb_policy) PURE;

std::string category() const override { return "envoy.load_balancers"; }
};

} // namespace Upstream
} // namespace Envoy
3 changes: 2 additions & 1 deletion envoy/upstream/load_balancer_type.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ enum class LoadBalancerType {
RingHash,
OriginalDst,
Maglev,
ClusterProvided
ClusterProvided,
LoadBalancingPolicyConfig
};

/**
Expand Down
15 changes: 15 additions & 0 deletions envoy/upstream/upstream.h
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,8 @@ using ProtocolOptionsConfigConstSharedPtr = std::shared_ptr<const ProtocolOption
*/
class ClusterTypedMetadataFactory : public Envoy::Config::TypedMetadataFactory {};

class TypedLoadBalancerFactory;

/**
* Information about a given upstream cluster.
*/
Expand Down Expand Up @@ -786,6 +788,19 @@ class ClusterInfo {
return std::dynamic_pointer_cast<const Derived>(extensionProtocolOptions(name));
}

/**
* @return const envoy::config::cluster::v3::LoadBalancingPolicy_Policy& the load balancing policy
* to use for this cluster.
*/
virtual const envoy::config::cluster::v3::LoadBalancingPolicy_Policy&
loadBalancingPolicy() const PURE;

/**
* @return the load balancer factory for this cluster if the load balancing type is
* LOAD_BALANCING_POLICY_CONFIG.
*/
virtual TypedLoadBalancerFactory* loadBalancerFactory() const PURE;

/**
* @return const envoy::config::cluster::v3::Cluster::CommonLbConfig& the common configuration for
* all load balancers for this cluster.
Expand Down
16 changes: 9 additions & 7 deletions generated_api_shadow/envoy/config/cluster/v3/cluster.proto

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

17 changes: 10 additions & 7 deletions generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto

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

8 changes: 8 additions & 0 deletions source/common/upstream/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,14 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "load_balancer_factory_base_lib",
hdrs = ["load_balancer_factory_base.h"],
deps = [
":load_balancer_lib",
],
)

envoy_cc_library(
name = "load_stats_reporter_lib",
srcs = ["load_stats_reporter.cc"],
Expand Down
8 changes: 8 additions & 0 deletions source/common/upstream/cluster_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -831,6 +831,13 @@ ClusterManagerImpl::loadCluster(const envoy::config::cluster::v3::Cluster& clust
}
} else if (cluster_reference.info()->lbType() == LoadBalancerType::ClusterProvided) {
cluster_entry_it->second->thread_aware_lb_ = std::move(new_cluster_pair.second);
} else if (cluster_reference.info()->lbType() == LoadBalancerType::LoadBalancingPolicyConfig) {
const auto& policy = cluster_reference.info()->loadBalancingPolicy();
TypedLoadBalancerFactory* typed_lb_factory = cluster_reference.info()->loadBalancerFactory();
RELEASE_ASSERT(typed_lb_factory != nullptr, "ClusterInfo should contain a valid factory");
cluster_entry_it->second->thread_aware_lb_ =
typed_lb_factory->create(cluster_reference.prioritySet(), cluster_reference.info()->stats(),
cluster_reference.info()->statsScope(), runtime_, random_, policy);
}

updateClusterCounts();
Expand Down Expand Up @@ -1377,6 +1384,7 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::ClusterEntry(
break;
}
case LoadBalancerType::ClusterProvided:
case LoadBalancerType::LoadBalancingPolicyConfig:
case LoadBalancerType::RingHash:
case LoadBalancerType::Maglev:
case LoadBalancerType::OriginalDst: {
Expand Down
29 changes: 29 additions & 0 deletions source/common/upstream/load_balancer_factory_base.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#pragma once

#include "envoy/upstream/load_balancer.h"

namespace Envoy {
namespace Upstream {

/**
* Base class for cluster provided load balancers and load balancers specified by load balancing
* policy config. This class should be extended directly if the load balancing policy specifies a
* thread-aware load balancer.
*
* TODO: provide a ThreadLocalLoadBalancer construct to abstract away thread-awareness from load
* balancing extensions that don't require it.
*/
class TypedLoadBalancerFactoryBase : public TypedLoadBalancerFactory {
public:
// Upstream::TypedLoadBalancerFactory
std::string name() const override { return name_; }

protected:
TypedLoadBalancerFactoryBase(const std::string& name) : name_(name) {}

private:
const std::string name_;
};

} // namespace Upstream
} // namespace Envoy
4 changes: 2 additions & 2 deletions source/common/upstream/subset_lb.cc
Original file line number Diff line number Diff line change
Expand Up @@ -781,8 +781,8 @@ SubsetLoadBalancer::PrioritySubsetImpl::PrioritySubsetImpl(const SubsetLoadBalan

case LoadBalancerType::OriginalDst:
case LoadBalancerType::ClusterProvided:
// LoadBalancerType::OriginalDst is blocked in the factory. LoadBalancerType::ClusterProvided
// is impossible because the subset LB returns a null load balancer from its factory.
case LoadBalancerType::LoadBalancingPolicyConfig:
// These load balancer types can only be created when there is no subset configuration.
NOT_REACHED_GCOVR_EXCL_LINE;
}

Expand Down
Loading