server: Introduce ServerFactoryContext by lambdai · Pull Request #8485 · envoyproxy/envoy

lambdai · 2019-10-04T06:34:24Z

Description:
Introduce ServerFactoryContext which is an implementation of CommonFactoryContext. Comparing to ListenerFactoryContext, ServerFactoryContext has a wider lifetime.
This is the prerequisite to share rds configuration among listeners.

Also experimenting move messageValidateVisitor out of CommonFactoryContext. The benefit is that now Server::Instance perfectly implements CommonFactoryContext.

ServerFactoryContext doesn't implement the function. The test shows it is never invoked.
Will put messageValidateVisitor into FactoryContext: this piece of work will be done in follow up PR

Notes to reviewers:
~~clang-tidy error is not introduced in this PR. Create #8509 to track~~
~~Will fix once this PR is merged~~

This PR is extracted from #8454

Risk Level:
Testing:
Docs Changes:
Release Notes:
Fixes #8303

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2019-10-04T06:37:32Z

Not fixing all the tests yet. Will resume tomorrow

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

htuch · 2019-10-07T14:24:32Z

/assign @htuch

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2019-10-08T07:02:39Z

clang_tidy passed!

lambdai · 2019-10-08T16:46:25Z

Also CC @mattklein123 as in the #8303

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai

Thank you for the review! I plan to chat with @htuch on how {dynamic,static} message valiadator should be provided in server::Instance and ServerFactoryContext.

source/common/local_info/local_info_impl.h

source/common/router/scoped_rds.cc

include/envoy/server/filter_config.h

source/server/server.h

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2019-10-14T21:46:45Z

Chatted with @htuch
The goal is not to introduce is_dynamic arg and the messageValidationVisitor in ServerFactoryContext. So that ServerFactoryContext is a subset of server::Instance.

The plan is to

messageValidationVisitor() should not be used in ServerFactoryContext. Mark it NOT_IMPLEMENT. Provide a visitor ref if it is needed.
Moving messageValidationVisitor out of CommonFactoryContext in the follow up PR.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2019-10-15T17:40:47Z

/retest

repokitteh-read-only · 2019-10-15T17:40:55Z

🔨 rebuilding ci/circleci: docs (failed build)
🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)
🔨 rebuilding ci/circleci: release (failed build)
🔨 rebuilding ci/circleci: api (failed build)
🔨 rebuilding ci/circleci: filter_example_mirror (failed build)

🐱

Caused by: a #8485 (comment) was created by @lambdai.

see: more, trace.

lambdai · 2019-10-15T23:54:53Z

/retest

repokitteh-read-only · 2019-10-15T23:54:55Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #8485 (comment) was created by @lambdai.

see: more, trace.

lambdai · 2019-10-16T18:39:14Z

It's probably flaky test since I couldn't reproduce
/azp run

mattklein123

Thanks, nice change. Some small nits and will defer to @htuch for further review.

source/common/router/rds_impl.cc

source/server/listener_manager_impl.h

mattklein123 · 2019-10-16T23:43:02Z

source/server/server.h

+  Stats::Scope& scope() override { return *server_scope_; }
+  Singleton::Manager& singletonManager() override { return server_.singletonManager(); }
+  ThreadLocal::Instance& threadLocal() override { return server_.threadLocal(); }
+  // TODO(lambdai): remove messageValidationVisitor from ServerFactoryContext


Can this just be done as part of this change?

Please allow me to address it in a follow up PR

OK, but this really should be removed in some PR in this space, as it's a problematic pattern, we now have multiple validation visitor accessors apparent in the per-route filter factory method.

lambdai · 2019-10-17T03:58:49Z

@mattklein123 Thanks!
The next PRs will be out pretty soon. With reasonable size and avoid you reviewers going though this large but plain PR.
And the nit will be fixed there

lambdai · 2019-10-17T17:01:14Z

Oops. I am resolving the conflict
I suspect it's due to another PR by me...

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2019-10-18T00:10:30Z

@mattklein123
The last commit is about merging
the listener_impl.h split PR
and
forward proxy which uses ConfigPerRoute

lambdai · 2019-10-18T16:23:11Z

@mattklein123 I think the failed envoy-macos is pure test infra failure. Is it good to merge? If not, should i merge master and retrigger the tests? Thanks!

mattklein123 · 2019-10-18T16:30:25Z

@lambdai the change LGTM but @htuch had previously reviewed so I would like him to verify and merge. Thank you!

lambdai · 2019-10-18T17:34:50Z

@mattklein123 Thank you for the update!

htuch

Thanks, nice correctness improvement!

htuch · 2019-10-18T21:30:00Z

source/server/server.h

+  Stats::Scope& scope() override { return *server_scope_; }
+  Singleton::Manager& singletonManager() override { return server_.singletonManager(); }
+  ThreadLocal::Instance& threadLocal() override { return server_.threadLocal(); }
+  // TODO(lambdai): remove messageValidationVisitor from ServerFactoryContext


OK, but this really should be removed in some PR in this space, as it's a problematic pattern, we now have multiple validation visitor accessors apparent in the per-route filter factory method.

lambdai added 9 commits October 4, 2019 04:10

filter_config.h

1976790

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

more files

4cf8a4d

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

rds_impl

78b99c9

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

vhds srds

72092e5

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

many filters

8e124e4

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

validationserver

c2f607b

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

server and l_m_i

072b07e

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

build envoy-static!

2db27cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

fix build test and fix listener_manager_test

2ea5687

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai requested review from alyssawilk, dio and mattklein123 as code owners October 4, 2019 06:34

lambdai mentioned this pull request Oct 4, 2019

Proof of Concept: Introduce ServerFactoryContext #8454

Closed

lambdai added 5 commits October 4, 2019 23:17

fix Destroyduinginit

04b53be

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

fix all tests

d022f01

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

clang-tiny

d6a8ba3

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

fix clangtidy

991a6fd

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

merge master

ebc3bf8

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai mentioned this pull request Oct 6, 2019

Clang-tidy report Server::Instance invoke virtual function in constructor #8509

Closed

repokitteh-read-only bot assigned htuch Oct 7, 2019

lambdai mentioned this pull request Oct 7, 2019

xds: share rds configuration among envoy #8523

Closed

lambdai added 3 commits October 7, 2019 21:35

Merge branch 'master' into serverfactorycontext

fe2d3d1

fix ValidationInstance

79d707a

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

another round of clang_tidy, error and warning

6d88265

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123 self-assigned this Oct 8, 2019

merge master

bf7307c

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai commented Oct 14, 2019

View reviewed changes

address comment

308699b

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Oct 14, 2019

mattklein123 added the waiting label Oct 15, 2019

remove validation visitor from serverfactorycontext

77761e0

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Oct 15, 2019

clang_tidy

ca9462b

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Merge remote-tracking branch 'origin/master' into serverfactorycontext

3aec6be

lambdai requested a review from mattklein123 October 15, 2019 21:18

Merge branch 'master' into serverfactorycontext

58650f0

mattklein123 previously approved these changes Oct 16, 2019

View reviewed changes

mattklein123 removed their assignment Oct 16, 2019

rebase and fix nits

e58b35a

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai dismissed mattklein123’s stale review via e58b35a October 17, 2019 18:52

htuch approved these changes Oct 18, 2019

View reviewed changes

htuch merged commit 44a4f1c into envoyproxy:master Oct 18, 2019

lambdai deleted the serverfactorycontext branch October 18, 2019 22:24

lambdai mentioned this pull request Oct 19, 2019

rds: make RouteConfigProvider shared among ListenerImpls, or move factory_context_ outside of Listener. #8303

Closed

Conversation

lambdai commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lambdai commented Oct 4, 2019

Uh oh!

htuch commented Oct 7, 2019

Uh oh!

lambdai commented Oct 8, 2019

Uh oh!

lambdai commented Oct 8, 2019

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdai commented Oct 14, 2019

Uh oh!

lambdai commented Oct 15, 2019

Uh oh!

repokitteh-read-only bot commented Oct 15, 2019

Uh oh!

lambdai commented Oct 15, 2019

Uh oh!

repokitteh-read-only bot commented Oct 15, 2019

Uh oh!

lambdai commented Oct 16, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattklein123 Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

lambdai Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

htuch Oct 18, 2019 • 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!

lambdai commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lambdai commented Oct 17, 2019

Uh oh!

lambdai commented Oct 18, 2019

Uh oh!

lambdai commented Oct 18, 2019

Uh oh!

mattklein123 commented Oct 18, 2019

Uh oh!

lambdai commented Oct 18, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Oct 18, 2019 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lambdai commented Oct 4, 2019 •

edited

Loading

htuch Oct 18, 2019 •

edited

Loading

lambdai commented Oct 17, 2019 •

edited

Loading

htuch Oct 18, 2019 •

edited

Loading