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_cli_config_gen): Add interface traffic engineering and te admin group for ethernet/port-channel #4754

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

Conversation

emilarista
Copy link
Contributor

Change Summary

Adds data model for interface traffic engineering and te admin groups for ethernet and port-channel interfaces.

Component(s) name

arista.avd.eos_cli_config_gen

Proposed changes

Adds the following data model to ethernet and port-channel interfaces:

    traffic_engineering:
      enabled: <bool>
      administrative_groups: <list>

Checklist

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)

Copy link

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-4754
# Activate the virtual environment
source test-avd-pr-4754/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/emilarista/ansible-avd.git@feat/eos_cli_config_gen/interface-te-admingrp#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/emilarista/ansible-avd.git#/ansible_collections/arista/avd/,feat/eos_cli_config_gen/interface-te-admingrp --force
# Optional: Install AVD examples
cd test-avd-pr-4754
ansible-playbook arista.avd.install_examples

@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 labels Nov 22, 2024
@emilarista emilarista marked this pull request as ready for review November 27, 2024 07:11
@emilarista emilarista requested review from a team as code owners November 27, 2024 07:11
Copy link
Contributor

@MaheshGSLAB MaheshGSLAB left a comment

Choose a reason for hiding this comment

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

Firs round of review and same comments applicable for Port-Channel interfaces.

Comment on lines +1206 to +1208
{% if ethernet_interface.traffic_engineering.administrative_groups is arista.avd.defined %}
traffic-engineering administrative-group {{ ethernet_interface.traffic_engineering.administrative_groups | join(",") }}
{% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{% if ethernet_interface.traffic_engineering.administrative_groups is arista.avd.defined %}
traffic-engineering administrative-group {{ ethernet_interface.traffic_engineering.administrative_groups | join(",") }}
{% endif %}
{% if ethernet_interface.traffic_engineering.administrative_groups is arista.avd.defined %}
traffic-engineering administrative-group {{ ethernet_interface.traffic_engineering.administrative_groups | join(",") }}
{% endif %}

We can configure the administrative-group without just configure the only traffic-engineering in EOS. So we can remove the nested if.

@@ -795,6 +795,23 @@
| {{ sync_e_interface.name }} | {{ sync_e_interface.sync_e.priority | arista.avd.default('127') }} |
{% endfor %}
{% endif %}
{% set te_interfaces = [] %}
{% for ethernet_interface in ethernet_interfaces | arista.avd.natural_sort('name') %}
{% if ethernet_interface.traffic_engineering.enabled is arista.avd.defined(true) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{% if ethernet_interface.traffic_engineering.enabled is arista.avd.defined(true) %}
{% if ethernet_interface.traffic_engineering.enabled is arista.avd.defined(true) or traffic_engineering.administrative_groups is arista.avd.defined %}

description: Whether to enable traffic-engineering on this interface.
administrative_groups:
type: list
description: List of traffic-engineering administrative groups, valid values are names, ranges 0-127, or single integers 0-127.
Copy link
Contributor

Choose a reason for hiding this comment

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

It support hexadecimal too.

traffic-engineering administrative-group ?
  <0x0-0xFFFFFFFF>,<0-127>,<0-127>-<0-127>,WORD  Administrative Group value in
                                                 hexadecimal, range or name
                                                 format

@@ -1201,6 +1201,12 @@ interface {{ ethernet_interface.name }}
{{ auth_failure_fallback_mba }}
{% endif %}
{% endif %}
{% if ethernet_interface.traffic_engineering.enabled is arista.avd.defined(true) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

The order is not correct as per EOS. It should come between sync-e and link tracking group

@MaheshGSLAB MaheshGSLAB requested a review from a team November 28, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants