Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: Preserve UUID in path table #2644

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dawn-minion
Copy link
Contributor

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.

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.
@fujita
Copy link
Member

fujita commented Apr 1, 2023

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

@dawn-minion
Copy link
Contributor Author

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.

@fujita
Copy link
Member

fujita commented Apr 1, 2023

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.

@dawn-minion
Copy link
Contributor Author

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 :)

@fujita
Copy link
Member

fujita commented Apr 1, 2023

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.

@dawn-minion
Copy link
Contributor Author

dawn-minion commented Apr 1, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants