datapath: Few minor improvements to DevicesController #28887
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
Add a helper to construct only RWTable[T] and TableMeta to allow
creating a table in a way that enforces that the table is populated
by the producer before it can be read:
var Cell = cell.Module("example", "Example module",
// Privately provides RWTable[*Foo]:
statedb.NewPrivateRWTableCell[*Foo]("foos", FooIDIndex),
cell.Provide(New),
)
func New(lc hive.Lifecycle, t statedb.RWTable[*Foo]) (FooController, Table[*Foo]) {
fooCtrl := ...
lc.Append(fooCtrl) // fooCtrl now starts before anything that reads Table[*Foo]
return fooCtl, t
}
Signed-off-by: Jussi Maki <jussi@isovalent.com>
To enforce that the device and route tables are populated before being read from, provide the Table[*Device] and Table[*Route[ from the DevicesController constructor. This makes sure that anything that depends on e.g. 'Table[*Device]' will be constructed after DevicesController and thus the DevicesController start hook will execute before it. Signed-off-by: Jussi Maki <jussi@isovalent.com>
As Linux does not send comprehensive set of route delete messages on device deletion, flush the routes on the link delete message. Since the link and route messages are coming over separate sockets they may come out of order, so keep track of what link indexes have been deleted and ignore route updates for deleted links. Linux does reuse the index after the 32 bits roll around, so remove the "dead link index" on link creation. Signed-off-by: Jussi Maki <jussi@isovalent.com>
Since subscribing to links/addresses/routes and then listing them is racy we might end up seeing the same address in the initial listing and as an update. Avoid adding the address twice by checking its existence first. Side note: we need the initial listing to be able to populate the tables before readers access them in order to keep the existing non-reconciling semantics. The netlink library we're using does not have a mechanism to inform that the initial listing is done in the subscriptions (even though netlink DONE message is sent for this), so instead DevicesController does subscribe + list which necessitates dealing with issues like this. Signed-off-by: Jussi Maki <jussi@isovalent.com>
dylandreimerink
approved these changes
Oct 31, 2023
|
/test |
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.
This pulls out couple improvemenst from #28712 to its own PR:
statedb: Add NewPrivateRWTableCell: Add ability for a constructor to provide theTable[T]and thus allow populating a table before readers can access it.datapath: Ensure device and route tables are populated before readers: Make sure DevicesController has initially populated the devices table before anything reads it. Note that this was not a bug earlier as we didn't useTable[*Device]directly but rather viaoption.Config.GetDeviceswhich came fromDeviceManagerwrapper aroundDevicesControllerand that did enforce the ordering.datapath/devices: Flush routes on device delete: Fix cleaning of route entries when device is deleted. Surprisingly Linux doesn't send netlink notifications for each deleted route when link is removed. This was not a big issue yet as routes were only used to figure out if a 'veth' device should be selected based on if it had a default route or not.datapath/devices: Avoid adding duplicate address: As DevicesController needs to populate the devices table before readers see it, it needs to do a Subscribe and List. This leads to a race where one might see an address both in listing and as an update. Fix this by not adding the address twice. (I did also think about using asets.Setfor addresses, but I prefer that they're in a slice and in a stable order. Happy to reconsider though).