envoy: removing srds from E-M by alyssawilk · Pull Request #33192 · envoyproxy/envoy

alyssawilk · 2024-03-28T15:21:53Z

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2024-03-28T15:21:58Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #33192 was opened by alyssawilk.

see: more, trace.

alyssawilk · 2024-04-05T14:58:58Z

/retest

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

adisuissa

Overall LGTM, thanks!
I don't think we have technical release notes, but if there's something like that, should notify SRDS users that they need to link the "//source/common/router:scoped_rds_lib" or otherwise it will fail during runtime.

adisuissa · 2024-04-23T14:31:37Z

+  auto srds_factory =
+      Envoy::Config::Utility::getFactoryByName<Router::SrdsFactory>(srds_factory_name);


nit: move to the kScopedRoutes switch case.

mind if I leave here? it's a pretty cheap function for the non-srds case and if you declare variables in a switch you need braces and it screws up coverage.

adisuissa · 2024-04-23T14:37:01Z

 * that must hold a reference/pointer to them.
 */
-class ConfigProviderManager {
+class ConfigProviderManager : public Singleton::Instance {


I can see why one would like to have a ConfigProviderManager as singleton, but it is unclear to me what triggered the change moving the Singleton inheritance from the implementation to the interface?
(I'm assuming this is something related to C++'s unique_ptr not aligned well with inheritance, but just want to make sure this change is needed).

yeah the implementation code which now only knows it has the interface but not the impl needs to know it is holding a singleton

adisuissa

LGTM, thanks!

srds: removing hard link in binary Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

…43392) ### Description: PR #33192 refactored SRDS to use a `SrdsFactory` interface for decoupling, but inadvertently changed the `init_manager` passed to `ScopedRoutesConfigProviderUtil::create()` from the listener-level init_manager to the server-level init_manager. When a listener with SRDS arrives via LDS after the server init_manager has already reached the `Initialized` state, the SRDS init target is silently discarded in release builds, and the SRDS subscription never starts. The fix adds an `Init::Manager&` parameter to `SrdsFactory::createConfigProvider()` and passes the listener's init_manager from the HCM config constructor, restoring the correct behavior that existed before PR #33192 and matching how RDS handles init_manager propagation. AI-assisted: Claude opus-4.6 was used to help write the unit test and PR description. The fix itself and root cause analysis were done manually. ### Risk Level: Low ### Testing: - `//test/common/router:scoped_rds_test` PASSED - `//test/extensions/filters/network/http_connection_manager:config_test` PASSED - `//test/integration:scoped_rds_integration_test` PASSED - Higress kind cluster integration test with Delta xDS + SRDS PASSED ### Docs Changes: N/A ### Release Notes: N/A ### Platform Specific Features: N/A --------- Signed-off-by: jingze <daijingze.djz@alibaba-inc.com> Signed-off-by: Jingze <52855280+Jing-ze@users.noreply.github.com>

…nvoyproxy#43392) ### Description: PR envoyproxy#33192 refactored SRDS to use a `SrdsFactory` interface for decoupling, but inadvertently changed the `init_manager` passed to `ScopedRoutesConfigProviderUtil::create()` from the listener-level init_manager to the server-level init_manager. When a listener with SRDS arrives via LDS after the server init_manager has already reached the `Initialized` state, the SRDS init target is silently discarded in release builds, and the SRDS subscription never starts. The fix adds an `Init::Manager&` parameter to `SrdsFactory::createConfigProvider()` and passes the listener's init_manager from the HCM config constructor, restoring the correct behavior that existed before PR envoyproxy#33192 and matching how RDS handles init_manager propagation. AI-assisted: Claude opus-4.6 was used to help write the unit test and PR description. The fix itself and root cause analysis were done manually. ### Risk Level: Low ### Testing: - `//test/common/router:scoped_rds_test` PASSED - `//test/extensions/filters/network/http_connection_manager:config_test` PASSED - `//test/integration:scoped_rds_integration_test` PASSED - Higress kind cluster integration test with Delta xDS + SRDS PASSED ### Docs Changes: N/A ### Release Notes: N/A ### Platform Specific Features: N/A --------- Signed-off-by: jingze <daijingze.djz@alibaba-inc.com> Signed-off-by: Jingze <52855280+Jing-ze@users.noreply.github.com> Signed-off-by: bjmask <11672696+bjmask@users.noreply.github.com>

…nvoyproxy#43392) ### Description: PR envoyproxy#33192 refactored SRDS to use a `SrdsFactory` interface for decoupling, but inadvertently changed the `init_manager` passed to `ScopedRoutesConfigProviderUtil::create()` from the listener-level init_manager to the server-level init_manager. When a listener with SRDS arrives via LDS after the server init_manager has already reached the `Initialized` state, the SRDS init target is silently discarded in release builds, and the SRDS subscription never starts. The fix adds an `Init::Manager&` parameter to `SrdsFactory::createConfigProvider()` and passes the listener's init_manager from the HCM config constructor, restoring the correct behavior that existed before PR envoyproxy#33192 and matching how RDS handles init_manager propagation. AI-assisted: Claude opus-4.6 was used to help write the unit test and PR description. The fix itself and root cause analysis were done manually. ### Risk Level: Low ### Testing: - `//test/common/router:scoped_rds_test` PASSED - `//test/extensions/filters/network/http_connection_manager:config_test` PASSED - `//test/integration:scoped_rds_integration_test` PASSED - Higress kind cluster integration test with Delta xDS + SRDS PASSED ### Docs Changes: N/A ### Release Notes: N/A ### Platform Specific Features: N/A --------- Signed-off-by: jingze <daijingze.djz@alibaba-inc.com> Signed-off-by: Jingze <52855280+Jing-ze@users.noreply.github.com>

alyssawilk force-pushed the rds2 branch 4 times, most recently from cdd1727 to d453464 Compare April 2, 2024 17:32

alyssawilk force-pushed the rds2 branch 2 times, most recently from 3a2fcff to 31dd785 Compare April 4, 2024 21:00

alyssawilk force-pushed the rds2 branch 3 times, most recently from a8a6d56 to a5386f4 Compare April 10, 2024 20:06

srds: removing hard link in binary

96da3dc

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the rds2 branch from a5386f4 to 96da3dc Compare April 22, 2024 12:38

alyssawilk assigned adisuissa Apr 22, 2024

alyssawilk marked this pull request as ready for review April 22, 2024 14:57

alyssawilk requested review from RyanTheOptimist, abeyad, fredyw and mattklein123 as code owners April 22, 2024 14:57

adisuissa reviewed Apr 23, 2024

View reviewed changes

alyssawilk assigned zuercher Apr 23, 2024

adisuissa approved these changes Apr 23, 2024

View reviewed changes

zuercher approved these changes Apr 25, 2024

View reviewed changes

alyssawilk merged commit f9c1457 into envoyproxy:main Apr 29, 2024

alyssawilk added a commit to alyssawilk/envoy that referenced this pull request Apr 29, 2024

envoy: removing srds from E-M (envoyproxy#33192)

45ed3b0

srds: removing hard link in binary Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk deleted the rds2 branch June 25, 2024 14:50

Jing-ze mentioned this pull request Feb 9, 2026

srds: fix init_manager to use listener scope instead of server scope #43392

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

envoy: removing srds from E-M#33192

envoy: removing srds from E-M#33192
alyssawilk merged 1 commit into
envoyproxy:mainfrom
alyssawilk:rds2

alyssawilk commented Mar 28, 2024

Uh oh!

repokitteh-read-only Bot commented Mar 28, 2024

Uh oh!

alyssawilk commented Apr 5, 2024

Uh oh!

adisuissa left a comment

Uh oh!

adisuissa Apr 23, 2024

Uh oh!

alyssawilk Apr 23, 2024

Uh oh!

adisuissa Apr 23, 2024

Uh oh!

alyssawilk Apr 23, 2024

Uh oh!

adisuissa left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		auto srds_factory =
		Envoy::Config::Utility::getFactoryByName<Router::SrdsFactory>(srds_factory_name);

Conversation

alyssawilk commented Mar 28, 2024

Uh oh!

repokitteh-read-only Bot commented Mar 28, 2024

Uh oh!

alyssawilk commented Apr 5, 2024

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

adisuissa Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

alyssawilk Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

adisuissa Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

alyssawilk Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants