Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config dump, listeners are added to warming list if workers are not started yet #7511

Merged
merged 2 commits into from
Jul 11, 2019
Merged

Conversation

incfly
Copy link
Contributor

@incfly incfly commented Jul 9, 2019

Signed-off-by: Jianfei Hu jianfeih@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Listeners config dump changed to warming_listeners if the workers are not started.
Risk Level: Low
Testing: Unit test, (existing unit test is good enough, with workers started before and after)
Docs Changes:
Release Notes: Listeners config dump changed to warming_listeners if the workers are not started.
[Optional Fixes #Issue] #6904
[Optional Deprecated:]

Signed-off-by: Jianfei Hu <jianfeih@google.com>
@incfly incfly marked this pull request as ready for review July 9, 2019 21:10
@incfly
Copy link
Contributor Author

incfly commented Jul 9, 2019

/cc @silentdai @asraa @mattklein123 for review.

mattklein123
mattklein123 previously approved these changes Jul 10, 2019
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is low risk and makes sense to me. @asraa @silentdai any concerns?

@lambdai
Copy link
Contributor

lambdai commented Jul 10, 2019

Make perfect sense. Thank you!
/lgtm

@asraa
Copy link
Contributor

asraa commented Jul 10, 2019

This looks good to me.

Mind if you add in
EXPECT_EQ(getListenersConfigDump().dynamic_warming_listeners().size(), 1)
here in ads_integration_test to just note that the pre-server initialization listeners are added as active but dumped as warming?

sendDiscoveryResponse<envoy::api::v2::Listener>(
Config::TypeUrl::get().Listener, {buildListener("listener_0", "route_config_0")},
{buildListener("listener_0", "route_config_0")}, {}, "1");
test_server_->waitForGaugeGe("listener_manager.total_listeners_active", 1);

@mattklein123
Copy link
Member

Waiting on @asraa comment.

/wait

Signed-off-by: Jianfei Hu <jianfeih@google.com>
@@ -969,6 +969,9 @@ TEST_P(AdsIntegrationTest, ListenerDrainBeforeServerStart) {
Config::TypeUrl::get().Listener, {buildListener("listener_0", "route_config_0")},
{buildListener("listener_0", "route_config_0")}, {}, "1");
test_server_->waitForGaugeGe("listener_manager.total_listeners_active", 1);
// Before server is started, even though listeners are added to active list
// we mark them as "warming" in config dump since they're not initialized yet.
EXPECT_EQ(getListenersConfigDump().dynamic_warming_listeners().size(), 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asraa Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@incfly
Copy link
Contributor Author

incfly commented Jul 10, 2019

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #7511 (comment) was created by @incfly.

see: more, trace.

@incfly
Copy link
Contributor Author

incfly commented Jul 10, 2019

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #7511 (comment) was created by @incfly.

see: more, trace.

@incfly
Copy link
Contributor Author

incfly commented Jul 11, 2019

Tests now all passed. @mattklein123 could you take another look or this is ready to go?

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will merge once 1.11 is cut.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants