Fix(eos_designs): Connected endpoints ESI should only be configured on EVPN VTEPs. #4020
Conversation
|
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-4020
# Activate the virtual environment
source test-avd-pr-4020/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/laxmikantchintakindi/avd.git@esi#subdirectory=python-avd" --force
# Install Ansible collection
ansible-galaxy collection install git+https://github.com/laxmikantchintakindi/avd.git#/ansible_collections/arista/avd/,esi --force
# Optional: Install AVD examples
cd test-avd-pr-4020
ansible-playbook arista.avd.install_examples |
| @@ -104,8 +104,11 @@ def _get_short_esi(self, adapter: dict, channel_group_id: int, short_esi: str = | |||
| # short_esi is only set when called from sub-interface port-channels. | |||
| if short_esi is None: | |||
| # Setting short_esi under port_channel will be removed in AVD5.0 | |||
| port_channel_short_esi = get(adapter, "port_channel.short_esi") | |||
| if (short_esi := get(adapter, "ethernet_segment.short_esi", default=port_channel_short_esi)) is None: | |||
| if self._hostvars["type"] != "l2leaf": | |||
There was a problem hiding this comment.
This is not how we should check for it. Types are customizable, so we have to check for capabilities instead. I suggest checking this:
| if self._hostvars["type"] != "l2leaf": | |
| if self.shared_utils.overlay_evpn and self.shared_utils.overlay_vtep: |
There was a problem hiding this comment.
Also this will not capture all cases, since you add it below the short_esi is none check. Please move these two conditions (negated) to the if in line 100. So we return None if these two are NOT true.
There was a problem hiding this comment.
My bad. It should also be for evpn-mpls so I will update my comment above
There was a problem hiding this comment.
if self.shared_utils.overlay_evpn and self.shared_utils.overlay_vtep:
can we write 'or' instead of 'and'?
There was a problem hiding this comment.
you can implement it any way you like. Just make sure you understand the logic and how it would work first.
There was a problem hiding this comment.
Fixed.
ClausHolbechArista
changed the title
ansible_collections/arista/avd/roles/eos_designs/python_modules/connected_endpoints/utils.py
Outdated
Show resolved
Hide resolved
github-actions
bot
added
the
state: CI Updated
ClausHolbechArista
added
the
one approval
|
Change Summary
Connected endpoints ESI should only matter for EVPN VTEPs. It is not needed to configure short_esi on l2leafs.
Related Issue(s)
Fixes #3064
Component(s) name
arista.avd.eos_designsProposed changes
Change python module to skip generating short_esi for l2leafs.
How to test
Add short_esi under l2leaf and make sure it is not generated in structured config.
Checklist
User Checklist
Repository Checklist