xds: share rds configuration among envoy 2/2 by lambdai · Pull Request #9209 · envoyproxy/envoy

lambdai · 2019-12-03T22:27:23Z

Full context is here
The above PR is split into #9008 and this one.

In this PR envoy will maintain only one copy of rds config for each resource name.
Also fixed a the bug of early listener notified ready describe in #8781
The test case is included in this PR.

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

htuch

Looks good, I think it would make sense to have @stevenzzzz also take a pass, thanks.
/wait

test/common/router/rds_impl_test.cc

htuch · 2019-12-05T16:24:14Z

source/common/router/rds_impl.cc

+    std::shared_ptr<RdsRouteConfigProviderImpl> new_provider{
+        new RdsRouteConfigProviderImpl(std::move(subscription), factory_context)};
+    dynamic_route_config_providers_.insert(
+        {manager_identifier, std::weak_ptr<RdsRouteConfigProviderImpl>(new_provider)});


Do we have to hold a strong reference to ensure this doesn't destruct on one of the worker threads?

Subscription holds a strong reference of Provider here:

envoy/source/common/router/rds_impl.cc

Line 218 in fb628ce

subscription_->routeConfigProviders().insert(this);

And map look up will only see two possibility:

no such key in map, no matter worker threads hold a strong reference or not.

map has such key. It means the subscription is not destructed. That subscription hold a strong ref to the provider.

Both possibilities are under the assumption that
subscription exist iff key exists in this dynamic_route_config_providers_ map
I think this assumption is correct because both the CRUD on map and construct/destruct on subscription occurs on main thread.
See Xin's comments 4 lines below.

My above comment could be wrong. I will add some ASSERT

might worth check if the proviider is destructed in main thread instead, with my most recent change to thread_local_impl, a SlotImpl owner is free to be destructed on any thread.

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

lambdai · 2019-12-05T21:33:31Z

I don't have a complete sense of the life time of RdsRouteConfigProviderImpl
But I think this PR is not breaking anything because

This PR unifies the ownership of RdsRouteConfigProvider. Previously a HCM could own a RdsRouteConfigProvider, or own srds config provider which owns shared RdsRouteConfigProvider. In any scenario RdsRouteConfigProvider outlives than the corresponding RdsRouteConfigProvider before this PR
Is the extended life time introduce new issue?
I think the answer is NO. There is no obvious cycle strong reference.
Directly respond to your concerning scenario, let's reason about it in this way: if the shared RdsRouteConfigProvider in this PR could be referenced, I can figure a sequence that RdsRouteConfigProvider, before this PR, is not destructed and is referenced.

Does above make sense?

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

stevenzzzz

LGTM.
As we've discussed, migrating RDS to the new config-provide-framework would revert most of your work here tho.

stevenzzzz · 2019-12-06T04:37:32Z

test/common/router/rds_impl_test.cc

  EXPECT_TRUE(provider2->configInfo().has_value());

+  EXPECT_EQ(provider_.get(), provider2.get())
+      << "fail to obtain the same rds config provider object";


is this for debugging?

The .get() part or the message?
The former: Yes, that's verbose. Will fix.
The latter: goal is to provide more details to debugging a test failure in the future.

test/common/router/rds_impl_test.cc

source/common/router/rds_impl.h

source/common/router/rds_impl.cc

stevenzzzz · 2019-12-06T04:57:42Z

source/common/router/rds_impl.cc

+    std::shared_ptr<RdsRouteConfigProviderImpl> new_provider{
+        new RdsRouteConfigProviderImpl(std::move(subscription), factory_context)};
+    dynamic_route_config_providers_.insert(
+        {manager_identifier, std::weak_ptr<RdsRouteConfigProviderImpl>(new_provider)});


might worth check if the proviider is destructed in main thread instead, with my most recent change to thread_local_impl, a SlotImpl owner is free to be destructed on any thread.

lambdai

Thanks! I will address the small fixes.
And leave the provider frame work in the following PRs

source/common/router/rds_impl.cc

source/common/router/rds_impl.h

lambdai · 2019-12-06T18:46:48Z

test/common/router/rds_impl_test.cc

  EXPECT_TRUE(provider2->configInfo().has_value());

+  EXPECT_EQ(provider_.get(), provider2.get())
+      << "fail to obtain the same rds config provider object";


The .get() part or the message?
The former: Yes, that's verbose. Will fix.
The latter: goal is to provide more details to debugging a test failure in the future.

test/common/router/rds_impl_test.cc

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

lambdai · 2019-12-06T20:05:45Z

@htuch @stevenzzzz
I think I find a hanging bug related to VHDS #9254
I am not going to fix it in this PR.

lambdai · 2019-12-13T19:02:54Z

@stevenzzzz and i don't have further action on this PR.
@htuch Is there any concern from your side?

htuch

Thanks!

Full context is here The above PR is split into envoyproxy#9008 and this one. In this PR envoy will maintain only one copy of rds config for each resource name. Also fixed a the bug of early listener notified ready describe in envoyproxy#8781 The test case is included in this PR. Signed-off-by: Yuchen Dai <silentdai@gmail.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

lambdai added 5 commits December 3, 2019 21:28

shared rds provider on top of server factory context

c09305c

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

fix early declared route ready

ba523c8

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

fix srds test

f1f42e2

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

address comment

4961df3

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

address comment

0ef95a5

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

lambdai requested review from alyssawilk and mattklein123 as code owners December 3, 2019 22:27

fix conflicts

98a5f8e

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

alyssawilk assigned htuch Dec 4, 2019

htuch suggested changes Dec 5, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 5, 2019

htuch requested a review from stevenzzzz December 5, 2019 16:25

htuch assigned stevenzzzz Dec 5, 2019

compress space lines

99c3ab3

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

repokitteh-read-only bot removed the waiting label Dec 5, 2019

BUILD clean up

bf2facb

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

lambdai mentioned this pull request Dec 5, 2019

Proxy 1.4 Crash in HttpConnectionManagerConfig::createFilterChain() envoyproxy/envoy-wasm#334

Closed

verbose assert

47232fc

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

stevenzzzz reviewed Dec 6, 2019

View reviewed changes

lambdai commented Dec 6, 2019

View reviewed changes

lambdai mentioned this pull request Dec 6, 2019

VHDS subscription could refer a destructed init manager #9254

Closed

address Xin's comment

f8b17f0

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

htuch added the waiting label Dec 10, 2019

htuch approved these changes Dec 13, 2019

View reviewed changes

htuch merged commit f2e7c97 into envoyproxy:master Dec 13, 2019

mattklein123 mentioned this pull request Dec 13, 2019

N^2 RDS memory for SNI #7923

Closed

davecheney mentioned this pull request Feb 27, 2020

Observed envoy memory while adding|removing ingress projectcontour/contour#499

Closed

ramaraochavali mentioned this pull request Jul 19, 2022

handle non wildcard resources during reconnect istio/istio#39960

Merged

10 tasks

Conversation

lambdai commented Dec 3, 2019

Uh oh!

htuch left a comment

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!

lambdai commented Dec 5, 2019

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lambdai commented Dec 6, 2019

Uh oh!

lambdai commented Dec 13, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants