hubble/peer: handle burst of change notifications #12024
Merged
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
rolinh
added
release-note/bug
|
test-me-please |
gandro
approved these changes
Jun 11, 2020
There was a problem hiding this comment.
Looks good! One optional nit.
There was also an offline discussion about maybe using a CondVar in the implementation instead of the notify channel trick, but I want to run some benchmarks first before changing things prematurely (especially since this is not on a any critical path).
Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
When a client calls Notify, a change notification is sent for every node in the cluster which allows the client to discover all nodes in the cluster. Subsequently, only changes are notified (when a node is updated or deleted or a new node is added to the cluster). With a sufficiently large cluster, the initial burst of notifications can be massive and the previous implementation did not accommodate for this as it used a default buffer of size 10, assuming it would be large enough to serialize notifications and send them to the client before new notifications are being pushed by the node manager. It appears that the node manager iterates over a hash map while holding a lock when it calls the notification handler and this process is much faster than serializing and sending these notifications over to the remote client via gRPC. However, as the initial buffer implementation used a channel, increasing the buffer size to a very large value would dramatically increase memory usage and most of it would be unused in most cases (in Go, a buffered channel allocates enough memory to hold every element it has be able to buffer). This commit fixes this problem by using a concurrent safe buffer implementation backed by a slice that is only allowed to grow to a predefined maximum size. This size defaults to 65_536 entries, which should be enough to accommodate bursts of notifications for large clusters in cluster-mesh. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
rolinh
force-pushed
the
pr/rolinh/hubble-fix-cn-burst-handling
branch
from
f1abeb0
to
e27ee36
Compare
|
test-me-please |
michi-covalent
approved these changes
Jun 11, 2020
This option has never been made configurable.
Good question! The memory allocation for a channel of this type and size would have been in the order of ~1M (verified using |
|
K8s-1.17-Kernel-4.19 failure is a known flake (IPv4 fragments) |
rolinh
added
the
ready-to-merge
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport done to v1.8
in 1.8.0
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.
When a client calls Notify, a change notification is sent for every node
in the cluster which allows the client to discover all nodes in the
cluster. Subsequently, only changes are notified (when a node is updated
or deleted or a new node is added to the cluster).
With a sufficiently large cluster, the initial burst of notifications
can be massive and the previous implementation did not accommodate for
this as it used a default buffer of size 10, assuming it would be large
enough to serialize notifications and send them to the client before
new notifications are being pushed by the node manager. It appears that
the node manager iterates over a hash map while holding a lock when it
calls the notification handler and this process is much faster than
serializing and sending these notifications over to the remote client
via gRPC.
However, as the initial buffer implementation used a channel, increasing
the buffer size to a very large value would dramatically increase memory
usage and most of it would be unused in most cases (in Go, a buffered
channel allocates enough memory to hold every element it has be able to
buffer).
This PR fixes this problem by using a concurrent safe buffer
implementation backed by a slice that is only allowed to grow to a
predefined maximum size. This size defaults to 65_536 entries, which
should be enough to accommodate bursts of notifications for large
clusters in cluster-mesh.