listener: listen socket factory can be cloned from listener draining …

[lambdai]

…filter chain

Commit Message:
Currently the in place updated old listener holds the listen fd during the drain timeout.

If the corresponding active listener is removed and added back, the added back listener creates fresh new passive sockets with new inodes.
The kernel spread the accepted requests to the old listen fd and new listenfd.

The accepted sockets to the old listen fd will be lost. Even somehow the kernel/ebpf requeue these sockets,
these sockets will be handled by envoy after the drain timeout is triggered.

This PR duplicate the listen fd from the old listener. The new listen fd share the same inode.
The newly create listener will consume the accepted sockets if any.

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

Additional Description:
Risk Level:
Testing: Unittest and a bunch of istio e2e tests
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
Fix #18616
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

[lambdai]

/assign @mattklein123

[lambdai]

I would argue that #18677 is also needed.

In the test case, we add the listener 0.0.0.0:8080. It's happy that the requests arrived prior to the add is queued.

However, the add back is pretty much unpredictable.
I would expect stopping the listener 0.0.0.0:8080 means envoy would reject the new request ASAP.

[lambdai]

/retest

[repokitteh-read-only]

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

🐱

Caused by: a #18686 (comment) was created by @lambdai.

see: more, trace.

[mattklein123]

I would argue that #18677 is also needed.

Can you explain this more please? From our lengthy offline discussion I know don't understand how #18677 helps. As long as the sockets are duplicated/cloned, there should be no issue?

[mattklein123]

Thanks, makes sense to me with small comment.

/wait

[mattklein123]

Thanks! We should backport this obviously.

[lambdai]

I would argue that #18677 is also needed.

Can you explain this more please? From our lengthy offline discussion I know don't understand how #18677 helps. As long as the sockets are duplicated/cloned, there should be no issue?

There is no issue if we add a fresh new listener after we stop the listener on that address.

Think about why adding back a fresh listener can duplicate that draining filter chain listener.
It is because the old inplace updated listener doesn't close the listen socket during the drain_timeout after the listener is told to remove.

The state of the system is described here:
https://github.com/envoyproxy/envoy/pull/18686/files#diff-f8e00b71778057a3747a30c4fc3f3ad13e551cadebaf0eca9bdb476b42cadbcaR5112-R5117

[lambdai]

Thanks! We should backport this obviously.

Thank you for the offline guidance and the quick approval!

[lambdai]

/backport

Only need backport to 1.20

[mattklein123]

Think about why adding back a fresh listener can duplicate that draining filter chain listener.
It is because the old inplace updated listener doesn't close the listen socket during the drain_timeout after the listener is told to remove.

Sorry I still don't understand. If you want to continue discussion on the other PR we can talk there, but I don't understand any scenario in which that PR will help. The real fix is cloning if we want to reuse a socket and not drop anything.