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.

Sign up for GitHub

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

Jump to bottom

cluster: destroy on main thread #14954

Merged
merged 43 commits into from
Feb 23, 2021
Merged

cluster: destroy on main thread #14954

merged 43 commits into from
Feb 23, 2021

Conversation

lambdai
Copy link
Contributor

@lambdai lambdai commented Feb 5, 2021 •
edited
Loading

Commit Message:
Always destroy cluster info object on the master thread.
Fixed SDS churn and a rare crash case.
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
Fixes:
#13209
istio/istio#30199
istio/istio#28315
[Optional Deprecated:]
[Optional API Considerations:]

lambdai added 15 commits November 18, 2020 21:51
@lambdai
destroy hosts on master
b474d23 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
tobetrypost
7b933b1 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
to tryPost
65ed52e 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
revert hosts guard
df9beaa 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
Merge branch 'master' into completedestroyonmaster
3af5aac 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
fix cluster test
b647d0c 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
fix server fuzz test
8c6266d 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
Merge branch 'main' into clusterdestory
d8d639c 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
cleanup
564b17a 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
remove extra runPostCallbacks() in run
8e380ce 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
remove exit flag in dispatcher
bcad62e 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
rename to movePost returning void, tests failing
50ec141 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
relax ssl manager required empty context
4746d53 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
format
0ca27ec 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
revert accidentally touched files
ec6eb4a 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
Copy link
Contributor Author

lambdai commented Feb 5, 2021 •
edited
Loading

ClusterInfo is not really destroyed because mainthread dispatcher stops dispatching posted tasks. SSLContextManager and symbol tables are not happy: these two managers expect all resources destroyed.

Before this commit the main thread dispatcher will drain the post queue but though, but the exit flag is fragile.

Any advice on how to gracefully shut down dispatcher? @antoniovicente

@mattklein123 mattklein123 self-assigned this Feb 5, 2021
@lambdai
Copy link
Contributor Author

lambdai commented Feb 5, 2021

The tests are failing because symbol tables is not empty when the IsolatedStoreImpl is destroyed.

I am investigating. But it's also good to hear if this is a big issue.

@tbarrella tbarrella mentioned this pull request Feb 5, 2021
Closed
antoniovicente
antoniovicente reviewed Feb 5, 2021
View reviewed changes
Copy link
Contributor

@antoniovicente antoniovicente left a comment

Choose a reason for hiding this comment

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

ClusterInfo is not really destroyed because mainthread dispatcher stops dispatching posted tasks. SSLContextManager and symbol tables are not happy: these two managers expect all resources destroyed.

Before this commit the main thread dispatcher will drain the post queue but though, but the exit flag is fragile.

Any advice on how to gracefully shut down dispatcher? @antoniovicente

Clean shutdown of dispatchers is a fairly involved topic. The envoy dispatcher class hasn't really tried to shoot for clean shutdown. There are some things that could be done to move in that direction like:

  • introducing a bool tryPost(cb) that rejects additional callbacks after shutdown.
  • executing scheduled callbacks as part of the shutdown process
  • force close downstream connections
  • not sure what's relevant to timers

source/common/upstream/upstream_impl.cc Outdated Show resolved Hide resolved
source/common/upstream/upstream_impl.cc Show resolved Hide resolved
@lambdai
add preShutdown
2f26bf4 
Signed-off-by: Yuchen Dai <[email protected]>
@antoniovicente antoniovicente self-assigned this Feb 6, 2021
@lambdai
fix typo
63b063a 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
fixing server tests
75c257a 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
clang-tidy
dd4b151 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
clang-tidy another try
968a611 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
fix format
d4dc8f8 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
Copy link
Contributor Author

lambdai commented Feb 8, 2021

Is tsan error a red herring?
subprocess.CalledProcessError: Command '['git', 'describe', '--all']' returned non-zero exit status 128.

https://dev.azure.com/cncf/envoy/_build/results?buildId=66038&view=logs&j=d1f76054-8f79-554b-6f4a-11d6a63b8e00&t=266e17e3-d213-54b5-deef-0dcee01da137&l=25588

mattklein123
mattklein123 previously approved these changes Feb 23, 2021
View reviewed changes
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!

@lambdai
Copy link
Contributor Author

lambdai commented Feb 23, 2021

fixing format

@lambdai
fix format
d1bb2d5 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai lambdai mentioned this pull request Feb 23, 2021
Merged
@mattklein123 mattklein123 changed the title cluster: destroy on master thread cluster: destroy on main thread Feb 23, 2021
@mattklein123 mattklein123 merged commit 114d5ae into envoyproxy:main Feb 23, 2021
@lambdai
Copy link
Contributor Author

lambdai commented Feb 23, 2021

Thank you!

@lambdai lambdai added the backport/review Request to backport to stable releases label Feb 23, 2021
@lambdai
Copy link
Contributor Author

lambdai commented Feb 23, 2021

@cpakulski The buggy behavior caused lots of churns in istio. See the PR description. Can we backport it?

@cpakulski
Copy link
Contributor

@lambdai Sure. To which releases?

@lambdai
Copy link
Contributor Author

lambdai commented Feb 23, 2021

@lambdai Sure. To which releases?

Thanks! The newly added SdsCdsIntegrationTest doesn't use any recent features. I am afraid all the stable releases are impacted.

@cpakulski
Copy link
Contributor

OK - will port to all 4 latest releases.

@lambdai
Copy link
Contributor Author

lambdai commented Feb 23, 2021

Awesome. Thanks!

@lambdai lambdai mentioned this pull request Feb 23, 2021
Closed
@Shikugawa Shikugawa added backport/approved Approved backports to stable releases and removed backport/review Request to backport to stable releases labels Feb 24, 2021
cpakulski pushed a commit to cpakulski/envoy that referenced this pull request Feb 25, 2021
@lambdai @cpakulski
cluster: destroy on main thread (envoyproxy#14954)
11ce2fe 
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
cpakulski added a commit that referenced this pull request Mar 3, 2021
@cpakulski @KBaichoo @lambdai
backport to 1.17: cluster: destroy on main thread (#14954) (#15197)
b961323 
* Dispatcher: keeps a stack of tracked objects. (#14573)

Dispatcher will now keep a stack of tracked objects; on crash it'll "unwind" and have those objects dump their state. Moreover, it'll invoke fatal actions with the tracked objects. This allows us to dump more information during crash.

See related PR: #14509

Will follow up with another PR dumping information at the codec/parser level.

Signed-off-by: Kevin Baichoo <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>

* cluster: destroy on main thread (#14954)

Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>

* Updated release notes.

Signed-off-by: Christoph Pakulski <[email protected]>

Co-authored-by: Kevin Baichoo <[email protected]>
Co-authored-by: Yuchen Dai <[email protected]>
@lambdai lambdai mentioned this pull request Mar 25, 2021
Closed
This was referenced Apr 23, 2021
Closed
Closed
@danielfoehrKn danielfoehrKn mentioned this pull request May 25, 2021
Closed
@kyessenov kyessenov mentioned this pull request Oct 12, 2022
Closed
@ewoksly
Copy link

ewoksly commented Oct 5, 2023

OK - will port to all 4 latest releases.

@cpakulski could you please confirm starting with version of envoy is this fixed?

@cpakulski
Copy link
Contributor

I do not remember the starting version. It was long long time ago. Please check the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoniovicente antoniovicente antoniovicente left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

@htuch htuch Awaiting requested review from htuch

Assignees

@mattklein123 mattklein123

@antoniovicente antoniovicente

Labels
backport/approved Approved backports to stable releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lambdai @antoniovicente @cpakulski @ewoksly @mattklein123 @Shikugawa