envoy: Patch original destination cluster to handle parallel host instantiation #778
+479
−53
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrajahalme
force-pushed
the
envoy-conn-pool-debug
branch
11 times, most recently
from
57d4adb
to
3ed83d0
Compare
jrajahalme
added
bug
jrajahalme
changed the title
sayboras
force-pushed
the
envoy-conn-pool-debug
branch
from
3ed83d0
to
90249c0
Compare
sayboras
approved these changes
Jun 3, 2024
patches/0006-original_dst_cluster-Avoid-multiple-hosts-for-the-sa.patch
Outdated
Show resolved
Hide resolved
patches/0006-original_dst_cluster-Avoid-multiple-hosts-for-the-sa.patch
Outdated
Show resolved
Hide resolved
Patch original destination cluster to avoid multiple hosts for the same address. Connection pool containers use HostSharedPtr as map keys, rather than the address of the host. This leads to multiple connections when there are multiple Host instances for the same address. This is breaking use of the original source address and port for upstream connections since only one such connection can exist at any one time. Original destination cluster implementation creates such duplicate Host instances when two worker threads are racing to create a Host for the same destination at the same time. Fix this by keeping a separate 'updates_map' where each worker places a newly created Host for the original destination. This map is used to look for the Host is it can not be found from the shared read-only 'host_map'. Access to 'updates_map' is syncronized so that it can be safely shared by the worker threads. The main threads consolidates the updates from the 'updates_map' to a new instance of the shared, read-only hosts map, so that the workers do not need to stall for possibly large map updates. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
force-pushed
the
envoy-conn-pool-debug
branch
from
90249c0
to
5cd5589
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Original destination cluster creates Host instances for the destination addresses on-demand. When multiple worker threads need to forward to the same destination at the same time (as is possible with
curl --parallel, for example), separate instances of the destination Host objects are created. Meanwhile, Envoy usesHostSharedPtras a map key in connection pool containers, which leads to multiple different connections being created for the same destination, even from the same worker, as the originally used Host instance is replaced with another, depending on which worker thread gets to add their Host object to the shared cluster state first. Multiple connections to the same destination fail when the original source address and port is being used, as is typically needed for Cilium policy enforcement, due to socket bind failing withcannot bind requested addresserror.This PR fixes this by modifying the original destination cluster implementation to only create one Host object for any destination for any one cluster instance. This requires closer coordination between the worker threads in a form of an additional mutex, on which a write lock is taken whenever a new Host needs to be created. This synchronizes creation of new Hosts between the threads so that a new Host instance is created only if one has not yet been created by any of the worker threads.
Another possible way of fixing this issue would be to replace
HostSharedPtrwith the destination address as a map key, wherever it is used. This strategy could fix related issues with other cluster and/or load balancer types, it they were to exist. Upstream discussion is needed to settle on the proper fix, and we can adapt as needed when that is done.It is yet to be decided if and when this should be backported.