listener, srds, rds, vhds: replace FactoryContext with ServerFactoryContext in router/... #9779

stevenzzzz · 2020-01-22T19:42:03Z

replace FactoryContext with ServerFactoryContext in router/..., So that SRDS/RDS/VHDS subscription could outlive their creator listener.

Signed-off-by: Xin Zhuang stevenzzz@google.com

Description:replace FactoryContext with ServerFactoryContext in router/..., So that SRDS/RDS/VHDS subscription could outlive their creator listener.
This PR:

Let ListenerImpl only pass down the dynamic_init_manager_ it owns to its children entities (HCMConfig, SRDS-Subscription, RDS-Subscription etc).
Construct a tree like init-manager & target dependency graph, by adding a init-target at each listener-impl, srds-subscription, rds-subscription.
Replace FactoryContext with ServerFactoryContext, a SRDS/RDS subscription can out live its creator listener impl for real now.

Risk Level: Med
There is one behavior change, that previously during a listenerImpl's initManager initializing [which could be either Server::initManager or Listener::initManager], if there are multiple SRDS or RDS responses that contains sub-resource, their targets are added to the listener::initManager() and will bar on the listerner::initManager() ready. With this modification, the sub-resources contains in the second response of a SRDS/RDS may not be able to bar the local-init-manager's ready(), if the first response's sub-resources are initialized and called ready on the SRDS's local init-manger ready() aheader of the second's response's arrival.
before==> SRDS(A, B, C) bar Listener::initManager(),
now ===> SRDS::local_init_manager bars Listener::initManager(), and (A, B) bars on local_init_manager, (C) in a second response may or may not bar on Listener::initManager() depending on if (A,B) finish initialization before (C)'s arrival.
Testing: unit test
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

…t SRDS/RDS/VHDS subscription could outlive their creator listener Signed-off-by: Xin Zhuang <stevenzzz@google.com>

htuch

General approach looks good, thanks, but I think there's still some issues around validation visitor inference with this PR.
/wait

htuch · 2020-01-24T03:07:19Z

source/common/config/config_provider_impl.cc

@@ -21,6 +21,7 @@ ImmutableConfigProviderBase::~ImmutableConfigProviderBase() {

 ConfigSubscriptionCommonBase::~ConfigSubscriptionCommonBase() {
  init_target_.ready();
+


Nit: please avoid gratuitous whitespace changes.

htuch · 2020-01-24T03:11:33Z

source/common/router/rds_impl.cc

-    : config_(new ConfigImpl(config, factory_context.getServerFactoryContext(),
-                             factory_context.messageValidationVisitor(), true)),
+    : config_(new ConfigImpl(config, factory_context,
+                             factory_context.messageValidationContext().staticValidationVisitor(),


I think this still might not be correct; what if you have an inline route in an HCM that is delivered via a Listener in a LDS message? The validation should be dynamic.

revert it. This PR is supposed to be a child PR of #9621.

But we can also work on this PR to solve the problem in one shot. :)

if we don't want FactoryContext to contain a messageValidationVisitor() method, I can do another cleanup PR which will pipe the validator from ListenerImpl --> ... --> HCM::Config.

htuch · 2020-01-24T03:13:09Z

source/common/router/rds_impl.cc

+      parent_init_target_(fmt::format("RdsRouteConfigSubscription init {}", route_config_name_),
+                          [this]() { local_init_manager_.initialize(init_watcher_); }),
+      init_watcher_(fmt::format("RDS local-init-watcher {}", rds.route_config_name()),
+                    [this]() { parent_init_target_.ready(); }),


This makes sense to me and is roughly how I was envisaging transitive init managers working. @mergeconflict @lambdai did you have any thoughts here?

LGTM. Based on my understanding this field initialization order doesn't introduce initialized-after-detroy

I think it's fine, if it's needed.

lambdai · 2020-01-24T18:33:43Z

source/server/listener_impl.h

+  std::unique_ptr<Init::WatcherImpl> init_watcher_;
+  // A target is added to Server's InitManager if workers_started_ is false.
+  std::unique_ptr<Init::TargetImpl> listener_init_target_;


When the ListenerImpl is in destroyed, listener_init_target_ is destroyed prior to init_watcher_
Is it possible it triggered init_watcher_ in between?

ListenerImpl destruction is supposed to be an "atomic" operation in master thread, same as listener_init_target_'s triggering. I think it's fine here?

Sorry for the churn. I agree it's fine. Although for different reason :)
On top of ~ListenerImpl() there might be ListenerManagerImpl::startWorkers() if this listener is the last uninitialized listener. It's fine because this destroying listener won't in the start list

mergeconflict

I just left a few comments here regarding init manager / target / handle usage (I didn't pay attention to anything else).

mergeconflict · 2020-01-24T19:51:46Z

source/server/listener_impl.h

@@ -224,13 +225,15 @@ class ListenerImpl : public Network::ListenerConfig,
  const uint64_t hash_;
  ProtobufMessage::ValidationVisitor& validation_visitor_;

+  // This init watcher, if available, notifies the "parent" listener manager when listener
+  // initialization is complete. It may be reset to cancel interest.
+  std::unique_ptr<Init::WatcherImpl> init_watcher_;


I know it was already like this, but I don't think this needs to be a unique_ptr.

mergeconflict · 2020-01-24T19:54:30Z

source/server/listener_impl.h

+  // initialization is complete. It may be reset to cancel interest.
+  std::unique_ptr<Init::WatcherImpl> init_watcher_;
+  // A target is added to Server's InitManager if workers_started_ is false.
+  std::unique_ptr<Init::TargetImpl> listener_init_target_;


This doesn't really need to be a unique_ptr either. I'd recommend always creating the target, so that you don't have to worry about ASSERT(listener_init_target_ != nullptr), and just add it to the parent's server's init manager if workers haven't been started.

mergeconflict · 2020-01-24T19:57:06Z

source/common/router/rds_impl.h

+  Init::SharedTargetImpl parent_init_target_;
+
+  // Init watcher on RDS and VHDS ready event. This watcher marks parent_init_target_ ready.
+  Init::WatcherImpl init_watcher_;


Might want to call this parent_init_watcher_ or local_init_watcher_ (I'm not sure which) for consistency.

moved to config-provider-framework now. named to local_init_watcher_.

latest init manager related changes still lgtm

Thanks!

mergeconflict · 2020-01-24T19:58:47Z

source/common/router/rds_impl.cc

+      parent_init_target_(fmt::format("RdsRouteConfigSubscription init {}", route_config_name_),
+                          [this]() { local_init_manager_.initialize(init_watcher_); }),
+      init_watcher_(fmt::format("RDS local-init-watcher {}", rds.route_config_name()),
+                    [this]() { parent_init_target_.ready(); }),


I think it's fine, if it's needed.

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

mergeconflict

init manager related changes lgtm, thanks!

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

mergeconflict

latest init manager related changes still lgtm

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

stevenzzzz

Thanks

junr03 · 2020-02-06T00:32:03Z

@stevenzzzz sorry for the delay here. I have been a bit swamped these days. Will prioritize taking a look tomorrow.

junr03

thanks for working on this. A few comments

junr03 · 2020-02-06T18:15:02Z

include/envoy/server/filter_config.h

@@ -55,6 +55,12 @@ class CommonFactoryContext {
   */
  virtual const LocalInfo::LocalInfo& localInfo() const PURE;

+  /**
+   * @return ProtobufMessage::ValidationContext& validation visitor for xDS&static configuration


Suggested change

* @return ProtobufMessage::ValidationContext& validation visitor for xDS&static configuration

* @return ProtobufMessage::ValidationContext& validation visitor for xDS and static configuration

junr03 · 2020-02-06T18:17:34Z

include/envoy/router/route_config_provider_manager.h

   */
  virtual RouteConfigProviderPtr
  createStaticRouteConfigProvider(const envoy::config::route::v3::RouteConfiguration& route_config,
-                                  Server::Configuration::FactoryContext& factory_context) PURE;
+                                  Server::Configuration::ServerFactoryContext& factory_context,
+                                  ProtobufMessage::ValidationVisitor& validator) PURE;


I need to read the rest of the PR, but given the change below which add messageValidationVisitor() to the CommonFactoryContext, why is this function also taking a validator as a parameter?

@stevenzzzz I am still wondering about this

sorry, missed this comment. I think you mean "add a validation visitor context"?
It's because we can't tell from the current ServerFactoryContext if the static validator or the dynamic validator should be used.
For example, a static Route config provider could be put into a dynamic(LDS) listener's config, in which case we should use the dynamic validator instead (per Harvey).
For that, we will have to pass validator alongside the Serverfatorycontext.

I see, so only the code that is building the config provider knows if it should be using the static or dynamic visitor, but the constructor itself does not. Hence why the caller of createStaticRouteConfigProvider needs to pass the intended visitor.

Not to bike shed, but it seems that we have gone in one direction here, and the opposite in the ListernerImpl constructor where we used to pass the validator reference but now we decide inside the constructor. Should we pass an added_via_api boolean here and have the constructor decide?

IMHO, I'd like the validator choice made as close to the consumer entity(in this case, the listenerImpl) as possible, which provides better modularity.

The listener could make the decision by itself as there is a somewhat hacky paramter add_via_api in its constructor, the added_via_api information could be deducted by any RDS-config-provider or any subscription-initiated config-provider,
but not in static config provider: its context doesn't provide enough information for making the choice.

Should we pass an added_via_api boolean here and have the constructor decide?

Right, that is why I was suggesting if we should hint the static config provider as well (albeit I have not looked if we have access to the same hint when we create this config provider, but my intuition tells me we do). I am not sure what the right answer here is. My point is that we seem to have gone in opposite directions where we could have the same setup.

Perhaps the next reviewer can state their preference.

discussed this w/ Harvey before, passing the correct validator is preferred over hinting (passing the added_via_api signal). Agreed it's a preference thing as to if we want to keep all the places have the same interface.

junr03 · 2020-02-06T21:44:07Z

source/common/config/config_provider_impl.h

+  Init::TargetImpl parent_init_target_;
+  // Watcher that marks parent_init_target_ ready when the local init manager is ready.
+  Init::WatcherImpl local_init_watcher_;
+  // Local manager that tracks the subscription's sub-resources(RDS subscriptions) initialization.


Suggested change

// Local manager that tracks the subscription's sub-resources(RDS subscriptions) initialization.

// Local manager that tracks the subscription's sub-resources initialization. For example, RDS in LDS.

Because RDS is just one example, right?

junr03 · 2020-02-06T21:57:12Z

source/common/router/scoped_rds.h

@@ -170,13 +171,14 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio

  // ScopedRouteInfo by scope name.
  ScopedRouteMap scoped_route_map_;
+  // For creating RDS subscriptions.
+  Server::Configuration::ServerFactoryContext& factory_context_;


why did this move in order?

revert it. It's unnecessary now.

ping on the revert

pong.
I think I updated this in another backup branch, oops.

junr03 · 2020-02-06T22:00:33Z

source/server/listener_impl.cc

@@ -392,18 +411,13 @@ void ListenerImpl::initialize() {
  // per listener init manager. See ~ListenerImpl() for why we gate the onListenerWarmed() call
  // by resetting the watcher.
  if (workers_started_) {
-    dynamic_init_manager_.initialize(*init_watcher_);
+    // If workers_started_ is true, dynamic_init_manager_ should be initialized by listener manager
+    // directly..


Suggested change

// directly..

// directly.

junr03 · 2020-02-06T22:14:51Z

source/server/listener_impl.cc

@@ -309,7 +326,6 @@ ListenerImpl::~ListenerImpl() {
  // a local init manager we should block the notification from trying to move us from warming to


Ok, I think I follow what happened here in terms of changing having different init managers and resetting this watch, vs having the new target and arrangement of firings.

However, there are some outdated comments throughout this file. The above for instance, as this destructor does not any longer reset the init_watcher_. Do you mind going through the file and deleting and updating comments. I think the process here is tricky and being extra crisp on what is going on would make the next person reading this have an easier time.

oops, this is back!
I have a major git sync problem with master and had to resolve w/ upstream master one by one. I will do a around of comment cleanup.

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

stevenzzzz · 2020-02-28T22:47:33Z

At a high level this LGTM. @wgallagher do you mind reviewing just the xDS plumbing part of the change?

@stevenzzzz given how complicated this code is, can we potentially get an integration test that covers a very complex init case? IMO this shouldn't be that hard given all the plumbing we have for writing xDS integration tests?

/wait

PTAL
Sorry for the delayed reply, lots going on recently. I added a integration test w/ name listener_lds_integration_test.cc.

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

mattklein123

Thanks for a ton for adding the integration test, nice work. Small comment nit and needs a format fix.

/wait

mattklein123 · 2020-02-29T00:06:58Z

test/integration/listener_lds_integration_test.cc

+INSTANTIATE_TEST_SUITE_P(IpVersionsAndGrpcTypes, ListenerIntegrationTest,
+                         GRPC_CLIENT_INTEGRATION_PARAMS);
+
+// Tests that a LDS deletion before Server initManager been initialized will block the Server from


s/will block/will not block ?

aah, that's the whole point of this test. :0

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

stevenzzzz · 2020-03-02T14:49:25Z

Thanks!

mattklein123 · 2020-03-02T21:21:53Z

CI issue looks legit. Can you merge master and take a look?

/wait

…al-look-longlive-srds Signed-off-by: Xin Zhuang <stevenzzz@google.com>

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

stevenzzzz · 2020-03-03T15:16:19Z

probably because of difference between libc++ and libstdc++, can't reproduce the error on my local box.

stevenzzzz · 2020-03-03T18:11:16Z

/retest
not sure if the failing test is related to this PR.

repokitteh-read-only · 2020-03-03T18:11:19Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #9779 (comment) was created by @stevenzzzz.

see: more, trace.

mattklein123

Thanks!

stevenzzzz · 2020-03-04T02:15:51Z

thank you.

…

On Tue, Mar 3, 2020 at 7:44 PM Matt Klein ***@***.***> wrote: Merged #9779 <#9779> into master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9779?email_source=notifications&email_token=AA2I6TBYKXLZQ7UU3X5JRCLRFWP5LA5CNFSM4KKLLZD2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOXBY23DQ#event-3094457742>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2I6TF6PDNZP5I43GPD2QTRFWP5LANCNFSM4KKLLZDQ> .

-- Xin Zhuang

replace FactoryContext with ServerFactoryContext in router/... so tha…

7b3bfc9

…t SRDS/RDS/VHDS subscription could outlive their creator listener Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz requested review from alyssawilk and mattklein123 as code owners January 22, 2020 19:42

stevenzzzz mentioned this pull request Jan 22, 2020

add messageValidationContext into CommonFactoryContext so xDS could u… #9621

Closed

htuch suggested changes Jan 24, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 24, 2020

htuch self-assigned this Jan 24, 2020

lambdai reviewed Jan 24, 2020

View reviewed changes

mergeconflict previously approved these changes Jan 24, 2020

View reviewed changes

stevenzzzz dismissed mergeconflict’s stale review via abc80ba January 27, 2020 16:45

repokitteh-read-only bot removed the waiting label Jan 27, 2020

fix feedbacks

e8de6c1

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

stevenzzzz force-pushed the final-look-longlive-srds branch from abc80ba to e8de6c1 Compare January 27, 2020 16:56

stevenzzzz changed the title ~~WIP[listener, srds, rds, vhds]: replace FactoryContext with ServerFactoryContext in router/...~~ listener, srds, rds, vhds: replace FactoryContext with ServerFactoryContext in router/... Jan 27, 2020

if only we can turn up spelling check at local

64757d1

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

mergeconflict previously approved these changes Jan 27, 2020

View reviewed changes

stevenzzzz added 2 commits January 28, 2020 14:43

merge with master

17570bc

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

fix typo introduced during merging w/master

282a4c9

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

stevenzzzz dismissed mergeconflict’s stale review via 282a4c9 January 28, 2020 21:23

move local-init-manager to config-provider framework

71842d5

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

mergeconflict previously approved these changes Jan 31, 2020

View reviewed changes

merge upstream master

2c4b342

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

stevenzzzz dismissed mergeconflict’s stale review via 2c4b342 February 3, 2020 18:06

yanavlasov assigned lambdai and junr03 and unassigned htuch Feb 3, 2020

stevenzzzz commented Feb 5, 2020

View reviewed changes

junr03 suggested changes Feb 6, 2020

View reviewed changes

fixes per review feedbacks

86799d4

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

stevenzzzz requested review from jmarantz, lizan, PiotrSikora, snowp and zuercher as code owners February 28, 2020 22:29

repokitteh-read-only bot removed the waiting label Feb 28, 2020

stevenzzzz added 2 commits February 28, 2020 17:39

merge from master

2dadf40

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

some comment update on integration test

7f7786f

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

fix format and typo

ebd5d3d

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

mattklein123 requested changes Feb 29, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 29, 2020

fix typo, one at a time

65eb728

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

repokitteh-read-only bot removed the waiting label Mar 2, 2020

repokitteh-read-only bot added the waiting label Mar 2, 2020

stevenzzzz added 2 commits March 2, 2020 16:24

Merge branch 'master' of https://github.com/envoyproxy/envoy into fin…

8145447

…al-look-longlive-srds Signed-off-by: Xin Zhuang <stevenzzz@google.com>

make type more specific

d1204f1

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

repokitteh-read-only bot removed the waiting label Mar 2, 2020

fix compilation error

b08bded

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

mattklein123 approved these changes Mar 3, 2020

View reviewed changes

mattklein123 merged commit 0957bfe into envoyproxy:master Mar 4, 2020

stevenzzzz deleted the final-look-longlive-srds branch March 4, 2020 02:46

stevenzzzz mentioned this pull request Mar 4, 2020

VHDS subscription could refer a destructed init manager #9254

Closed

		@@ -21,6 +21,7 @@ ImmutableConfigProviderBase::~ImmutableConfigProviderBase() {

		ConfigSubscriptionCommonBase::~ConfigSubscriptionCommonBase() {
		init_target_.ready();

	* @return ProtobufMessage::ValidationContext& validation visitor for xDS&static configuration
	* @return ProtobufMessage::ValidationContext& validation visitor for xDS and static configuration

	// Local manager that tracks the subscription's sub-resources(RDS subscriptions) initialization.
	// Local manager that tracks the subscription's sub-resources initialization. For example, RDS in LDS.

		@@ -309,7 +326,6 @@ ListenerImpl::~ListenerImpl() {
		// a local init manager we should block the notification from trying to move us from warming to

listener, srds, rds, vhds: replace FactoryContext with ServerFactoryContext in router/... #9779

listener, srds, rds, vhds: replace FactoryContext with ServerFactoryContext in router/... #9779

Uh oh!

Conversation

stevenzzzz commented Jan 22, 2020

Uh oh!

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

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

mergeconflict left a comment

Choose a reason for hiding this comment

Uh oh!

mergeconflict left a comment

Choose a reason for hiding this comment

Uh oh!

stevenzzzz left a comment

Choose a reason for hiding this comment

Uh oh!

junr03 commented Feb 6, 2020

Uh oh!

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