Clusterinfo consistency #4600

stevenzzzz · 2018-10-04T20:39:25Z

Description:
Cache threadlocalCluster.info in ActiveStream and open it to filters via StreamFilterCallbacks.

Risk Level: Low (nobody uses it).
Testing: unit tests.
Docs Changes:
Release Notes:
[Optional Fixes #Issue] #4558
[Optional Deprecated:]

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

stevenzzzz · 2018-10-04T20:42:33Z

@mattklein123 Hey Matt PTAL and see if it's okay. (I made it to be always on as I don't think it should be off when we have the API).

mattklein123

This looks like it's on the right track. Flushing out a few comments. Two things:

Looks like CI is broken, can you take a look?
Can you look at our built-in filters and see where you can use the new cached calls instead of an additional lookup? Quite a bit of cleanup is possible I think.

mattklein123 · 2018-10-05T00:19:48Z

include/envoy/http/filter.h

+   * Returns the clusterInfo for the current request.
+   * This method is to avoid multiple look ups in the filter chain, it also provides a consistent
+   * view of clusterInfo after a route is picked/repicked.
+   * NOTE: Cached clusterInfo and route should be updated the same time.


s/should be/will be

mattklein123 · 2018-10-05T00:20:02Z

include/envoy/http/filter.h

@@ -111,6 +112,13 @@ class StreamFilterCallbacks {
   */
  virtual Router::RouteConstSharedPtr route() PURE;

+  /**
+   * Returns the clusterInfo for the current request.


s/request/cached route

mattklein123 · 2018-10-05T00:21:05Z

source/common/http/conn_manager_impl.cc

+  } else {
+    Upstream::ThreadLocalCluster* local_cluster =
+        connection_manager_.cluster_manager_.get(route->routeEntry()->clusterName());
+    cached_cluster_info_ = local_cluster->info();


local_cluster can be nullptr. Please add test that covers this case.

Thanks for the catch.

mattklein123 · 2018-10-05T00:21:52Z

source/common/http/conn_manager_impl.cc

@@ -1477,8 +1484,17 @@ Tracing::Span& ConnectionManagerImpl::ActiveStreamFilterBase::activeSpan() {

 Tracing::Config& ConnectionManagerImpl::ActiveStreamFilterBase::tracingConfig() { return parent_; }

+Upstream::ClusterInfoConstSharedPtr ConnectionManagerImpl::ActiveStreamFilterBase::clusterInfo() {
+  // NOTE: Refreshing route caches clusterInfo as well.
+  if (!parent_.cached_route_.has_value()) {


I don't think this is necessary as we don't do this for the route call. The HCM will initialize the cache before calling any filters?

I think we did the same for route(), see route() right below?
I think the cached_cluster_info has the same position as the cached_route. Say after an entity clearRouteCache and a fitler wants to access clusterInfo without knowing that, that could be a problem?

Right, sorry, makes sense.

…no cluster' cached_cluster_info test case Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

stevenzzzz · 2018-10-05T21:29:03Z

Hey @mattklein123 Matt, I want to make the switch in another PR if that's fine by you?

mattklein123 · 2018-10-05T21:31:18Z

Hey @mattklein123 Matt, I want to make the switch in another PR if that's fine by you?

Do you promise you will do a follow up PR right after this one?

Can you please also merge master?

stevenzzzz · 2018-10-05T21:39:52Z

sure thing. :)
I found there is another similar cache route implementation, but at network filter layer: https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/network/thrift_proxy/conn_manager.cc
I think we want to do the same thing there?

mattklein123 · 2018-10-05T21:44:57Z

I think we want to do the same thing there?

cc @zuercher ^. Yeah we likely do but I wouldn't worry about it right now.

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

stevenzzzz · 2018-10-05T22:10:00Z

wow, so all I did is:
git checkout master
git pull upstream master
git checkout feature-branch
git merge master
git commit
git push origin

Annnd there are 160+ files in my PR. Which I think most changes are in sync with master HEAD but shown as diff here.
Sorry for the mess, I need some git help to figure out what happened.

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

mattklein123 · 2018-10-07T20:34:32Z

@stevenzzzz please check CI

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

merge master. Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

stevenzzzz

@stevenzzzz please check CI

Thanks! Fixed a bug. CI should be happy now.

stevenzzzz · 2018-10-08T15:28:30Z

source/common/http/conn_manager_impl.cc

@@ -1477,8 +1484,17 @@ Tracing::Span& ConnectionManagerImpl::ActiveStreamFilterBase::activeSpan() {

 Tracing::Config& ConnectionManagerImpl::ActiveStreamFilterBase::tracingConfig() { return parent_; }

+Upstream::ClusterInfoConstSharedPtr ConnectionManagerImpl::ActiveStreamFilterBase::clusterInfo() {
+  // NOTE: Refreshing route caches clusterInfo as well.
+  if (!parent_.cached_route_.has_value()) {


mattklein123

LGTM, thanks. Small nit. Reminder that you promised to do the filter cleanup right after this PR. :)

mattklein123 · 2018-10-08T17:05:45Z

include/envoy/http/filter.h

+   * view of clusterInfo after a route is picked/repicked.
+   * NOTE: Cached clusterInfo and route will be updated the same time.
+   */
+  virtual Upstream::ClusterInfoConstSharedPtr clusterInfo() PURE;


nit: newline after this line

done. Thought fix_format would catch this.

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

stevenzzzz

Thanks for the reminder. working on filter cleanup after this PR.

stevenzzzz · 2018-10-08T17:28:03Z

include/envoy/http/filter.h

+   * view of clusterInfo after a route is picked/repicked.
+   * NOTE: Cached clusterInfo and route will be updated the same time.
+   */
+  virtual Upstream::ClusterInfoConstSharedPtr clusterInfo() PURE;


done. Thought fix_format would catch this.

mattklein123

Thanks. I opened #4643 to track the follow up. Thank you!

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com> Signed-off-by: Aaltan Ahmad <aa@stripe.com>

Xin Zhuang added 3 commits October 4, 2018 16:34

cache clusterInfo in ActiveStream and open it to filters via callbacks

76a4cd5

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

add tests for conn-manager-impl

abcdad0

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

save fix format

1118f33

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

mattklein123 requested changes Oct 5, 2018

View reviewed changes

junr03 assigned mattklein123 Oct 5, 2018

fix unknown(possible naive git op caused) errors and add 'has-route, …

9090f16

…no cluster' cached_cluster_info test case Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

merge master

dda83c1

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

stevenzzzz requested review from lizan and zuercher as code owners October 5, 2018 22:01

stevenzzzz closed this Oct 5, 2018

stevenzzzz reopened this Oct 5, 2018

sync master from upstream, and merge master again

b4f0c53

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

Xin Zhuang added 2 commits October 8, 2018 10:19

fix bug, the cluster_entry can be null in a route

0fe3b68

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

Merge branch 'master' into clusterinfo-consistency

d1ffe37

merge master. Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

stevenzzzz commented Oct 8, 2018

View reviewed changes

mattklein123 reviewed Oct 8, 2018

View reviewed changes

fix nits

30925c6

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

stevenzzzz commented Oct 8, 2018

View reviewed changes

mattklein123 mentioned this pull request Oct 8, 2018

use new cached clusterInfo() in built-in filters #4643

Closed

mattklein123 approved these changes Oct 8, 2018

View reviewed changes

mattklein123 merged commit c5be849 into envoyproxy:master Oct 8, 2018

stevenzzzz mentioned this pull request Oct 10, 2018

Switch to use callbacks->clusterInfo in built-in filters' code. #4674

Merged

aa-stripe pushed a commit to aa-stripe/envoy that referenced this pull request Oct 11, 2018

Clusterinfo consistency (envoyproxy#4600)

6cde28c

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com> Signed-off-by: Aaltan Ahmad <aa@stripe.com>

stevenzzzz deleted the clusterinfo-consistency branch May 9, 2019 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clusterinfo consistency #4600

Clusterinfo consistency #4600

stevenzzzz commented Oct 4, 2018 •

edited

Loading

stevenzzzz commented Oct 4, 2018

mattklein123 left a comment

mattklein123 Oct 5, 2018

stevenzzzz Oct 5, 2018

mattklein123 Oct 5, 2018

stevenzzzz Oct 5, 2018

mattklein123 Oct 5, 2018

stevenzzzz Oct 5, 2018

mattklein123 Oct 5, 2018

stevenzzzz Oct 5, 2018

mattklein123 Oct 5, 2018

stevenzzzz Oct 8, 2018

stevenzzzz commented Oct 5, 2018 •

edited

Loading

mattklein123 commented Oct 5, 2018

stevenzzzz commented Oct 5, 2018

mattklein123 commented Oct 5, 2018

stevenzzzz commented Oct 5, 2018

mattklein123 commented Oct 7, 2018

stevenzzzz left a comment

stevenzzzz Oct 8, 2018

mattklein123 left a comment

mattklein123 Oct 8, 2018

stevenzzzz Oct 8, 2018

stevenzzzz left a comment

stevenzzzz Oct 8, 2018

mattklein123 left a comment

Clusterinfo consistency #4600

Clusterinfo consistency #4600

Conversation

stevenzzzz commented Oct 4, 2018 • edited Loading

stevenzzzz commented Oct 4, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenzzzz commented Oct 5, 2018 • edited Loading

mattklein123 commented Oct 5, 2018

stevenzzzz commented Oct 5, 2018

mattklein123 commented Oct 5, 2018

stevenzzzz commented Oct 5, 2018

mattklein123 commented Oct 7, 2018

stevenzzzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenzzzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

stevenzzzz commented Oct 4, 2018 •

edited

Loading

stevenzzzz commented Oct 5, 2018 •

edited

Loading