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

If vrf name #8610

Closed
wants to merge 3 commits into from
Closed

If vrf name #8610

wants to merge 3 commits into from

Conversation

donaldsharp
Copy link
Member

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.

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>
@LabN-CI
Copy link
Collaborator

LabN-CI commented May 2, 2021

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/8610 d4a86ca
Date 05/02/2021
Start 18:50:57
Finish 19:16:51
Run-Time 25:54
Total 1815
Pass 1815
Fail 0
Valgrind-Errors
Valgrind-Loss
Details vncregress-2021-05-02-18:50:57.txt
Log autoscript-2021-05-02-18:52:04.log.bz2
Memory 502 482 427

For details, please contact louberger

@idryzhov idryzhov self-requested a review May 3, 2021 13:22
Copy link
Contributor

@idryzhov idryzhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +215 to +218
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct.

  1. You can't count on the existence of the VRF here. It may not exist yet.
  2. 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) and vrf_name (may be null) to this function,
  • pass both vrf_id and vrf_name to if_new,
  • inside the if_new store both vrf_id and vrf_name in 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should change this function. It is called only from the code where we know the real vrf_id.

Comment on lines -677 to +687
vrf = vrf_get(ifp->vrf_id, NULL);
vrf = vrf_lookup_by_name(vrf_name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@idryzhov
Copy link
Contributor

idryzhov commented Feb 9, 2022

The ability to add interfaces to not-yet-known VRFs was added in f60a118.
@donaldsharp can we close this PR?

@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

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

Successfully merging this pull request may close these issues.

4 participants