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

Pull in vrfs associated with vlans #482

Merged
merged 2 commits into from
Sep 22, 2022

Conversation

jbemmel
Copy link
Collaborator

@jbemmel jbemmel commented Sep 21, 2022

As illustrated here there are cases where vlans get pulled into a node without being referenced from an interface.

Currently, any vrf associated with such a vlan does not get pulled in - while I believe it should

This may be another case of some underlying issue that is better fixed elsewhere - just submitting the case and a straightforward fix to start with

This PR also ensures there is a deterministic order in which local vrfidx values get assigned, by sorting the vrfs on their global id. This avoids a dependency on the order in which vlans (with associated vrfs) appear in the file

@jbemmel jbemmel marked this pull request as draft September 21, 2022 19:32
@jbemmel jbemmel marked this pull request as ready for review September 21, 2022 20:21
Copy link
Owner

@ipspace ipspace left a comment

Choose a reason for hiding this comment

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

Why exactly do you want to have VRFs sorted? Why does it matter?

Also, how does this fix the issue you started with (assuming it's worth fixing)?

As for "is it worth fixing": VLAN is not used ==> no VLAN interfaces ==> no VRF reference. What's the problem?

@jbemmel
Copy link
Collaborator Author

jbemmel commented Sep 22, 2022

As you can see in the commit history, at first I did not sort the vrfs. This lead to failed test cases, because pulling in the vrfs associated with vlans changed the order in which they got added to the nodes in some cases.

I didn't like that, the way vlans are associated with nodes and the order in which they are listed should not impact the vrfidx assignment imho. Those are the kinds of small irregularities that can cause operational issues. For SR OS and SR Linux I can remove those dependencies on vrfidx altogether and use globally unique id instead - but still

My use case is here, I'm using a custom script to generate BGP policies and need to reference all the ipv4 prefixes associated with all vlans - not just the ones on the local node. I also need the export community from each customer vrf.

I could instead reference each vrf and vlan on each leaf explicitly, instead of relying on the grouping mechanism or the pulling in of the customer vrf through the vlan - but that could lead to inconsistencies. I want to define things once, and derive the corresponding configs automatically

@ipspace
Copy link
Owner

ipspace commented Sep 22, 2022

As you can see in the commit history, at first I did not sort the vrfs. This lead to failed test cases, because pulling in the vrfs associated with vlans changed the order in which they got added to the nodes in some cases.

That's an interesting one -- it would be nice to understand which part of Python3 is not deterministic. I also wonder why we never hit it so far.

I didn't like that, the way vlans are associated with nodes and the order in which they are listed should not impact the vrfidx assignment imho.

And why does that matter?

Those are the kinds of small irregularities that can cause operational issues.

What operational issues exactly? We're talking about a lab-building tool. But OK, let's say I'm persuaded that consistency is good, so let's sort VRFs by ID.

My use case is here, I'm using a custom script to generate BGP policies and need to reference all the ipv4 prefixes associated with all vlans - not just the ones on the local node. I also need the export community from each customer vrf.

Plugin.

I could instead reference each vrf and vlan on each leaf explicitly, instead of relying on the grouping mechanism or the pulling in of the customer vrf through the vlan - but that could lead to inconsistencies. I want to define things once, and derive the corresponding configs automatically

Ah, now I understand why you try to pull VLAN and VRF data into nodes that don't need them. What you're trying to do is outside of the scope of netlab VLAN and VRF configuration modules, but could be easily solved with a plugin.

Copy link
Owner

@ipspace ipspace left a comment

Choose a reason for hiding this comment

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

OK, I'm merging this, but if it turns out it broke anything, it's the last unneeded bit of code I'm adding to netlab purely for someone's convenience.

@ipspace ipspace merged commit 001f670 into ipspace:dev Sep 22, 2022
@jbemmel jbemmel deleted the pull-in-vrfs-for-vlans branch September 22, 2022 12:50
ipspace added a commit that referenced this pull request Jun 21, 2023
Some module (like EVPN) modify global VRF data and expect the modified global VRF data to be used for node VRF data. Code introduced in #482 pulls global VRF data into node VRF data very early in the transformation process -- in those cases, the global changes are never copied into the node VRF data.

This PR fixes the #482 logic to create stub node VRF structures, and carefully populates them with auto-generated or global ID/RD/RT values. The full global VRF data is then merged with node VRF data in post_link_transform hook.

Modules that want to use merged (global + node) VRF data are OK to use node VRF data in post_transform hook, otherwise they should use `vrf.get_node_vrf_data` function to get the merged data. Modules that want to modify VRF data have to do so before the post_link_transform hook.
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