search

FRRouting / frr

The FRRouting Protocol Suite
https://frrouting.org/
Other
3.27k stars 1.24k forks source link

Add Mapping From Table ID to VRF #5982

Open sworleys opened 4 years ago

sworleys commented 4 years ago

In zebra/rt_netlink.c, we call vrf_lookup_by_id() when we get a route to find the associated vrf object for that table.

This should be improved to be a hash table mapping of Table ID to zebra_vrf object.

You would need to create a hash table probably in zebra/zebra_vrf.h that uses the table_id hash as a key and then update the code in vrf_lookup_by_id() to use this.

This should be a fun one! Good luck!

To Reproduce

  1. Create a vrf in the kernel with iproute2
  2. ip link add vrf-red type vrf table 1111
  3. Put an interface in the vrf
  4. ip link add test type dummy
  5. ip link set dev test master vrf-red
  6. ip link set test up
  7. Create a route in the kernel with iproute2 in the vrf
  8. ip route add 3.3.3.3/32 dev test vrf vrf-red
  9. Zebra reads in the route in the vrf and calls the function

With the new code you should be able to use that table ID 1111 to lookup the vrf.

Versions

proelbtn commented 4 years ago

I'll try it.

pguibert6WIND commented 4 years ago

can the hash_lookup() function be avoided when vrf backend is not netns ?

sworleys commented 4 years ago

can the hash_lookup() function be avoided when vrf backend is not netns ?

sure that makes sense to me.

sworleys commented 4 years ago

@proelbtn you will need to a vrf_is_backend_netns() check or something similar in the codepath before to doing the lookup. If it is netns, you will need to continue the old way probably.

85ayush commented 3 years ago

I would love to work on this issue, I am new to open source contribution. Can you please guide me?

sworleys commented 3 years ago

I would love to work on this issue, I am new to open source contribution. Can you please guide me?

Definitely!

85ayush commented 3 years ago

Thank you, so I am starting working on it, where should I start from? And if I get stuck somewhere how can I contact you for help?

Please reply.

On Tue, 23 Feb 2021, 10:30 pm Stephen Worley, notifications@github.com wrote:

I would love to work on this issue, I am new to open source contribution. Can you please guide me?

Definitely!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/FRRouting/frr/issues/5982#issuecomment-784351311, or unsubscribe https://github.com/notifications/unsubscribe-auth/AR2RQWCG66TOS2LHUYNB7O3TAPNMTANCNFSM4LF2PXLA .

nathan-duddles commented 2 years ago

I see in zebra/rt_netlink.c that vrf_lookup_by_id() is used at lines 1405 and 1650. This function is defined in lib/vrf.c starting at line 275 and uses a find function on a red-black tree (lib/vrf.h includes openbsd-tree.h).

Is this issue for updating the implementation of vrf_lookup_by_table() which starts at line 367 in zebra/rt_netlink.c?

It seems the implementation of vrf_lookup_by_table() is iterating through a red-black tree. It looks like this function is only used at line 768 in zebra/rt_netlink.c and line 343 in if_netlink.c .

Edit: grammar

sworleys commented 2 years ago

I see in zebra/rt_netlink.c that vrf_lookup_by_id() is used at lines 1405 and 1650. This function is defined in lib/vrf.c starting at line 275 and uses a find function on a red-black tree (lib/vrf.h includes openbsd-tree.h).

Is this issue for updating the implementation of vrf_lookup_by_table() which starts at line 367 in zebra/rt_netlink.c?

It seems the implementation of vrf_lookup_by_table() is iterating through a red-black tree. It looks like this function is only used at line 768 in zebra/rt_netlink.c and line 343 in if_netlink.c .

Edit: grammar

Yea the idea is to create a table or tree to maintain a lookup from ID -> vrf

so that we don't have to iterate over all the VRFs and match them.

Yes you update the implementation here and move the function to reside in lib/vrf.c[h] as a more general API.

nathan-duddles commented 2 years ago

Thanks for your reply!

I have another basic question: Do you recommend using lib/hash.c for this?

I see there is some documentation on this in frr/doc/developer/lists.rst. Besides this and the source code, is there anything else I should be looking at?

sworleys commented 2 years ago

You won't use lib/hash directly. That is the old style. We have a newer API provided by lib/typesafe.h that acts as a wrapper for all our list/table/tree implementations to provide common macros for lookup/iteration.

if you look here you can see the macros available to use and how to use them: http://docs.frrouting.org/projects/dev-guide/en/latest/lists.html#cheat-sheet

there are also examples scattered about the code base.

nathan-duddles commented 2 years ago

Thank you, @sworleys! This is super helpful!

nathan-duddles commented 2 years ago

I think I may be ready to submit a pull request for this. I have my patch on this branch.

I have gone through the steps in the Pre-submission Checklist, except when I run make test in the frr directory, I get the message: make: *** No rule to make target 'test'. Stop. I'm running all topotests on my branch now.

I have also done my manual testing by running ospf with vrf in GNS3 between alpine docker images built from my branch, and everything worked as it did when built from the master branch.

Are there any other ways you would suggest testing for this?

Before submitting a pull request, should I rebase my branch?

donaldsharp commented 2 years ago

it's make check not make test. Easy to get that mixed up. If topotests work then great. It's always a great idea to rebase to latest master and push up changes without merge commits

nathan-duddles commented 2 years ago

Thanks! Now I do remember seeing make check somewhere... it looks like this may need to updated in the Pre-submission Checklist section: http://docs.frrouting.org/projects/dev-guide/en/latest/workflow.html#pre-submission-checklist.

make check results: 435 passed, 5 skipped, 3 warnings

Warning are all: PytestCacheWarning: cache could not write path...

Topology tests are still running, but seem to be doing fine. I'll go ahead and rebase and then submit a pull request.

One thing that I found when searching the code base is another potential place where the vrf_lookup_by_table() could be used. This is in pimd/pim_iface.c at line 1714. Should I submit an issue and/or pull request about this?

Secondly, as suggested earlier in this thread, I didn't create a hashmap or change the way vrf_lookup_by_table() worked vrf backended by netns. That said, it seems that this could be done in a similar way as with table_id. Should try implementing this also?

Shubh-Agarwal30 commented 1 year ago

hey is this issue is still open to work upon , or someone is working on it?