tcp_proxy: add support for weighted clusters by venilnoronha · Pull Request #4430 · envoyproxy/envoy

venilnoronha · 2018-09-14T23:49:23Z

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>

ggreenway

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

ggreenway · 2018-09-15T00:06:31Z

api/envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.proto

+      // 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];


uint32, not g.p.UInt32Value

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 · 2018-09-15T01:14:11Z

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>

api/envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.proto

source/common/tcp_proxy/tcp_proxy.cc

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

ggreenway · 2018-09-19T22:51:24Z

api/envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.proto

+      // 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;


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.

Reverted in 0834714.

ggreenway · 2018-09-19T22:52:41Z

api/envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.proto

  message WeightedCluster {
    message ClusterWeight {
-      // Name of the upstream cluster.
+      // Name of the upstream cluster. The cluster must exist in the


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

Reverted in 0834714.

This reverts commit ed031a2. Signed-off-by: Venil Noronha <veniln@vmware.com>

api/envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.proto

ggreenway · 2018-09-20T20:06:25Z

source/common/tcp_proxy/tcp_proxy.cc

+  // 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;


0; (UL suffix isn't needed).

Addressed in 99352da.

ggreenway · 2018-09-20T20:11:14Z

source/common/tcp_proxy/tcp_proxy.cc

+  // 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_) {


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?

And that is a dupe of code in hcm

Addressed in 9dce4f5.

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

ggreenway

Thanks for refactoring that.

ggreenway · 2018-09-21T21:27:08Z

source/common/tcp_proxy/tcp_proxy.cc

             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));


nit: std::make_unique, instead of new

Fixed in af356c1.

ggreenway · 2018-09-21T21:28:53Z

source/common/tcp_proxy/tcp_proxy.h

    const std::string cluster_name_;
    const uint64_t cluster_weight_;
  };
+  typedef std::shared_ptr<WeightedClusterEntry> WeightedClusterEntrySharedPtr;


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

Fixed in af356c1.

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

ggreenway

LGTM. Thanks!

venilnoronha · 2018-09-21T21:45:34Z

@ggreenway @rshriram thanks for reviewing!

tcp_proxy: add support for weighted clusters

e6aff9f

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

ggreenway requested changes Sep 15, 2018

View reviewed changes

ggreenway self-assigned this Sep 15, 2018

ggreenway reviewed Sep 15, 2018

View reviewed changes

Use uint32 instead of google.protobuf.UInt32Value

9c55555

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

Fix proto errors

9dadee9

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

venilnoronha force-pushed the tcp-weighted-clusters branch from 506dd06 to 9dadee9 Compare September 15, 2018 01:14

rshriram reviewed Sep 17, 2018

View reviewed changes

api/envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.proto Show resolved Hide resolved

rshriram reviewed Sep 17, 2018

View reviewed changes

source/common/tcp_proxy/tcp_proxy.cc Show resolved Hide resolved

rshriram mentioned this pull request Sep 17, 2018

TCP proxy API: Weighted clusters and cluster_from_sni #4444

Closed

venilnoronha mentioned this pull request Sep 17, 2018

tcp_proxy: remove support for v1 routes #4446

Closed

Add metadata_match and per_filter_config fields to proto

ed031a2

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

venilnoronha force-pushed the tcp-weighted-clusters branch from 5a1196a to ed031a2 Compare September 19, 2018 00:36

venilnoronha changed the title ~~[WIP] tcp_proxy: add support for weighted clusters~~ tcp_proxy: add support for weighted clusters Sep 19, 2018

ggreenway requested changes Sep 19, 2018

View reviewed changes

Revert "Add metadata_match and per_filter_config fields to proto"

82e4765

This reverts commit ed031a2. Signed-off-by: Venil Noronha <veniln@vmware.com>

venilnoronha force-pushed the tcp-weighted-clusters branch 3 times, most recently from d6c0c5c to 9dadee9 Compare September 20, 2018 01:05

ggreenway requested changes Sep 20, 2018

View reviewed changes

venilnoronha added 2 commits September 20, 2018 13:21

Document field behavior in proto and drop UL suffix

99352da

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

Extract WeightedClusterUtil for a common operation

9dce4f5

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

venilnoronha requested a review from zuercher as a code owner September 21, 2018 21:11

ggreenway requested changes Sep 21, 2018

View reviewed changes

Fix type initialization and pointer declaration

af356c1

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

ggreenway approved these changes Sep 21, 2018

View reviewed changes

ggreenway merged commit 10625c5 into envoyproxy:master Sep 24, 2018

venilnoronha mentioned this pull request Oct 5, 2018

tcp_proxy: remove support for v1 routes #4625

Closed

Conversation

venilnoronha commented Sep 14, 2018

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

venilnoronha commented Sep 15, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

venilnoronha Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

venilnoronha Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

venilnoronha commented Sep 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

venilnoronha Sep 19, 2018 •

edited

Loading

venilnoronha Sep 19, 2018 •

edited

Loading