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

Refactor(eos_designs): Remove dependency on overlay_routing_protocol and evpn_role for WAN routers #4865

Open
wants to merge 30 commits into
base: devel
Choose a base branch
from

Conversation

gmuloc
Copy link
Contributor

@gmuloc gmuloc commented Jan 9, 2025

Change Summary

This is a stepping stone PR to introduce EVPN GW between DPS and VXLAN in PR #4831

Since the intro of the WAN routers, the overlay_routing_protocol HAD to be ibgp and the evpn_role HAD to be the same as the wan_role. Making both keys being essentially ignored.
This PR decorelates and "relax" the requirements.

It introduces a new boolean that continue to ignore the keys and it is enabled by default in order to prevent extra configuration to be generated. the knob is added as PREVIEW

Component(s) name

arista.avd.eos_designs

Proposed changes

  • [Adjust multiple places where the code was implicitly relying on iBGP / evpn_role == wan_role for decisions around iBGP and so on. Relying only on wan_router + wan_role (eventually could add later relying on wan_mode but today both are actually using iBGP)
  • Adjust schema to remove the limitations
  • Remove negative unit tests

How to test

  • molecule test should render the same.
  • need to add new molecule test to enable EVPN on LAN and be happy

Checklist

User Checklit

  • need to review WAN how-to to clean it up
  • need to review cv-pathfinder example to clean it up

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 state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated role: eos_designs issue related to eos_designs role labels Jan 9, 2025
Copy link

github-actions bot commented Jan 9, 2025

Review docs on Read the Docs

To test this pull request:

# Create virtual environment for this testing below the current directory
python -m venv test-avd-pr-4865
# Activate the virtual environment
source test-avd-pr-4865/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/gmuloc/avd.git@relax-wan-default-requirements#subdirectory=python-avd" --force
# Point Ansible collections path to the Python virtual environment
export ANSIBLE_COLLECTIONS_PATH=$VIRTUAL_ENV/ansible_collections
# Install Ansible collection
ansible-galaxy collection install git+https://github.com/gmuloc/avd.git#/ansible_collections/arista/avd/,relax-wan-default-requirements --force
# Optional: Install AVD examples
cd test-avd-pr-4865
ansible-playbook arista.avd.install_examples

@gmuloc gmuloc marked this pull request as ready for review January 20, 2025 15:29
@gmuloc gmuloc requested review from a team as code owners January 20, 2025 15:29
python-avd/pyavd/_eos_designs/shared_utils/overlay.py Outdated Show resolved Hide resolved
return (self.inputs.overlay_routing_protocol or default_overlay_routing_protocol).lower()

@cached_property
def overlay_address_families(self: SharedUtils) -> list[str]:
if self.overlay_routing_protocol in ["ebgp", "ibgp"]:
if self.overlay_routing_protocol in ["ebgp", "ibgp"] or self.is_wan_router:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? Would it not be clearer if we add the wan_router handling in the places where this is checked today?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so in filtered_tenants we are looping over these address families to see if we select a VRF. Which means we would need to check if the af of the vrf.address_families are in the overlay_address_families or if it is as WAN router AND the af is epvn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

otherwise it could work from the point of view of network_services/router_bgp where we could vorce the call to _update_router_bgp_vrf_evpn_or_mpls_cfg for a WAN router

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer if we remove all ties to overlay_address_families from WAN, since it would be the only way the user could prevent a VRF from going to the LAN in a future state. (one VRF only being on WAN and local interfaces, but not extended to LAN)

@@ -19,6 +19,9 @@ vlan 101
vlan 666
name VLAN666
!
vlan 1000
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is added because a new SVI was created

Comment on lines 134 to 135
vrf default
!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should not happen

@@ -186,6 +186,7 @@ router_bgp:
redistribute:
connected:
enabled: true
- name: default
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should not happen

Comment on lines +7 to +8
# the default is currently ibgp for WAN routers but need to change it soon.
overlay_routing_protocol: none
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is probably not needed now that we are setting it to None without the knob. Test this

@@ -0,0 +1,3 @@
---
# Setting back to ebgp for Leaves
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if removing the none in CV_PATHFINDER_TESTS then this is not needed anymore

@@ -64,6 +64,7 @@ def overlay(self: EosDesignsFacts) -> dict | None:
@cached_property
def vtep_ip(self: EosDesignsFacts) -> str | None:
"""Exposed in avd_switch_facts."""
if self.shared_utils.vtep:
# TODO: Probably need to handle this differently for WAN router - maybe `dps_ip` is needed.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ClausHolbechArista - need to discuss this together

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants