libnetwork/networkdb: prioritize local table broadcasts over event rebroadcasts #50193
Conversation
This file contains hidden or 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
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ce52ebc to
d830219
Compare
robmry
reviewed
Jun 13, 2025
78e32e9 to
9549022
Compare
The map key for nDB.networks is the network ID. The struct field is not actually used anywhere in practice. Signed-off-by: Cory Snider <csnider@mirantis.com>
When joining a network that was previously joined but not yet reaped, NetworkDB replaces the network struct value with a zeroed-out one with the entries count copied over. This is also the case when joining a network that is currently joined! Consequently, joining a network has the side effect of clearing the broadcast queue. If the queue is cleared while messages are still pending broadcast, convergence may be delayed until the next bulk sync cycle. Make it an error to join a network twice without leaving. Retain the existing broadcast queue when rejoining a network that has not yet been reaped. Signed-off-by: Cory Snider <csnider@mirantis.com>
NetworkDB uses a muli-dimensional map of struct network to keep track of network attachments for both remote nodes and the local node. Only a subset of the struct fields are used for remote nodes' network attachments. The tableBroadcasts pointer field in particular is always initialized for network values representing local attachments (read: nDB.networks[nDB.config.NodeID]) and always nil for remote attachments. Consequently, unnecessary defensive nil-pointer checks are peppered throughout the code despite the aforementioned invariant. Enshrine the invariant that tableBroadcasts is initialized iff the network attachment is for the local node in the type system. Pare down struct network to only the fields needed for remote network attachments and move the local-only fields into a new struct thisNodeNetwork. Elide the unnecessary nil-checks. Signed-off-by: Cory Snider <csnider@mirantis.com>
Log more details when assertions fail to provide a more complete picture of what went wrong when TestCRUDTableEntries fails. Log the state of each NetworkDB instance at various points in TestCRUDTableEntries to provide an even more complete picture. Increase the global logger verbosity in tests so warnings and debug logs are printed to the test log. Signed-off-by: Cory Snider <csnider@mirantis.com>
A network node is responsible for both broadcasting table events for entries it owns and for rebroadcasting table events from other nodes it has received. Table events to be broadcast are added to a single queue per network, including events for rebroadcasting. As the memberlist TransmitLimitedQueue is (to a first approximation) LIFO, a flood of events from other nodes could delay the broadcasting of locally-generated events indefinitely. Prioritize broadcasting local events by splitting up the queues and only pulling from the rebroadcast queue if there is free space in the gossip packet after draining the local-broadcast queue. Signed-off-by: Cory Snider <csnider@mirantis.com>
9549022 to
6ec6e09
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR improves the convergence rate of NetworkDB by prioritizing local event broadcasts over remote rebroadcasts during gossip cycles. Key changes include:
- Refactoring local network attachments to use a new thisNodeNetworks structure.
- Splitting broadcast queues into local tableBroadcasts and remote tableRebroadcasts.
- Enhancing test diagnostics with additional logging and table dumps.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| libnetwork/networkdb/networkdbdiagnostic.go | Updated diagnostic lookup to use thisNodeNetworks. |
| libnetwork/networkdb/networkdb_test.go | Modified tests to use the new network map and added dumpTable for debugging info. |
| libnetwork/networkdb/networkdb.go | Refactored Join/Leave logic to use thisNodeNetworks and introduced dual broadcast queues. |
| libnetwork/networkdb/delegate.go | Adjusted event handling to account for the new broadcast queue split. |
| libnetwork/networkdb/cluster.go | Updated network reaping and gossip functions to work with thisNodeNetworks. |
| libnetwork/networkdb/broadcast.go | Added a helper (getBroadcasts) to merge broadcasts from multiple queues. |
Comments suppressed due to low confidence (2)
libnetwork/networkdb/delegate.go:290
- Update the accompanying comment to reflect that the nil check for tableBroadcasts was removed and that remote rebroadcasts are now handled via 'tableRebroadcasts'.
if !ok || n.leaving {
libnetwork/networkdb/broadcast.go:163
- [nitpick] Consider adding detailed function comments describing the behavior, parameters, and return value of 'getBroadcasts' to improve maintainability.
func getBroadcasts(overhead, limit int, queues ...*memberlist.TransmitLimitedQueue) [][]byte {
robmry
approved these changes
Jun 17, 2025
akerouanton
approved these changes
Jun 18, 2025
|
No backport for 28.3.2 - we can backport it for the new release along with #50344 |
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.
- What I did
- How I did it
The flaky TestNetworkDBCRUDTableEntries only waits long enough for a handful of gossip cycles to execute before it asserts that the entries owned by one node exist on the other nodes. It does not wait long enough for a bulk-sync to be performed between the nodes. The test therefore asserts, effectively, that gossip is sufficient to reliably converge in a lossless network scenario. The flaky test failures are always due to the CREATE event for a single table entry not being propagated through gossip. The stats and debug logs (not committed to this PR for being very spammy with a low signal:noise ratio) revealed that when the test flakes, the CREATE event for the offending entry was enqueued but never received by the other node and the owning node's queue is holding a large number of messages. Rebroadcasts for messages received from other nodes are delaying the broadcasting of the local node's messages.
Improve the convergence rate of NetworkDB by preferentially broadcasting event messages for entries owned by the local node before rebroadcasting received messages. Enqueue the messages for rebroadcasting into a separate queue which is only pulled from if there is space left over in the gossip packet after filling it with local event messages.
- How to verify it
$ go test -count=100 -run CRUDTableEntries -failfast ./libnetwork/networkdb- Human readable description for the release notes
- Improve the convergence rate of NetworkDB, part of the management plane for overlay networking, after bursts of updates.- A picture of a cute animal (not mandatory but encouraged)