listener: clean up accept filter before creating connection by lambdai · Pull Request #8922 · envoyproxy/envoy

lambdai · 2019-11-07T01:18:19Z

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

Description:
When listener continue to accept connection on listener filter timeout, the OS socket fd could be owned by two separate FileEventImpl.
The destructing FileEventImpl cannot recover the intention of the alive FileEventImpl.

The observation is that EdgeTrigger flag is cleared and the fd will be LevelTriggered and the EPOLLOUT will be activated in each epoll_wait
immediately after the invoke.

Envoy worker thread owning that connecting will be consuming 1 cpu core in the repeated epoll_wait.

This PR is maintaining the immutability that at most one FileEventImpl could owns the fd so the assigned event always match the expectation
of FileEventImpl.

Before this PR

 # registered by tls_inspector
1573028637.322859 epoll_ctl(14, EPOLL_CTL_ADD, 26, {EPOLLIN|EPOLLRDHUP|EPOLLET, {u32=26, u64=26}}) = 0 
# registered by HCM
1573028637.419820 epoll_ctl(14, EPOLL_CTL_MOD, 26, {EPOLLIN|EPOLLOUT|EPOLLRDHUP|EPOLLET, {u32=26, u64=26}}) = 0         
# tls_inspector clean up, EPOLLET is cleared somehow
1573028637.420488 epoll_ctl(14, EPOLL_CTL_MOD, 26, {EPOLLIN|EPOLLOUT, {u32=26, u64=26}}) = 0                                                   

#repeated immediate return of epoll_wait, no write since the http write buffer is empty at the beginning
1573028637.420537 epoll_wait(14, [{EPOLLOUT, {u32=26, u64=26}}], 32, 24) = 1

With this PR

# registered by tls_inspector
1573030976.461595 epoll_ctl(14, EPOLL_CTL_ADD, 26, {EPOLLIN|EPOLLRDHUP|EPOLLET, {u32=26, u64=26}}) = 0 
# tls_inspector clean up  
1573030976.562546 epoll_ctl(14, EPOLL_CTL_DEL, 26, 0x7f1d914cd0a0) = 0                                                                                          
# registered by HCM, EPOLLET show up and leave there forever
1573030976.562986 epoll_ctl(14, EPOLL_CTL_ADD, 26, {EPOLLIN|EPOLLOUT|EPOLLET, {u32=26, u64=26}}) = 0

The proof that EPOLLIN|EPOLLOUT w/o EPOLLET is registered due to ~FileEventImpl
Below fd 21 op 3 event_uaddr 5 is recognized as epoll_ctl( ..., EPOLL_MOD, 21, { EPOLLIN|EPOLLOUT, {...}}) where EPOLLET (1 << 31) is missing.
fd 21 is the English spelling of 26 since I capture the logs in different run.

fd 21 op 3 event_uaddr 5 0x7f443e11d24a : epoll_ctl 0xa/0x30 [/lib/x86_64-linux-gnu/libc-2.27.so]
 0x562790a619b2 : epoll_apply_one_change 0x172/0x570 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x562790a61356 : epoll_nochangelist_del 0xa6/0xb0 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x562790a53b1c : evmap_io_del_ 0x34c/0x3e0 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x562790a48d15 : event_del_nolock_ 0x2d5/0x490 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x562790a4e60c : event_del_ 0xac/0x100 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x562790a4cc2a : event_del 0x1a/0x20 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x56279038c145 : Envoy::Event::ImplBase::~ImplBase() 0x15/0x30 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x562790366047 : Envoy::Event::FileEventImpl::~FileEventImpl() 0x47/0x60 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x56279036607c : Envoy::Event::FileEventImpl::~FileEventImpl() 0x1c/0x30 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x56278f7b8a5f : std::default_delete<Envoy::Event::FileEvent>::operator()(Envoy::Event::FileEvent*) const 0x2f/0x40 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x56278f7b83a3 : std::unique_ptr<Envoy::Event::FileEvent, std::default_delete<Envoy::Event::FileEvent> >::~unique_ptr() 0x53/0x80 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x56278f7d0187 : Envoy::Extensions::ListenerFilters::TlsInspector::Filter::~Filter() 0x47/0x70 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x56278f7d01cc : Envoy::Extensions::ListenerFilters::TlsInspector::Filter::~Filter() 0x1c/0x30 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x56278f7b23bf : std::default_delete<Envoy::Network::ListenerFilter>::operator()(Envoy::Network::ListenerFilter*) const 0x2f/0x40 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x56278f7b1cd3 : std::unique_ptr<Envoy::Network::ListenerFilter, std::default_delete<Envoy::Network::ListenerFilter> >::~unique_ptr() 0x53/0x80 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x562790350879 : void __gnu_cxx::new_allocator<std::_List_node<std::unique_ptr<Envoy::Network::ListenerFilter, std::default_delete<Envoy::Network::ListenerFilter> > > >::destroy<std::unique_ptr<Envoy::Network::ListenerFilter, std::default_delete<Envoy::Network::ListenerFilter> > >(std::unique_ptr<Envoy::Network::ListenerFilter, std::default_delete<Envoy::Network::ListenerFilter> >*) 0x19/0x20 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x5627903507d0 : void std::allocator_traits<std::allocator<std::_List_node<std::unique_ptr<Envoy::Network::ListenerFilter, std::default_delete<Envoy::Network::ListenerFilter> > > > >::destroy<std::unique_ptr<Envoy::Network::ListenerFilter, std::default_delete<Envoy::Network::ListenerFilter> > >(std::allocator<std::_List_node<std::unique_ptr<Envoy::Network::ListenerFilter, std::default_delete<Envoy::Network::ListenerFilter> > > >&, std::unique_ptr<Envoy::Network::ListenerFilter, std::default_delete<Envoy::Network::ListenerFilter> >*) 0x20/0x30 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x56279035072e : std::__cxx11::_List_base<std::unique_ptr<Envoy::Network::ListenerFilter, std::default_delete<Envoy::Network::ListenerFilter> >, std::allocator<std::unique_ptr<Envoy::Network::ListenerFilter, std::default_delete<Envoy::Network::ListenerFilter> > > >::_M_clear() 0x6e/0xa0 [/home/lambdai/workspace/tls/envoymaster1106.dbg]
 0x5627903505cf : std::__cxx11::list<std::unique_ptr<Envoy::Network::ListenerFilter, std::default_delete<Envoy::Network::ListenerFilter> >, std::allocator<std::unique_ptr<Envoy::Network::ListenerFilter, std::default_delete<Envoy::Network::ListenerFilter> > > >::clear() 0x1f/0x40 [/home/lambdai/workspace/tls/envoymaster1106.dbg]

Risk Level:
Testing:
Docs Changes:
Release Notes:
Fix istio/istio#18229

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

mattklein123

Thanks for digging into this. I realize the urgency around this so a TODO/issue is fine for follow up, but optimally we would have some integration test that covers this, or at minimum some unit test. Can you try to add something to this PR or if not TODO that also? Thanks.

/wait

source/server/connection_handler_impl.cc

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

lambdai · 2019-11-07T22:40:14Z

test/mocks/network/mocks.cc

-MockListenerFilter::MockListenerFilter() = default;
-MockListenerFilter::~MockListenerFilter() = default;
+MockListenerFilter::MockListenerFilter() {
+  // ON_CALL(*this, die_()).WillByDefault(testing::Invoke([](){}));


I don't really know why ON_CALL + NiceMock reports uninterested call.
I add explicit EXPECT_CALL in each usage. That's a tech debt.
I will investigate later

It doesn't work in the destructor of a mock. We do this all over the place. Please delete this comment.

Sad...During the constructor or destructor of MockFoo, the mock object is not nice or strict

mattklein123

LGTM with small nits, thanks.

/wait

source/server/connection_handler_impl.cc

mattklein123 · 2019-11-07T22:55:17Z

test/mocks/network/mocks.cc

-MockListenerFilter::MockListenerFilter() = default;
-MockListenerFilter::~MockListenerFilter() = default;
+MockListenerFilter::MockListenerFilter() {
+  // ON_CALL(*this, die_()).WillByDefault(testing::Invoke([](){}));


It doesn't work in the destructor of a mock. We do this all over the place. Please delete this comment.

mattklein123 · 2019-11-07T22:55:31Z

test/mocks/network/mocks.h

  MockListenerFilter();
  ~MockListenerFilter() override;

+  MOCK_METHOD0(die_, void());


s/die_/destroy which is what we typically name this method

Updating...
Looks like destroy_ w/ underscore is preferred since it is added in MockClass

test/server/connection_handler_test.cc

lambdai · 2019-11-07T23:05:16Z

Thanks! Running test locally. Update in 5 minutes

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

mattklein123

LGTM, thanks

Signed-off-by: Yuchen Dai <silentdai@gmail.com> Signed-off-by: Lizan Zhou <lizan@tetrate.io>

listener: clean up accept filter before creating connection

2ff37ee

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

mattklein123 requested changes Nov 7, 2019

View reviewed changes

source/server/connection_handler_impl.cc Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Nov 7, 2019

mattklein123 assigned lizan, PiotrSikora and mattklein123 Nov 7, 2019

This was referenced Nov 7, 2019

Protecting envoy from stacking events on the same fd #8924

Open

Protecting envoy from stacking events on the same fd #8925

Open

address comment

0012845

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

repokitteh-read-only bot removed the waiting label Nov 7, 2019

lambdai requested a review from mattklein123 November 7, 2019 03:04

mattklein123 added the waiting:any label Nov 7, 2019

repokitteh-read-only bot removed the waiting:any label Nov 7, 2019

lambdai mentioned this pull request Nov 7, 2019

[1.4] listener: clean up accept filters before connection creation istio/envoy#119

Merged

lambdai added 2 commits November 7, 2019 20:05

add unit test

354ad99

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

add unit test

8523a5f

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

lambdai commented Nov 7, 2019

View reviewed changes

mattklein123 requested changes Nov 7, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 7, 2019

fix times, naming and comments

ab41193

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

repokitteh-read-only bot removed the waiting label Nov 7, 2019

lambdai requested a review from mattklein123 November 7, 2019 23:08

lizan approved these changes Nov 7, 2019

View reviewed changes

mattklein123 approved these changes Nov 7, 2019

View reviewed changes

mattklein123 merged commit c8de199 into envoyproxy:master Nov 8, 2019

lizan pushed a commit that referenced this pull request Nov 9, 2019

listener: clean up accept filter before creating connection (#8922)

0cd355a

Signed-off-by: Yuchen Dai <silentdai@gmail.com> Signed-off-by: Lizan Zhou <lizan@tetrate.io>

soulxu mentioned this pull request Jun 12, 2021

envoy still try to create file event twice on the same fd when continue_on_listener_filters_timeout is true #16951

Closed

This was referenced Jun 15, 2021

Listener: reset the file event when destroying listener filters #16952

Merged

Pilot-agent in high CPU after an upgrade to 1.8.4 istio/istio#32822

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

listener: clean up accept filter before creating connection#8922

listener: clean up accept filter before creating connection#8922
mattklein123 merged 5 commits intoenvoyproxy:masterfrom
lambdai:ADfix

lambdai commented Nov 7, 2019

Uh oh!

mattklein123 left a comment

Uh oh!

Uh oh!

lambdai Nov 7, 2019

Uh oh!

mattklein123 Nov 7, 2019

Uh oh!

lambdai Nov 7, 2019

Uh oh!

mattklein123 left a comment

Uh oh!

Uh oh!

mattklein123 Nov 7, 2019

Uh oh!

mattklein123 Nov 7, 2019

Uh oh!

lambdai Nov 7, 2019

Uh oh!

Uh oh!

lambdai commented Nov 7, 2019

Uh oh!

mattklein123 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

lambdai commented Nov 7, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lambdai Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

lambdai Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

lambdai Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lambdai commented Nov 7, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants