Support for multiple weighted clusters in routing by rshriram · Pull Request #361 · envoyproxy/envoy

rshriram · 2017-01-20T05:32:55Z

[apologies for the delayed turn around for this feature].

This PR provides an intuitive way of specifying cluster weights, when a particular route is backed by two or more clusters. Instead of using the existing Runtime style probability specification, users can group related clusters under a single weighted_clusters block with % weights assigned to each cluster.

Please refer to #247 for more details.

The current PR is missing documentation. Will add that in a day or so. TLDR:
In the routing config, you can have either the cluster key or the weighted_clusters key.
The weighted_clusters is of the following format:

"weighted_clusters" : {
"runtime_key_prefix" : "foo",
"clusters" : [{ "name" : "cluster-name", "weight" : N },
...
]}

where N is an integer from 0 to 100. It determines the proportion of request traffic to be sent to the target cluster. The sum of all weights in the weighted cluster should be equal to 100.

On runtime support for weighted clusters: If we wish to validate the weights provided by the user (as we do it currently), it is not exactly clear how the weights would be validated (i.e. sum of weights == 100) if runtime support were to be enabled for a weighted cluster. My current inclination is to skip runtime support altogether for weighted_clusters. With the addition of RDS, a user can combine weighted cluster and RDS to dynamically change weights. It may not occur instantaneously (as it depends on the RDS poll interval).

Fixes #247

@mattklein123 @ccaraman

p.s.: For reviewing this PR, please do not look at the individual commits. Instead, I suggest looking at config_impl.h|cc as one change set and then config_impl_test.cc as another change set.

mattklein123

re: runtime, I think it wouldn't be too hard to add (we could just do it inline and abort runtime processing if the values are wrong, but I think it's OK to ignore this for now. We can do it in a follow up if we need it.

mattklein123 · 2017-01-20T16:47:55Z

source/common/router/config_impl.cc

There is no point in having this method (the base matches method) return a Route*. Just have it return bool like it used to (and change method name to something else so it's not an override).

mattklein123 · 2017-01-20T16:50:05Z

source/common/router/config_impl.cc

please, fix: static const uint64_t max_weight

mattklein123 · 2017-01-20T16:52:51Z

source/common/router/config_impl.cc

This should be unique_ptr. I just looked and I'm not sure why RouteEntryImplBasePtr is a shared_ptr currently. Switch that to unique_ptr also.

mattklein123 · 2017-01-20T16:53:50Z

source/common/router/config_impl.cc

please fix per above.

mattklein123 · 2017-01-20T16:54:12Z

source/common/router/config_impl.cc

move comment into header file or into function.

mattklein123 · 2017-01-20T16:59:43Z

source/common/router/config_impl.h

unique_ptr, newline before this line

mattklein123 · 2017-01-20T17:02:20Z

source/common/router/config_impl.h

nit: please use normal layout of organizing overrides under a comment of the class they override, with any non-override methods at the top.

mattklein123 · 2017-01-20T17:02:32Z

source/common/router/config_impl.h

nit: please wrap comments at 100 col length.

nit: still needs comment wrapping fix

mattklein123 · 2017-01-20T17:02:52Z

source/common/router/config_impl.h

Should never happen, return nullptr.

mattklein123 · 2017-01-20T17:03:03Z

source/common/router/config_impl.h

should never happen, return this.

…tryImplBase

rshriram · 2017-01-21T04:41:35Z

added runtime support as well.

mattklein123 · 2017-01-22T21:11:09Z

@rshriram: @ccaraman will review first. I will take final pass once she is done.

ccaraman · 2017-01-23T17:55:52Z

docs/configuration/http_conn_man/route_config/route.rst

-  *(sometimes required, string)* If the route is not a redirect (*host_redirect* and/or
-  *path_redirect* is specified), *cluster* must be specified and indicates which upstream cluster
-  the request should be forwarded to.
+  *(sometimes required, string)* If the route is not a redirect (i.e., *host_redirect* and/or


nit: remove i.e.

ccaraman · 2017-01-23T17:59:22Z

source/common/router/config_impl.cc

      priority_(ConfigUtility::parsePriority(route)) {

+  bool have_weighted_clusters = route.hasObject("weighted_clusters");
+  bool have_cluster = !cluster_name_.empty() || have_weighted_clusters;


nit: new line below

mattklein123

overall looks good. small small comments/nits

mattklein123 · 2017-01-24T15:50:23Z

docs/configuration/http_conn_man/route_config/route.rst

+-----------------
+
+Compared to the ``cluster`` field that specifies a single upstream cluster as the target
+of a request, the ``weighted_clusters`` allows for specification of multiple upstream clusters


"the weighted_clusters option..."

mattklein123 · 2017-01-24T15:51:07Z

docs/configuration/http_conn_man/route_config/route.rst

+
+  weight
+    *(required, integer)* An integer between 0-100. When a request matches the route,
+    choice of an upstream cluster is determined by its weight. The sum of


"the choice"

mattklein123 · 2017-01-24T15:51:18Z

docs/configuration/http_conn_man/route_config/route.rst

+  weight
+    *(required, integer)* An integer between 0-100. When a request matches the route,
+    choice of an upstream cluster is determined by its weight. The sum of
+    weights across all entries in the ``clusters`` array should add up to 100.


"must add up to"

mattklein123 · 2017-01-24T15:53:28Z

docs/configuration/http_conn_man/route_config/weighted_routing.rst

+
+.. _config_http_conn_man_route_table_weighted_routing_probabilities:
+
+(b) Probabilistic route selection


This is somewhat of a duplication of https://lyft.github.io/envoy/docs/configuration/http_conn_man/route_config/traffic_shifting.html can we link and/or de-dup.

So I considered that briefly. I couldn't link/reuse that document as is, because it was targeting a 2 cluster use case. Without a 3 cluster example, pointing to that document was sort of giving the incorrect impression that you can create multiple route blocks, where the value can be distributed just like weighted clusters (e.g., 33, 33, 34).

On the other hand, moving the three cluster example to traffic_shifting didn't make sense either, because the title of the page was "traffic shifting from one version to another". IOW,
The traffic shift describes a basic simple use case (moving from one version to another). Traffic splitting describes a more generic use case (using a 3 cluster example to differentiate weights vs probabilities. It is applicable to the 2 version use case as well).

If we go down the de-dup path, we could eliminate traffic shifting, with the assumption that the 3 cluster example for probability based splitting will be sufficient to construct a 2-cluster example as well. IOW, kill traffic shift, keep traffic split. Thoughts?

I think I would kill traffic shifting in place of your new docs. @junr03 thoughts? You wrote the other doc.

P.S., you might just add a brief sentence or something to your doc that mentions the 2 cluster case. Whatever you think is best is fine with me.

yes, thats what I was thinking. if you and @junr03 have no objection to killing the traffic shifting doc, I was thinking of changing the weighted_routing to Traffic Splitting/Shifting, and add an example of two cluster case as well.

It looks like that would be a bigger change. For the moment, i have linked out to the traffic shifting doc as @mattklein123 suggested

mattklein123 · 2017-01-24T15:54:54Z

source/common/router/config_impl.cc

+
+    const Json::ObjectPtr weighted_clusters_json = route.getObject("weighted_clusters");
+
+    if (!weighted_clusters_json->hasObject("clusters")) {


I would just delete this check, we will catch this with schema (makes code simpler).

mattklein123 · 2017-01-24T15:57:44Z

source/common/router/config_impl.cc

  for (const RouteEntryImplBasePtr& route : routes_) {
-    if (route->matches(headers, random_value)) {
-      return route.get();
+    const Route* cEntry = route->matches(headers, random_value);


nit: s/cEntry/route

That would throw an error since route is the iteration variable in the for loop. I could rename it to something more sensible.

sorry, just pick another name like "entry" or "route_entry"

mattklein123 · 2017-01-24T15:58:11Z

source/common/router/config_impl.h

nit: still needs comment wrapping fix

mattklein123 · 2017-01-24T15:58:38Z

source/common/router/config_impl.h

+      return loader_.snapshot().getInteger(runtime_key_, cluster_weight_);
+    }
+
+    static const uint64_t MAX_CLUSTER_WEIGHT;


nit: variable definitions go below method definitions

mattklein123 · 2017-01-24T15:59:06Z

source/common/router/config_impl.h

+    const std::string runtime_key_;
+    Runtime::Loader& loader_;
+    const std::string cluster_name_;
+    uint64_t cluster_weight_;


mattklein123 · 2017-01-24T15:59:17Z

source/common/router/config_impl.h

+    const std::string cluster_name_;
+    uint64_t cluster_weight_;
+  };
+  typedef std::unique_ptr<WeightedClusterEntry> WeightedClusterEntryPtr;


nit: newline before this line

what's the convention for new lines?

I don't have anything typed out (sorry), and clang-format doesn't check for this. You can look at the rest of the code base which in general is very consistent. I'm sorry that this is annoying but it's important to keep things consistent and you will pick it up pretty quickly.

rshriram · 2017-01-24T17:53:49Z

addressed all except the traffic shifting/splitting issue. Waiting on feedback from @junr03 .

ccaraman · 2017-01-24T18:06:53Z

docs/configuration/http_conn_man/route_config/route.rst

 .. _config_http_conn_man_route_table_route_cluster:

 cluster
+  *(sometimes required, string)* If the route is not a redirect (i.e., *host_redirect* and/or


nit: remove ie.

ccaraman · 2017-01-24T18:20:57Z

test/common/router/config_impl_test.cc

+    EXPECT_EQ(nullptr, config.route(headers, 0)->redirectEntry());
+    EXPECT_EQ("www2", config.route(headers, 0)->routeEntry()->clusterName());
+  }
+  {


nit: new line

ccaraman · 2017-01-24T18:23:52Z

test/common/router/config_impl_test.cc

+  }
+}
+
+TEST(RouteMatcherTest, WeightedClusters) {


nit: Please add a test where random value is greater than 100.

junr03 · 2017-01-24T20:20:13Z

@rshriram after reading your doc it is obvious to me how you could do traffic shifting with the same mechanism, so I agree that having both docs is somewhat duplication. However, I think that it is better to have a traffic shifting example to make the use case explicit for users. I think it would be best to merge the two docs, eliminating duplicate info from the traffic shifting doc as needed.

mattklein123

looks great. thank you for all the fixes/changes.

rshriram · 2017-01-25T00:07:49Z

@junr03 I will send a separate PR to merge the two docs.

junr03 · 2017-01-25T00:08:42Z

great!

mattklein123 · 2017-01-25T00:17:17Z

@rshriram @junr03 sorry I thought this was done. A follow up docs PR sounds good.

* Removed pending requests management * Exposed ConfigLoadingStatus * Added description of the config loading status

Automatic merge from submit-queue. [DO NOT MERGE] Auto PR to update dependencies of mixerclient This PR will be merged automatically once checks are successful. ```release-note none ```

Signed-off-by: Kuat Yessenov <kuat@google.com>

**Commit Message** This adds timeouts field on AIServiceBackend to allow the configuration of timeouts per backend. **Related Issues/PRs (if applicable)** Fixes #361 --------- Signed-off-by: Loong <long0dai@foxmail.com>

mattklein123 reviewed Jan 20, 2017

View reviewed changes

Shriram Rajagopalan added 6 commits January 20, 2017 19:27

initial commit of weighted clusters

cc9a2b6

adding WeightedClusterEntry implementation

806daa8

convert WeightedClusterEntry impl into a private struct under RouteEn…

8f769dd

…tryImplBase

adding test cases for Weighted Clusters

a0976e7

fix leaking raw ptr

c8f6943

adding runtime support for weighted clusters

cb69f9d

rshriram force-pushed the weighted_clusters branch from c31b83f to cb69f9d Compare January 21, 2017 00:27

Shriram Rajagopalan added 2 commits January 20, 2017 20:08

addressing PR comments

a3c684d

move to unique_ptr and fix runtime key collision

990efb1

rshriram changed the title ~~[WIP] Support for multiple weighted clusters in routing~~ Support for multiple weighted clusters in routing Jan 21, 2017

Shriram Rajagopalan added 2 commits January 21, 2017 16:06

adding documentation

bbc64aa

check for empty clusters and fix TODOs

d671930

ccaraman reviewed Jan 23, 2017

View reviewed changes

addressing nits from @ccaraman

0b02250

mattklein123 reviewed Jan 24, 2017

View reviewed changes

addressing comments from Matt

279caf9

ccaraman reviewed Jan 24, 2017

View reviewed changes

Shriram Rajagopalan added 2 commits January 24, 2017 15:01

test with random_value >100

c4cdc8a

link to traffic shifting docs

cce025e

mattklein123 approved these changes Jan 24, 2017

View reviewed changes

mattklein123 merged commit 2debf05 into envoyproxy:master Jan 24, 2017

rshriram deleted the weighted_clusters branch January 30, 2017 02:38

rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018

Removed pending requests management (envoyproxy#361)

a5e6a34

* Removed pending requests management * Exposed ConfigLoadingStatus * Added description of the config loading status

jplevyak pushed a commit to jplevyak/envoy that referenced this pull request Jan 22, 2020

wasm: drop absl from time and duration (envoyproxy#361)

456bf9a

Signed-off-by: Kuat Yessenov <kuat@google.com>


		.. _config_http_conn_man_route_table_weighted_routing_probabilities:

		(b) Probabilistic route selection


		const Json::ObjectPtr weighted_clusters_json = route.getObject("weighted_clusters");

		if (!weighted_clusters_json->hasObject("clusters")) {

Conversation

rshriram commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram commented Jan 21, 2017

Uh oh!

mattklein123 commented Jan 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 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!

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!

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!

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!

rshriram commented Jan 24, 2017

Uh oh!

rshriram commented Jan 20, 2017 •

edited

Loading

junr03 commented Jan 24, 2017 •

edited

Loading