ecds: rationalize ownership #19630

kyessenov · 2022-01-20T07:09:06Z

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

Commit Message: Rationalize ownership of config providers and subscriptions.
Additional Description: This should fix issue #14930.

The root cause is unclear ownership of config between listeners and subscriptions (which are shared and operate separately).
The bug is triggered when multiple churning listeners reference the same ECDS resource, while at the same time ECDS pushes updates to it.

This PR fixes most issues discovered:

Ensure that the config subscription only references the server factory context, not the listener context, as the listener context may go away while the subscription is active.
Ensure that the asynchronous onConfigUpdate call on a worker does not reference the dynamic config provider as it is owned by a listener and may go away after onConfigUpdate but before all workers process the event. The solution is to create a shared pointer intermediary that can outlast the dynamic config provider.
Ensure that the filter factory is always deleted on main. Regular thread local destructor does not guarantee that. The solution is to issue an empty update to TLS during the destructor. This requires exposing the isShutdown accessor to avoid an assertion failure during process shutdown.
Move the terminal filter detection to the provider from the subscriptiion. This is because the terminal filter condition on a factory depends on the listener context (unlikely to be used, but is a public filter interface).

Risk Level: medium
Testing: black box integration test over long time with racey xDS, unit, added integration test
Docs Changes: none
Release Notes: none

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

kyessenov · 2022-01-20T07:10:51Z

/cc @tbarrella @bianpengyuan

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

lambdai · 2022-01-20T19:12:04Z

Several high level questions

Ensure that the config subscription only references the server factory context, not the listener context, as the listener context may go away while the subscription is active.

From the perspective of lifetime, this is the right move. It's a interface change to the component which may use context. I guess if the compile succeeds it should be ok? There is some stats scope change along with the context. Perhaps it should be reflected in the release log.

Ensure that the filter factory is always deleted on main. Regular thread local destructor does not guarantee that. The solution is to issue an empty update to TLS during the destructor. This requires exposing the isShutdown accessor to avoid an assertion failure during process shutdown.

isShutdown seems right in this case. However, there is another corner case that is unrelated to shutdown:
The TLS can be destroyed before the lambda that executing on each worker thread. Is the filter factory deleted on the main thread in this case?

kyessenov · 2022-01-20T19:28:26Z

From the perspective of lifetime, this is the right move. It's a interface change to the component which may use context. I guess if the compile succeeds it should be ok? There is some stats scope change along with the context. Perhaps it should be reflected in the release log.

Would it be fair to add that ECDS stats now bind to server scope so are not cleared when listeners are removed?

isShutdown seems right in this case. However, there is another corner case that is unrelated to shutdown: The TLS can be destroyed before the lambda that executing on each worker thread. Is the filter factory deleted on the main thread in this case?

runOnAllThreads ensures that the last call is always on main by posting to main from last deletion of shared pointer on a worker. Because we pass the main config as part of the call, it shouldn't matter whether backing TLS is gone?

lambdai · 2022-01-20T19:38:26Z

Would it be fair to add that ECDS stats now bind to server scope so are not cleared when listeners are removed?

I don't have the answer.
Server scope is easier to implemented correctly.
Per listener scope is probably what user wants to see.
I don't understand which is chosen in this PR right now...

kyessenov · 2022-01-20T19:51:53Z

Would it be fair to add that ECDS stats now bind to server scope so are not cleared when listeners are removed?

I don't have the answer. Server scope is easier to implemented correctly. Per listener scope is probably what user wants to see. I don't understand which is chosen in this PR right now...

Scopes are derived from server factory context in this PR, so it's not tied to listener one. However, we still carry on stats_prefix from the listener even when listeners for the subscription change. I suppose we can change that so that the stat prefix for ECDS is completely decoupled from listeners.

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

lizan · 2022-01-20T20:35:57Z

/assign-from @envoyproxy/first-pass-reviewers

repokitteh-read-only · 2022-01-20T20:36:00Z

@envoyproxy/first-pass-reviewers assignee is @adisuissa

🐱

Caused by: a #19630 (comment) was created by @lizan.

see: more, trace.

lambdai · 2022-01-20T21:46:47Z

isShutdown seems right in this case. However, there is another corner case that is unrelated to shutdown: The TLS can be destroyed before the lambda that executing on each worker thread. Is the filter factory deleted on the main thread in this case?

runOnAllThreads ensures that the last call is always on main by posting to main from last deletion of shared pointer on a worker. Because we pass the main config as part of the call, it shouldn't matter whether backing TLS is gone?

You are right. The on-main-thread callback is not associated with the slot. Sorry for the churn

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

mattklein123 · 2022-01-26T16:41:27Z

@adisuissa can you review this one please?

adisuissa · 2022-01-27T05:05:24Z

Thanks for working on this @kyessenov!
I'm still looking through the changes and trying to get a better understanding of the subtleties of ECDS, and will give my comments tomorrow.

BTW: I think that this PR also fixes #19628.

kyessenov · 2022-01-31T17:44:44Z

Gentle ping for a review. This is destabilizing Wasm APIs in Istio.

adisuissa

Thanks for fixing this!
Overall I agree with the change, but I have a question regarding the shared pointer.

adisuissa · 2022-02-07T19:45:13Z

source/common/filter/config_discovery_impl.cc

@@ -160,8 +151,7 @@ void FilterConfigSubscription::onConfigUpdate(
 last_config_hash_ = 0;
 last_config_ = nullptr;
 last_type_url_ = "";
- last_filter_is_terminal_ = false;
- last_filter_name_ = "";
+ last_factory_name_ = "";


nit: add last_version_info_ = "";

adisuissa · 2022-02-08T02:22:14Z

test/integration/extension_discovery_integration_test.cc

+ ASSERT_TRUE(response->complete());
+ EXPECT_EQ("200", response->headers().getStatusValue());
+ }
+}


nit: codec_client_->close();

adisuissa · 2022-02-08T03:27:48Z

source/common/filter/config_discovery_impl.h

+
+ const std::string stat_prefix_;
+ Server::Configuration::FactoryContext& factory_context_;
+ std::shared_ptr<MainConfig> main_config_;


I'm not certain that shared_ptr makes a difference in this case.
IIUC the only update is on the pointed object (current_config_), which would be overrun whether this is a shared pointer or not.

During destruction, main_config is shared with the runOnAllThreads callbacks which guarantees that the last reference is deleted on the main thread. That ensures that the inner config is always deleted on the main thread, which is necessary if the inner config has its own TLS.

I see, thanks for clarification.

adisuissa · 2022-02-08T03:29:27Z

test/integration/extension_discovery_integration_test.cc

@@ -731,5 +731,59 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailTerminalFilterNotAtEndOfFilte
 EXPECT_EQ("500", response->headers().getStatusValue());
 }

+// Validate that deleting listeners does not break active ECDS subscription.
+TEST_P(ExtensionDiscoveryIntegrationTest, ReloadBoth) {


I understand that this is not part of this PR, but I think that the current integration test doesn't exercise the onConfigRemoved path because the test doesn't use delta-xDS.

Ack, let's do this separately since this looks like some existing debt.

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

adisuissa

LGTM.
/assign-from @envoyproxy/senior-maintainers

adisuissa · 2022-02-08T15:06:42Z

source/common/filter/config_discovery_impl.h

+
+ const std::string stat_prefix_;
+ Server::Configuration::FactoryContext& factory_context_;
+ std::shared_ptr<MainConfig> main_config_;


I see, thanks for clarification.

repokitteh-read-only · 2022-02-08T15:07:16Z

@envoyproxy/senior-maintainers assignee is @snowp

🐱

Caused by: a #19630 (review) was submitted by @adisuissa.

see: more, trace.

kyessenov · 2022-02-08T16:54:04Z

/retest
(timeout)

repokitteh-read-only · 2022-02-08T16:54:08Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19630 (comment) was created by @kyessenov.

see: more, trace.

kyessenov · 2022-02-14T17:24:56Z

Another gentle ping for a crash fix.

snowp

Thanks!

- Update `ENVOY_COMMIT` to the latest Envoy's commit envoyproxy/envoy#19630 - Sync the `.bazelrc` to the current Envoy's version. - Update pool stream interface according to envoyproxy/envoy#19388 using reasonable defaults. H3 early send feature maybe adopted in the future, but is not in the scope of this PR. - Added a hack to prevent integration tests from failing due to a non-empty error log. logging introduced in envoyproxy/envoy@027d6ca appears to be purely informational and should be fixed upstream envoyproxy/envoy#19964 Signed-off-by: tomjzzhang <4367421+tomjzzhang@users.noreply.github.com>

fix ecds

430ffc2

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

fix tests

491f900

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

kyessenov changed the title ~~ecds: rationalize ownership (WIP)~~ ecds: rationalize ownership Jan 20, 2022

add integration

2e6864d

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

kyessenov mentioned this pull request Jan 20, 2022

ECDS stats reuse stale listener stat prefix #19638

Closed

typo

6c80e8c

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

repokitteh-read-only bot assigned adisuissa Jan 20, 2022

kyessenov added 2 commits January 20, 2022 14:02

fix macos

2154844

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

fix macos

bc235de

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

adisuissa reviewed Feb 8, 2022

View reviewed changes

kyessenov added 3 commits February 7, 2022 21:18

Merge remote-tracking branch 'upstream/main' into ecds_fix2

c21bbcb

review

4fb4084

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

add comment

efcba0d

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

adisuissa approved these changes Feb 8, 2022

View reviewed changes

repokitteh-read-only bot assigned snowp Feb 8, 2022

snowp approved these changes Feb 14, 2022

View reviewed changes

snowp merged commit cc089aa into envoyproxy:main Feb 14, 2022

tomjzzhang mentioned this pull request Feb 15, 2022

Update Envoy to cc089aa (Feb 14th 2022) envoyproxy/nighthawk#806

Merged

huanghuangzym mentioned this pull request Jul 1, 2022

Segfault from ECDS update #14930

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ecds: rationalize ownership #19630

ecds: rationalize ownership #19630

kyessenov commented Jan 20, 2022 •

edited

Loading

kyessenov commented Jan 20, 2022

lambdai commented Jan 20, 2022

kyessenov commented Jan 20, 2022

lambdai commented Jan 20, 2022

kyessenov commented Jan 20, 2022 •

edited

Loading

lizan commented Jan 20, 2022

repokitteh-read-only bot commented Jan 20, 2022

lambdai commented Jan 20, 2022

mattklein123 commented Jan 26, 2022

adisuissa commented Jan 27, 2022

kyessenov commented Jan 31, 2022

adisuissa left a comment

adisuissa Feb 7, 2022

kyessenov Feb 8, 2022

adisuissa Feb 8, 2022

kyessenov Feb 8, 2022

adisuissa Feb 8, 2022

kyessenov Feb 8, 2022

adisuissa Feb 8, 2022

adisuissa Feb 8, 2022

kyessenov Feb 8, 2022

adisuissa left a comment

adisuissa Feb 8, 2022

repokitteh-read-only bot commented Feb 8, 2022

kyessenov commented Feb 8, 2022

repokitteh-read-only bot commented Feb 8, 2022

kyessenov commented Feb 14, 2022

snowp left a comment

ecds: rationalize ownership #19630

ecds: rationalize ownership #19630

Conversation

kyessenov commented Jan 20, 2022 • edited Loading

kyessenov commented Jan 20, 2022

lambdai commented Jan 20, 2022

kyessenov commented Jan 20, 2022

lambdai commented Jan 20, 2022

kyessenov commented Jan 20, 2022 • edited Loading

lizan commented Jan 20, 2022

repokitteh-read-only bot commented Jan 20, 2022

lambdai commented Jan 20, 2022

mattklein123 commented Jan 26, 2022

adisuissa commented Jan 27, 2022

kyessenov commented Jan 31, 2022

adisuissa 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

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Feb 8, 2022

kyessenov commented Feb 8, 2022

repokitteh-read-only bot commented Feb 8, 2022

kyessenov commented Feb 14, 2022

snowp left a comment

Choose a reason for hiding this comment

kyessenov commented Jan 20, 2022 •

edited

Loading

kyessenov commented Jan 20, 2022 •

edited

Loading