gobgp icon indicating copy to clipboard operation
gobgp copied to clipboard

API: Preserve UUID in path table

Open dawn-minion opened this issue 1 year ago • 6 comments

Changes the AddPath API to add the UUID generated to the path object. Previously this UUID was only usable for deleting routes, and would be dropped when inserted into the table. That meant then if the same route was passed to a table event watcher, the UUID was set to nil, despite being made by the API.

With this change, the UUID is preserved and will be passed to callers of ListPath, or when passed to an event watcher. To implement this an additional UUID field has been added onto the table path object, and the AddPath, api2Path functions modified to preserve them. The Marshaller was also updated to return this value to end users as well.

Additionally the DeletePath call has been simplified when deleting paths by UUID. Since each path now stores the UUID they are checked directly rather than using a lookup map. This also means that the uuidMap has been removed.

dawn-minion avatar Mar 31 '23 21:03 dawn-minion

The reason UUID isn't stored in Path struct is that we avoid increasing the size of Path struct.

fujita avatar Apr 01 '23 03:04 fujita

Oh i see. Could I ask why though? 16 bytes increase seems fairly negligible, and with the removal of uuidMap it would end up being an equal amount of total memory usage.

dawn-minion avatar Apr 01 '23 04:04 dawn-minion

There are use-cases that GoBGP receives millions of routes from peers so 16 bytes in Path struct matters. On the other hand, I never heard that someone inserts millions local routes.

fujita avatar Apr 01 '23 04:04 fujita

Thank you for the explanation. Wouldn’t the memory usage be the same though? 16 bytes in uuidMap today, or 16 bytes in Path struct. This change only moves it from one place to another, it’s not 16 extra bytes. Sorry if I’m misunderstanding of course :)

dawn-minion avatar Apr 01 '23 05:04 dawn-minion

Wouldn’t the memory usage be the same though? 16 bytes in uuidMap today, or 16 bytes in Path struct.

I don't think so. Suppose that gobgpd establishes with a peer having millions routes. Then gobgpd has millions of Path objects but uuidMap is empty.

fujita avatar Apr 01 '23 06:04 fujita

~Wouldn’t it be the same here though? UUID is only set if the path is added via the gRPC AddPath, otherwise the pointer is nil. So if 1mil routes were received from a peer all uuids are nil.~

~I chose a pointer type in the internal Path so the size change is only the size of a pointer. Or is the pointer size the concern here?~

EDIT: I forget that there are two objects named "Path" in GoBGP - the gRPC one and the internal one. You are right in that the internal Path object will now be 4 or 8 bytes larger each as the UUID is stored there as a pointer (32/64 bit), so for 1 million routes it would be an increase of 4-8MiB. Is that too much for the additional functionality?

The reason I was asking for this feature is we use many GoBGP servers that share routes only via the gRPC/API, never direct peering. We add in potentially millions of routes via the gRPC and keeping track of them without the UUIDs is difficult, as it is only usable for deletion.

dawn-minion avatar Apr 01 '23 17:04 dawn-minion