node: Implement LocalNodeStore as StateDB table #40918
Merged
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
58c5fc3 to
0c9cb29
Compare
|
/test |
0c9cb29 to
c8d89f4
Compare
This was referenced Aug 7, 2025
|
/test |
c8d89f4 to
d20e91d
Compare
|
/test |
bimmlerd
reviewed
Aug 7, 2025
smagnani96
approved these changes
Aug 7, 2025
d20e91d to
a60f457
Compare
|
/test |
01172da to
f4d850f
Compare
|
/test |
f4d850f to
98da628
Compare
|
/test |
tommyp1ckles
approved these changes
Aug 14, 2025
sayboras
approved these changes
Aug 14, 2025
98da628 to
0350388
Compare
|
/test |
0350388 to
642fff7
Compare
|
/test |
642fff7 to
3c1b027
Compare
|
/test |
3c1b027 to
44905a8
Compare
brb
approved these changes
Aug 15, 2025
44905a8 to
266a9be
Compare
As the first step of transition towards a unified table of nodes, add a StateDB table for the local node which will allow us to implement LocalNodeStore with it. The type is 'TempLocalNode' so we can replace the existing 'LocalNode' with it in the next commit. Eventually we would have also the remote nodes in this table, but until then to avoid confusion the table is named "local-node" and the type is 'LocalNode'. We'll also need to rethink the role of 'types.Node' which is the serialization struct for KVStore and we likely want to eventually decouple that from our internal representation. Baby steps... Signed-off-by: Jussi Maki <jussi@isovalent.com>
As a first step towards replacing LocalNodeStore and NodeManager with a StateDB table of nodes, implement LocalNodeStore using the Table[*node.LocalNode]. The 'LocalNode' type now has the attributes relevant only for the local node behind 'LocalNodeInfo' to make transitioning this into a table of both local and remote nodes easier. Few tests were also adopted to use 'Eventually' since now 'LocalNodeStore.Observe' is not synchronous to calls to 'LocalNodeStore.Update'. This has the immediate benefit that observers of LocalNodeStore can no longer block the updates to it and that we can inspect the local node in cilium shell via 'db/show local-node'. Signed-off-by: Jussi Maki <jussi@isovalent.com>
Without the IdentityAllocator the test sometimes fails in label injection due to unset local identity allocator if `nodeDiscovery.StartDiscovery()` has managed to enqueue the prefixes of the local node. Signed-off-by: Jussi Maki <jussi@isovalent.com>
266a9be to
249292a
Compare
|
/test |
This was referenced Aug 20, 2025
Closed
This was referenced Sep 5, 2025
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.
As a first step towards replacing LocalNodeStore and NodeManager with a StateDB table of nodes, implement LocalNodeStore as a Table[*node.LocalNode]. While the longer term goal would be a unified table of local and remote nodes this needs more thinking around e.g. how the
types.Nodewould need to evolve etc. So opting for a small incremental change first.This has the immediate benefit that observers of LocalNodeStore can no longer block the updates to it making it much safer. Most recently we hit this in #40095 (comment) where innocent rate limiting effectively throttled LocalNodeStore update throughput to 1 per second. With this change we'd remove this foot gun.
Another benefit is that we can inspect the local node in cilium shell via 'db/show local-node' (
-f jsonfor full contents).Further motivation behind this change is to simplify multiple control loops in e.g.
pkg/loadbalancerthat are almost purely around tables with theLocalNodeStorebeing the exception. Examples of this can be found from #40998, #41000 and #41001 where we can remove clunky atomic pointer side channels.The last commit fixed flakyness in the wireguard agent tests where IPCache was configured without IdentityAllocator and thus sometimes the test hit the label injection with the local node's prefixes causing it to fail to bump revision and thus causing peer GC to be stuck. Looks like the LocalNodeStore changes here made it easier to trigger.
stress -p 1 go test ./pkg/... -test.count 1said1h1m35s: 84 runs so far, 3 failures (3.57%), 1 activewith the 3 failures all being a workqueue goroutine leak that #41129 would fix, so doesn't look like there are other obvious flakes due to this change.