If vrf name #8610
If vrf name #8610
Conversation
Instead of relying on the ifp->vrf_id, pass in the vrf_name for lookup. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Use the vrf name for if_create_name instead of the vrf_id. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
On creation of ifindex's use the vrf name instead of the vrf index. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
There was a problem hiding this comment.
I left some comments. Overall the idea seems to solve the problem of creating the interface in non-existing VRF but it adds a possibility of a memory leak.
Example for OSPF:
- create ospf router in non-existing vrf
- create virtual link in this ospf router
After that, the VRF is automatically created and never deleted even if the ospf config is deleted.
To correctly solve this issue we should actually allow creating the interface without the underlying VRF and associating the interface with a VRF after its instantiation.
| vrf = vrf_lookup_by_name(vrf_name); | ||
| assert(vrf); | ||
|
|
||
| ifp = if_new(vrf_id); | ||
| ifp = if_new(vrf->vrf_id != VRF_UNKNOWN ? vrf->vrf_id : VRF_DEFAULT); |
There was a problem hiding this comment.
This is not correct.
- You can't count on the existence of the VRF here. It may not exist yet.
- Why do you associate the interface with the default VRF when the index is VRF_UNKNOWN? The VRF name is known and we should associate the interface with this VRF.
To summarize the above, I think we should do the following:
- pass both
vrf_id(may be unknown) andvrf_name(may be null) to this function, - pass both
vrf_idandvrf_nametoif_new, - inside the
if_newstore bothvrf_idandvrf_namein the interface structure for later use.
This allows calling this function from places where we know only one of the two arguments.
|
|
||
| hook_call(if_add, ifp); | ||
| return ifp; | ||
| } | ||
|
|
||
| struct interface *if_create_ifindex(ifindex_t ifindex, vrf_id_t vrf_id) | ||
| struct interface *if_create_ifindex(ifindex_t ifindex, const char *vrf_name) |
There was a problem hiding this comment.
I don't think we should change this function. It is called only from the code where we know the real vrf_id.
| vrf = vrf_get(ifp->vrf_id, NULL); | ||
| vrf = vrf_lookup_by_name(vrf_name); |
There was a problem hiding this comment.
We must call vrf_get here to always create the VRF if it doesn't exist.
Following my previous suggestion of saving both vrf_id and vrf_name in the interface structure, this should be changed to:
vrf = vrf_get(ifp->vrf_id, ifp->vrf_name);
One of the arguments may be NULL, this is already handled in vrf_get.
The same change should be done in if_set_index.
|
The ability to add interfaces to not-yet-known VRFs was added in f60a118. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
convert code to use the vrf name instead of the vrf id for interface creation. There are cases where we have vrf_id of unknown due to pre-creation of vrfs. Start allowing us to associate these interfaces with the vrfs when we know the name but not the id yet.