QUIC hot restart part 6 - child instance pauses listening until parent is drained #31130

ravenblackx · 2023-11-30T18:20:29Z

Commit Message: QUIC hot restart part 6 - child instance pauses listening until parent is drained
Additional Description: Make the child instance not read from the UDP socket until the parent instance stops reading. From the prior changes, the parent instance forwards UDP packets it doesn't recognize to the child instance via a side-channel, so not reading doesn't mean not responding - it just means not intercepting packets which may be intended for the parent instance.

In this change there is no drain optimization - if the parent has no more live connections we will still wait and forward packets until the drain time completes. I think it is most likely not worth optimizing for that, since the eventual goal is to migrate the QUIC-packet-sorting behavior onto eBPF at which point, for systems that care about performance, all of this manual forwarding support will be unused anyway.

I've tried to keep entirely of the performance hot path, so the change only touches constructors and enable/disable operations and stays entirely out of the read path.

It's awkward because the point at which we detect that the socket is being forwarded (the creation of the UdpListenSocket) the UdpListener and ActiveQuicListener don't exist, but those are where we need to disable or enable reading. Also awkward is the potential race - the unpause callback can potentially arrive after the listener was deleted, and must go to the right thread, so there's a bit of a tangle ensuring that that's the case - for this we still call the callback, but abort if the listener was deleted by the time the dispatcher tries to perform the action.

Finally, there's a potential race of disable/enable and this paused state, so I've added enough state to the UdpListener that the sequence start_paused->disable->unpause does not accidentally end with the listener enabled.

Risk Level: Some risk, this is touching quite a bit of core code. The touches should mostly not do anything except to QUIC listeners, so the risk isn't as big as it looks.
Testing: Unit tests provide coverage; I intend to augment hotrestart_handoff test to make sure this is fully doing what it needs to do for QUIC listeners.
Docs Changes:
Release Notes:
Platform Specific Features:

repokitteh-read-only · 2023-11-30T18:20:34Z

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: #31130 was opened by ravenblackx.

see: more, trace.

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ggreenway

I think this is a reasonable approach overall. But I think you'll need a way to un-register for these callbacks, in the case that the quic listener is removed via LDS after the child has started, but before the parent has finished draining.

I'd also like to get at least one more opinion on this, maybe oneof @wbpcode @soulxu @alyssawilk @yanavlasov.

envoy/network/listen_socket.h

ggreenway · 2023-12-12T22:39:54Z

source/common/network/udp_listener_impl.cc

@@ -34,20 +34,49 @@ UdpListenerImpl::UdpListenerImpl(Event::Dispatcher& dispatcher, SocketSharedPtr
    : BaseListenerImpl(dispatcher, std::move(socket)), cb_(cb), time_source_(time_source),
      // Default prefer_gro to false for downstream server traffic.
      config_(config, false) {
+  parent_drained_callback_registry_ =
+      dynamic_cast<const UdpListenSocket&>(*socket_).parentDrainedCallbackRegistry();


Why is a dynamic_cast needed? Can the types be changed so that this isn't needed?

Couldn't upgrade the types because it cascaded back up through multiple layers to eventually interfaces which, in order to be able to use the specified type, caused a circular dependency.
So instead I put a virtual function on the base Socket type, which is yuck, but works cleanly and doesn't cause any kind of dependency cascade.

ggreenway · 2023-12-12T22:43:58Z

envoy/network/listen_socket.h

+   * @param callback the function to call when the listener matching address is
+   *                 drained on the parent instance.
+   */
+  virtual void registerParentDrainedCallback(std::string address,


Why is the address a string instead an address shared_ptr?

Now a (const reference to) an address shared_ptr. It was an ill-considered simplification (it still ends up as a string in the end because we can't make a map keyed on address objects) - stringing it early reduces dependencies and makes it simpler to test, but also reduces how much it makes sense.

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx · 2023-12-13T22:42:08Z

I think this is a reasonable approach overall. But I think you'll need a way to un-register for these callbacks, in the case that the quic listener is removed via LDS after the child has started, but before the parent has finished draining.

That's handled as-is by the callback self-aborting, rather than by unregistering. I don't think unregistering can work reliably because the destructor may not be called on the thread that owns the registry (it could work but would require a network of locks and the behavior would become much less clear if destruction and unpausing are trying to be concurrent - having the callback abort on-thread if the destructor has happened means the "race" occurs exclusively in the worker thread, so there can be no attempted concurrency between destruction and notification).

soulxu · 2023-12-14T09:02:47Z

I think this is a reasonable approach overall. But I think you'll need a way to un-register for these callbacks, in the case that the quic listener is removed via LDS after the child has started, but before the parent has finished draining.

I'd also like to get at least one more opinion on this, maybe oneof @wbpcode @soulxu @alyssawilk @yanavlasov.

Or we only register the thread worker as the callbacks, then we can use the existing disableListeners/enableListeners method (maybe add a new flag to disable all the udp listener) of ConnectionHandler to pause/unpause the listener.

envoy/source/common/listener_manager/connection_handler_impl.h

Lines 56 to 57 in d797293

    
           void disableListeners() override; 
        
           void enableListeners() override;

ravenblackx · 2023-12-26T15:43:28Z

I think this is a reasonable approach overall. But I think you'll need a way to un-register for these callbacks, in the case that the quic listener is removed via LDS after the child has started, but before the parent has finished draining.
I'd also like to get at least one more opinion on this, maybe oneof @wbpcode @soulxu @alyssawilk @yanavlasov.

Or we only register the thread worker as the callbacks, then we can use the existing disableListeners/enableListeners method (maybe add a new flag to disable all the udp listener) of ConnectionHandler to pause/unpause the listener.

The problem with using the existing functions is there are already calls to those functions which could potentially make something racily do the wrong thing, e.g. if a listener starts up and is disabled by a call to disableListeners for hot restart purposes, then is disabled for some other reason, then hot restart draining completes and calls enableListeners, now the socket would be listening despite the other caller of disableListeners expecting it not to be. Tracking it separately ensures that the socket is only reading if both things want it to be.

So my rationale for separating out this paused state is that it can play well with other enable/disable operations with no risk of conflict. Another way to achieve this would be to add a "disabledness" counter so that two disables must be matched to two enables, but knowing whether that would be safe or not would require digging a lot more into existing code to ensure it doesn't currently expect one enable to be capable of overriding two prior disables.

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx · 2024-01-10T22:47:40Z

/coverage

repokitteh-read-only · 2024-01-10T22:47:44Z

Coverage for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/31130/coverage/index.html

The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.

🐱

Caused by: a #31130 (comment) was created by @ravenblackx.

see: more, trace.

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx · 2024-01-16T17:47:23Z

/retest

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ggreenway

Looks good overall!

/wait

ggreenway · 2024-01-22T23:03:55Z

source/common/network/udp_listener_impl.cc

+        socket_->connectionInfoProvider().localAddress(),
+        [this, &dispatcher, alive = std::weak_ptr<void>(destruction_checker_)]() {
+          dispatcher.post([this, alive = std::move(alive)]() {
+            if (alive.lock()) {


I think to be not-racy this needs to be

auto ptr = alive.lock(); if (ptr != nullptr) {

That will guarantee that the pointer can't be deleted while unpause() is running.

It's safe because the destruction happens on the same thread, but changing as suggested because you're right that it otherwise merits a comment explaining this, and it's easier to just make it not matter. :)

source/server/hot_restarting_child.cc

ggreenway · 2024-01-22T23:10:10Z

source/server/hot_restarting_child.cc

+void HotRestartingChild::allDrainsImplicitlyComplete() {
+  for (auto& drain_action : on_drained_actions_) {
+    // call the callback.
+    std::move(drain_action.second)();


What does the std::move do here? Is it needed?

Yeah, this is required for calling an AnyInvocable to do with it being a "call once" type.

source/server/hot_restarting_child.cc

test/common/network/udp_listener_impl_test.cc

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ggreenway

LGTM

ravenblackx · 2024-02-29T17:36:27Z

/retest

…il parent is drained (envoyproxy#31130)" This reverts commit f7352b3. Signed-off-by: Ryan Northey <ryan@synca.io>

#32658) Revert "QUIC hot restart part 6 - child instance pauses listening until parent is drained (#31130)" This reverts commit f7352b3. Signed-off-by: Ryan Northey <ryan@synca.io>

ravenblackx force-pushed the hot_restart_6 branch from 2d2e97b to a156650 Compare November 30, 2023 22:15

WIP

f37e653

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx force-pushed the hot_restart_6 branch from a156650 to f37e653 Compare November 30, 2023 22:20

ravenblackx added 4 commits December 7, 2023 22:00

Merge branch 'main' into hot_restart_6

fbdf5e6

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Try with an interface

3079afa

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Make disable-while-paused still effective

c35a900

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Merge branch 'main' into hot_restart_6

02f8636

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx changed the title ~~Hot restart 6 [WIP]~~ QUIC hot restart 6 [WIP] Dec 12, 2023

ggreenway requested changes Dec 12, 2023

View reviewed changes

rename to ParentDrainedCallbackRegistrar, ditch dynamic_cast

1f75736

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Don't prematurely stringify address

dfdd281

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx requested a review from ggreenway December 26, 2023 21:10

ravenblackx added 3 commits January 4, 2024 19:15

Merge branch 'main' into hot_restart_6

1f658a8

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Add unit tests that not-listening works as expected

1fb258d

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Remove debug prints

e803fa8

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx added 2 commits January 11, 2024 15:14

Spelling

3edc57e

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Add unit tests for parent drained callbacks at the hot restart level

1edfaf4

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx changed the title ~~QUIC hot restart 6 [WIP]~~ QUIC hot restart part 6 - child instance not listening until parent is drained Jan 11, 2024

ravenblackx added 2 commits January 11, 2024 18:22

Merge branch 'main' into hot_restart_6

e3f3f53

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Merge branch 'main' into hot_restart_6

2b15d6d

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx changed the title ~~QUIC hot restart part 6 - child instance not listening until parent is drained~~ QUIC hot restart part 6 - child instance pauses listening until parent is drained Jan 11, 2024

Skip HotRestart tests on Windows

223216d

Signed-off-by: Raven Black <ravenblack@dropbox.com>

GTEST_SKIP syntax is not like many docs say

732ae5a

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx marked this pull request as ready for review January 19, 2024 16:23

ravenblackx assigned ggreenway Jan 19, 2024

ggreenway requested changes Jan 22, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 22, 2024

Only mark as drained on parent terminate; comments

1fa6815

Signed-off-by: Raven Black <ravenblack@dropbox.com>

repokitteh-read-only bot removed the waiting label Feb 20, 2024

ravenblackx added 2 commits February 21, 2024 20:00

Merge branch 'main' into hot_restart_6

5e3f924

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Multiple listeners per address

ca2436e

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ggreenway approved these changes Feb 29, 2024

View reviewed changes

ravenblackx merged commit f7352b3 into envoyproxy:main Feb 29, 2024
52 of 53 checks passed

ravenblackx deleted the hot_restart_6 branch February 29, 2024 17:37

phlax added a commit to phlax/envoy that referenced this pull request Mar 1, 2024

Revert "QUIC hot restart part 6 - child instance pauses listening unt…

b4a5f8b

…il parent is drained (envoyproxy#31130)" This reverts commit f7352b3. Signed-off-by: Ryan Northey <ryan@synca.io>

phlax mentioned this pull request Mar 1, 2024

Revert "QUIC hot restart part 6 - child instance pauses listening unt… #32658

Merged

ravenblackx mentioned this pull request Mar 1, 2024

QUIC hot restart part 6 - child instance pauses listening until parent is drained (second try) #32664

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QUIC hot restart part 6 - child instance pauses listening until parent is drained #31130

QUIC hot restart part 6 - child instance pauses listening until parent is drained #31130

ravenblackx commented Nov 30, 2023 •

edited

Loading

repokitteh-read-only bot commented Nov 30, 2023

ggreenway left a comment

ggreenway Dec 12, 2023

ravenblackx Dec 13, 2023

ggreenway Dec 12, 2023

ravenblackx Dec 26, 2023

ravenblackx commented Dec 13, 2023 •

edited

Loading

soulxu commented Dec 14, 2023 •

edited

Loading

ravenblackx commented Dec 26, 2023

ravenblackx commented Jan 10, 2024

repokitteh-read-only bot commented Jan 10, 2024

ravenblackx commented Jan 16, 2024

ggreenway left a comment

ggreenway Jan 22, 2024

ravenblackx Feb 20, 2024

ggreenway Jan 22, 2024

ravenblackx Feb 20, 2024

ggreenway left a comment

ravenblackx commented Feb 29, 2024

QUIC hot restart part 6 - child instance pauses listening until parent is drained #31130

QUIC hot restart part 6 - child instance pauses listening until parent is drained #31130

Conversation

ravenblackx commented Nov 30, 2023 • edited Loading

repokitteh-read-only bot commented Nov 30, 2023

ggreenway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravenblackx commented Dec 13, 2023 • edited Loading

soulxu commented Dec 14, 2023 • edited Loading

ravenblackx commented Dec 26, 2023

ravenblackx commented Jan 10, 2024

repokitteh-read-only bot commented Jan 10, 2024

ravenblackx commented Jan 16, 2024

ggreenway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway left a comment

Choose a reason for hiding this comment

ravenblackx commented Feb 29, 2024

ravenblackx commented Nov 30, 2023 •

edited

Loading

ravenblackx commented Dec 13, 2023 •

edited

Loading

soulxu commented Dec 14, 2023 •

edited

Loading