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): Accept auto as argument for rd_override and rt_override #4858

Draft
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

rrajpuro-anet
Copy link

Change Summary

This PR adds feature to use the specific keyword auto when overriding rd or rt to auto.

Related Issue(s)

Fixes #4857

Component(s) name

arista.avd.eos_designs

Proposed changes

  • Tests the rd_overrride or rt_override to match the literal string auto.
  • Return auto if matches

Pending changes

  • Need to update documenation to following
  rt_override supports three formats:
    - A single number which will be used in the RT fields instead of mac_vrf_id/mac_vrf_vni (see 'overlay_rt_type' for details).
    - A full RT string with colon separator which will override the full RT.
    - The literal string `auto`           
  rd_override supports three formats:
    - A single number which will be used in the RD assigned number field instead of mac_vrf_id/mac_vrf_vni (see 'overlay_rd_type' for details).
    - A full RD string with colon separator which will override the full RD.
    - The literal string `auto`
  • Need to write tests
  • 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)

@rrajpuro-anet rrajpuro-anet requested review from a team as code owners January 6, 2025 20:07
Copy link

github-actions bot commented Jan 6, 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-4858
# Activate the virtual environment
source test-avd-pr-4858/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/rrajpuro-anet/avd.git@devel#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/rrajpuro-anet/avd.git#/ansible_collections/arista/avd/,devel --force
# Optional: Install AVD examples
cd test-avd-pr-4858
ansible-playbook arista.avd.install_examples

@rrajpuro-anet rrajpuro-anet changed the title Feat (eos_cli_config_gen): Accept auto as argument for rd_override Feat (eos_cli_config_gen): Accept auto as argument for rd_override and rt_override Jan 6, 2025
@ClausHolbechArista ClausHolbechArista changed the title Feat (eos_cli_config_gen): Accept auto as argument for rd_override and rt_override Feat(eos_cli_config_gen): Accept auto as argument for rd_override and rt_override Jan 7, 2025
@ClausHolbechArista
Copy link
Contributor

Please add this to molecule tests and verify the outputs are as expected.

I would suggest modifying the file ansible_collections/arista/avd/molecule/eos_designs_unit_tests/inventory/group_vars/DC1_TENANT_NETWORKS/Tenant_A.yml and add the rt / rd override on VRF Tenant_a_ERP_Zone (line 78) and the SVI 122 below that.
If this also applies to EVPN bundles, we should also add testing in ansible_collections/arista/avd/molecule/eos_designs_unit_tests/inventory/host_vars/evpn_vlan_bundle.yml where you should test it on one of the bundles (ex. bundle6 in line 162). In the same file it would also be interesting to test on vlan 20 (line 29) since it will get it's own bundle using a different code path.

After updating the files mentioned you can rerun molecule with:

cd ansible_collections/arista/avd
molecule converge -s eos_designs_unit_tests

and then inspect the changed files if you get all and only the changes you expect.

Moving this PR to draft while you work on this. Set it "ready for review" once you have addressed by comments and added test coverage.

@ClausHolbechArista ClausHolbechArista marked this pull request as draft January 7, 2025 07:00
@gmuloc gmuloc changed the title Feat(eos_cli_config_gen): Accept auto as argument for rd_override and rt_override Feat(eos_designs): Accept auto as argument for rd_override and rt_override Jan 7, 2025
- Check for None is cheaper than isInstance

Co-authored-by: Claus Holbech <[email protected]>
@rrajpuro-anet
Copy link
Author

I ran the above requested unit tests are following are my observations on setting rt and rd override:

Case1: evpn_vlan_aware_bundles: true

  1. setting on vrf: overrides as expected.
  2. setting on a specific bundle in evpn_vlan_bundles: overrides as expected.
  3. setting on a single vlan in l2vlans: overrides only if vlans are not auto bundles based on vlan name.
  4. setting on a single svi in vrfs.svis: does not override if bundles are auto generated

Case2: evpn_vlan_aware_bundles: false

  1. setting on vrf: overrides as expected.
  2. setting on a specific bundle in evpn_vlan_bundles: overrides as expected.
  3. setting on a single vlan in l2vlans: overrides as expected.
  4. setting on a single svi in vrfs.svis: overrides as expected.

In my opinion it does not make much sense to override these parameters when using vlan aware bundles or atleast when they are auto assigned.

If we agree. Please let me know what tests to add so that we can merge this feature as soon as possilbe.

NOTE: DC1_FABRIC has evpn_vlan_aware_bundles: true. The following test will not validate our requirement.

I would suggest modifying the file ansible_collections/arista/avd/molecule/eos_designs_unit_tests/inventory/group_vars/DC1_TENANT_NETWORKS/Tenant_A.yml and add the rt / rd override on VRF Tenant_a_ERP_Zone (line 78) and the SVI 122 below that.

Molecule Test commits on this branch -> https://github.com/rrajpuro-anet/avd/tree/rd-rt-override

@ClausHolbechArista
Copy link
Contributor

If we agree. Please let me know what tests to add so that we can merge this feature as soon as possilbe.

Thank you for going through the tests. The tests you have comitted cover the non-bundle case. If you could add the auto variant in some bundle test as well that would be good. You don't need to add on SVI for the bundle test, but you would need to cover the three other cases since the code paths are separate. Thanks.

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.

Feat (eos_designs): Accept auto as argument for rd_override
2 participants