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

Feat(eos_designs): Add per MACVRF EVPN domain scope #2347

Merged
merged 14 commits into from
Jan 17, 2023

Conversation

MitchV85
Copy link
Contributor

@MitchV85 MitchV85 commented Dec 20, 2022

Change Summary

Add an optional evpn_l2_multi_domain boolean parameter to enable the extension of VLANs into remote EVPN domains

TOI: https://www.arista.com/en/support/toi/eos-4-26-1f/14785-multi-domain-evpn-vxlan

The evpn_l2_multi_domain parameter can be defined at the following levels:

  • Tenant
  • VRF
  • SVI/L2VLAN

It will default to a value of True, but this can be overridden as needed at the different levels defined above.

Related Issue(s)

N/A

Component(s) name

arista.avd.eos_designs

Proposed changes

Extend the Network Services data model, providing an optional evpn_l2_multi_domain boolean parameter at the following levels:

  • Tenant
  • VRF
  • SVI/L2VLAN
---

tenants:
  <TENANT NAME>:
    evpn_l2_multi_domain: < true | false | default -> true >
    vrfs:
      <VRF NAME>:
        evpn_l2_multi_domain: < true | false >
        svis:
          <VLAN ID>:
            evpn_l2_multi_domain: < true | false >

    l2vlans:
      <VLAN ID>:
            evpn_l2_multi_domain: < true | false >

Generated output if evpn_l2_multi_domain: false:

router bgp 100
   !
   vlan 10
      rd 10.231.0.2:10010
      route-target both 10010:10010
      redistribute learned

Generated output if evpn_l2_multi_domain: true:

router bgp 100
   !
   vlan 10
      rd 10.231.0.2:10010
      rd evpn domain remote 10.231.0.2:10010
      route-target both 10010:10010
      route-target import export evpn domain remote 10010:10010
      redistribute learned

How to test

molecule converge -s eos_designs_unit_tests

Checklist

User Checklist

N/A

Repository Checklist

  • My code has been rebased from devel before I start
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation and documentation have been updated accordingly.
  • I have updated molecule CI testing accordingly. (check the box if not applicable)

@github-actions github-actions bot added the role: eos_designs issue related to eos_designs role label Dec 20, 2022
@github-actions github-actions bot added role: eos_cli_config_gen issue related to eos_cli_config_gen role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated and removed role: eos_cli_config_gen issue related to eos_cli_config_gen role state: Documentation role Updated state: CI Updated CI scenario have been updated in the PR labels Jan 3, 2023
Copy link
Contributor

@ClausHolbechArista ClausHolbechArista left a comment

Choose a reason for hiding this comment

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

We need to add

  • Documentation (update the network-services.md file)
  • Molecule test case

Maybe we also need to check if this data model is what we want. Maybe a single boolean "evpn_export_to_remote_domain" (or something better).

@MitchV85
Copy link
Contributor Author

MitchV85 commented Jan 4, 2023

We need to add

  • Documentation (update the network-services.md file)
  • Molecule test case

Maybe we also need to check if this data model is what we want. Maybe a single boolean "evpn_export_to_remote_domain" (or something better).

I like this idea. I've changed the data model to a boolean parameter called evpn_multi_domain. It's optional, and will default to false, which is a change from the existing default behavior of making all MACVRFs mutli-domain when a node is an EVPN/VXLAN L2 GW.

@github-actions github-actions bot added state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated labels Jan 4, 2023
@ClausHolbechArista
Copy link
Contributor

@carlbuchmann please review the documentation on this when you are back.

@MitchV85 MitchV85 marked this pull request as ready for review January 4, 2023 16:30
@MitchV85 MitchV85 requested a review from a team as a code owner January 4, 2023 16:30
Copy link
Contributor

@gmuloc gmuloc left a comment

Choose a reason for hiding this comment

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

couple of quick comments

Question on the testing - it looks like the feature is only tested on VRF and not on VLANs - is it correct? Would it be possible to add a test for Vlan as well to prevent regression there in the future?

@MitchV85
Copy link
Contributor Author

couple of quick comments

Question on the testing - it looks like the feature is only tested on VRF and not on VLANs - is it correct? Would it be possible to add a test for Vlan as well to prevent regression there in the future?

Are you referring to setting evpn_multi_domain to False for a VLAN in the inventory files used for the molecule test (molecule/eos_designs_unit_tests/inventory/group_vars/DC1_TENANTS_NETWORKS/)? If so, we can certainly make this adjustment. Let me know!

@gmuloc
Copy link
Contributor

gmuloc commented Jan 11, 2023

couple of quick comments
Question on the testing - it looks like the feature is only tested on VRF and not on VLANs - is it correct? Would it be possible to add a test for Vlan as well to prevent regression there in the future?

Are you referring to setting evpn_multi_domain to False for a VLAN in the inventory files used for the molecule test (molecule/eos_designs_unit_tests/inventory/group_vars/DC1_TENANTS_NETWORKS/)? If so, we can certainly make this adjustment. Let me know!

Yeah that's what I mean adding a SVI test

for now I am not sure what is the expected config for a SVI if you have the new setting as follow:

global: True
vrf: False
svi: None

Looking at
def _router_bgp_vlans_vlan(self, vlan, tenant) -> dict | None: new line it seems that the VRF value would be ignored and the value for the vlans would end up being False

https://github.com/aristanetworks/ansible-avd/pull/2347/files#diff-91e209c523bb520a8c29f348c4bf3734e3c9e0323090b85f1549f6ec28b898d4R293

if self._evpn_gateway_vxlan_l2 and default(vlan.get("evpn_multi_domain"), tenant.get("evpn_multi_domain", True)) is True:

however the documentation states that it should override tenant AND vrf settings

# Explicitly extend this VLAN to remote EVPN domains to override setting of tenants.<tenant>.evpn_multi_domain (default -> true)
# and tenants.<tenant>.vrfs.<vrf>.evpn_multi_domain

Should it be something like: (though the vrf is not available in the function today)

if self._evpn_gateway_vxlan_l2 and default(vlan.get("evpn_multi_domain"), vrf.get("evpn_multi_domain"), tenant.get("evpn_multi_domain", True)) is True:

@MitchV85
Copy link
Contributor Author

With the current changes...

  • VLAN-Aware Bundle MACVRF model, the Tenant/VRF is where evpn_multi_domain can be set. Setting this value under the SVI will not have an impact on a VLAN that's a part of a VLAN-Aware Bundle.

  • For VLAN-Based, setting the evpn_multi_domain parameter under the SVI/VLAN works as intended. This was tested by setting evpn_vlan_aware_bundles: false in molecule/eos_designs_unit_tests/inventory/group_vars/DC1_FABRIC.yml.

  • Setting evpn_multi_domain parameter under the VRF in a VLAN-Based model will not have an effect since (as you pointed out) VRF is not available in the _router_bgp_vlans_vlan function today.

It doesn't appear that anything in DC1 leverages VLAN-Based MACVRFs for molecule testing today...Do we need a new DC2 test?

It sounds like we're at a fork in the road here...perhaps adding vrf to the _router_bgp_vlans_vlan function is the right approach here so that there isn't any confusion related to the evpn_multi_domain parameter under vrf being ignored for the VLAN-Based MACVRF model. I'm not sure what the ripple effect is of this change, but I will give it a shot this evening!

Thanks for the feedback thus far!

@github-actions github-actions bot added the state: conflict PR with conflict label Jan 12, 2023
@github-actions
Copy link

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

@MitchV85
Copy link
Contributor Author

@gmuloc and @ClausHolbechArista

I've added vrf to the _router_bgp_vlans_vlan function in eos_designs/python_modules/network_services/router_bgp.py

setting evpn_multi_domain parameter under vrf is now valid for both the VLAN-Based MACVRF model and VLAN-Aware Bundle MACVRF models.

The molecule test was adjusted to enable VLAN-Based MACVRF testing.

  • evpn_vlan_aware_bundles: false was set in molecule/eos_designs_unit_tests/inventory/group_vars/DC1_FABRIC.yml
  • evpn_multi_domain was set to false at the tenant level in molecule/eos_designs_unit_tests/inventory/group_vars/DC1_TENANTS_NETWORKS/Tenant_A.yml
  • evpn_multi_domain was set to false at the vrf level in molecule/eos_designs_unit_tests/inventory/group_vars/DC1_TENANTS_NETWORKS/Tenant_B.yml
  • evpn_multi_domain was set to false at the vrf level and true at the svi level in molecule/eos_designs_unit_tests/inventory/group_vars/DC1_TENANTS_NETWORKS/Tenant_C.yml

Net result is that only VLAN 350 in Tenant C has a MACVRF with remote domain RD/RT values. This is the expected behavior.

Let me know what I missed! Once this is good, will update the documentation...

Thanks again!

@@ -290,7 +290,7 @@ def _router_bgp_vlans_vlan(self, vlan, tenant) -> dict | None:
"eos_cli": get(vlan, "bgp.raw_eos_cli"),
"struct_cfg": get(vlan, "bgp.structured_config"),
}
if self._evpn_gateway_vxlan_l2:
if self._evpn_gateway_vxlan_l2 and default(vlan.get("evpn_multi_domain"), tenant.get("evpn_multi_domain"), vrf.get("evpn_multi_domain"), True) is True:
Copy link
Contributor

@gmuloc gmuloc Jan 12, 2023

Choose a reason for hiding this comment

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

I would change the order to vlan -> vrf -> tenant rather than vlan -> tenant -> vrf?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@github-actions github-actions bot added role: eos_cli_config_gen issue related to eos_cli_config_gen role type: code quality CI and development toolset type: documentation Improvements or additions to documentation labels Jan 12, 2023
@MitchV85 MitchV85 closed this Jan 12, 2023
@github-actions github-actions bot added the state: CI Updated CI scenario have been updated in the PR label Jan 12, 2023
@MitchV85 MitchV85 marked this pull request as ready for review January 12, 2023 22:20
@MitchV85
Copy link
Contributor Author

Ready for review again. Thanks again for all the help @gmuloc @ClausHolbechArista @tgodaA !

@ClausHolbechArista
Copy link
Contributor

Since all this only affect the L2 gateway and vlan services, we should probably include l2 in the key. We also have L3 gateway functionality, where you may want to limit VRFs/tenants independently of L2.

@MitchV85
Copy link
Contributor Author

MitchV85 commented Jan 16, 2023

Since all this only affect the L2 gateway and vlan services, we should probably include l2 in the key. We also have L3 gateway functionality, where you may want to limit VRFs/tenants independently of L2.

I like this suggestion. Changed evpn_multi_domain parameter to evpn_l2_multi_domain

Copy link
Contributor

@tgodaA tgodaA left a comment

Choose a reason for hiding this comment

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

LGTM

@ClausHolbechArista ClausHolbechArista merged commit 9ce39a3 into aristanetworks:devel Jan 17, 2023
@MitchV85 MitchV85 deleted the evpn-l2gw-toggle branch January 17, 2023 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rn: Feat(eos_designs) role: eos_designs issue related to eos_designs role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants