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

tcp_proxy: add support for weighted clusters #4430

Merged
merged 8 commits into from Sep 24, 2018

Conversation

venilnoronha
Copy link
Member

Description:
This PR adds support for optional weighted clusters in TCP Proxy.

Signed-off-by: Venil Noronha veniln@vmware.com

Risk Level: Med
Testing: Added 2 new tests
Docs Changes: N/A
Release Notes: Introduced weighted clusters in version_history.rst

/cc @rshriram

Signed-off-by: Venil Noronha <veniln@vmware.com>
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

I skimmed this and it looks good. I'll take a closer look when it's no longer [WIP]

@ggreenway ggreenway self-assigned this Sep 15, 2018
// When a request matches the route, the choice of an upstream cluster is
// determined by its weight. The sum of weights across all entries in the
// clusters array determines the total weight.
google.protobuf.UInt32Value weight = 2 [(validate.rules).uint32.gte = 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

uint32, not g.p.UInt32Value

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 9c55555.

P.S. I'm amazed at the number of fives in the hash. :)

Signed-off-by: Venil Noronha <veniln@vmware.com>
@venilnoronha
Copy link
Member Author

I marked it WIP as I was thinking of moving WeightedCluster (which is common to thrift_proxy) to a common location. Do you think we should do that?

Signed-off-by: Venil Noronha <veniln@vmware.com>
Signed-off-by: Venil Noronha <veniln@vmware.com>
@venilnoronha venilnoronha changed the title [WIP] tcp_proxy: add support for weighted clusters tcp_proxy: add support for weighted clusters Sep 19, 2018
// criteria. Only endpoints in the upstream cluster with metadata
// matching that set in metadata_match will be considered. The filter
// name should be specified as *envoy.lb*.
envoy.api.v2.core.Metadata metadata_match = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the rationale for adding these two new fields in this PR? They're not implemented, so unless there's a good reason, let's wait to add them until they're implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted in 0834714.

@@ -146,13 +147,28 @@ message TcpProxy {
// The router selects an upstream cluster based on these weights.
message WeightedCluster {
message ClusterWeight {
// Name of the upstream cluster.
// Name of the upstream cluster. The cluster must exist in the
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this change is correct. The tcp_proxy handles the case of a missing cluster.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted in 0834714.

This reverts commit ed031a2.

Signed-off-by: Venil Noronha <veniln@vmware.com>
@venilnoronha venilnoronha force-pushed the tcp-weighted-clusters branch 3 times, most recently from d6c0c5c to 9dadee9 Compare September 20, 2018 01:05
// Weighted clusters will be enabled only if both the default cluster and
// deprecated v1 routes are absent.
if (routes_.empty() && config.has_weighted_clusters()) {
total_cluster_weight_ = 0UL;
Copy link
Contributor

Choose a reason for hiding this comment

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

0; (UL suffix isn't needed).

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 99352da.

// Find the right cluster to route to based on the interval in which
// the selected value falls. The intervals are determined as
// [0, cluster1_weight), [cluster1_weight, cluster1_weight+cluster2_weight),..
for (const WeightedClusterEntry& cluster : weighted_clusters_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is duplicate of the same thing in thrift_proxy. Is there any way to share the code for this algorithm? Maybe a templatized function just for the algorithm, so you don't need to make the entries be a common type?

Copy link
Member

Choose a reason for hiding this comment

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

And that is a dupe of code in hcm

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 9dce4f5.

Signed-off-by: Venil Noronha <veniln@vmware.com>
Signed-off-by: Venil Noronha <veniln@vmware.com>
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Thanks for refactoring that.

@@ -83,8 +84,9 @@ Config::Config(const envoy::config::filter::network::tcp_proxy::v2::TcpProxy& co
total_cluster_weight_ = 0;
for (const envoy::config::filter::network::tcp_proxy::v2::TcpProxy::WeightedCluster::
ClusterWeight& cluster_desc : config.weighted_clusters().clusters()) {
weighted_clusters_.emplace_back(WeightedClusterEntry(cluster_desc));
total_cluster_weight_ += weighted_clusters_.back().cluster_weight_;
std::unique_ptr<WeightedClusterEntry> cluster_entry(new WeightedClusterEntry(cluster_desc));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: std::make_unique, instead of new

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in af356c1.

const std::string cluster_name_;
const uint64_t cluster_weight_;
};
typedef std::shared_ptr<WeightedClusterEntry> WeightedClusterEntrySharedPtr;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be a unique_ptr instead of shared_ptr. I don't think these are ever shared.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in af356c1.

Signed-off-by: Venil Noronha <veniln@vmware.com>
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@venilnoronha
Copy link
Member Author

@ggreenway @rshriram thanks for reviewing!

@ggreenway ggreenway merged commit 10625c5 into envoyproxy:master Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants