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,eos_cli_config_gen): Recirculation Channel Interface #4554

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

Conversation

nathanmusser
Copy link
Contributor

@nathanmusser nathanmusser commented Oct 4, 2024

Change Summary

Add support for recirculation channel interfaces for cpu-mirroring and adding a front panel interface to the recirculation.

Related Issue(s)

Fixes #4495

Component(s) name

arista.avd.eos_designs
arista.avd.cli_config_gen

Proposed changes

Add recirculation_interfaces model to eos_cli_config_gen to generate the recirculation interface. Add the traffic-loopback source system device mac to ethernet_interfaces model. Add the ability to put an ethernet interface into a recirculation channel group. Potentially expose all this in the platform settings so it can be configured per platform with an easy toggle.

How to test

TBD

Checklist

User Checklist

  • Add recirculation_interfaces data model
  • Add recirculation_interfaces documentation
  • Add recirculation_interfaces molecule testing
  • Add traffic-loopback key to ethernet_interfaces
  • Add traffic-loopback key documentation
  • Add traffic-loopback key molecule testing
  • Add recirc channel group assignment
  • Add recirc channel group documentation
  • Add recirc channel group molecule testing

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

github-actions bot commented Oct 4, 2024

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-4554
# Activate the virtual environment
source test-avd-pr-4554/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/nathanmusser/avd.git@issue4495_recirc_channel#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/nathanmusser/avd.git#/ansible_collections/arista/avd/,issue4495_recirc_channel --force
# Optional: Install AVD examples
cd test-avd-pr-4554
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 Oct 4, 2024
@nathanmusser
Copy link
Contributor Author

When adding an interface to the recirculation channel you use the syntax channel-group recirculation x but our channel_group data model under ethernet_interfaces is

    channel_group:
      id: <int>
      mode: <str; "on" | "active" | "passive">

I'm suggesting we add a new key recirculation to the channel group, and the associated logic to the template to render the channel-group as it currently is when false, and when true add the recirculation keyword before the ID.

    channel_group:
      id: <int>
      mode: <str; "on" | "active" | "passive">
      recirculation: <bool>

I've already added the recirculation_channel interface. I generally referenced the format of other interface types (port-channel, dps, etc..) when structuring the data model. But one thing I don't love is that we're keying off a user supplied interface "name" but there's no validation that the user is supplying a valid name for a recirculation interface. But none of the other interface types seem to validate this either. As an example a user could create a tunnel interface using the name "Recirc1" and eos_cli_config_gen would allow it but it would have options under it that would not be accepted on the switch. I've seen the pattern: key in the schema used elsewhere but I don't see it on any of the interfaces and wasn't sure if there was a reason why.

I still wouldn't really like the pattern: though, as we now define the interface as Recirc-channel x in one place but refer to it later as just the integer ID. This appears to be how all the other interface types are done in AVD but makes for more friction between the structured_config and other tooling that might want to interact with it. It seems to me that since we know the interface name is always going to start with "Recirc-channel" that doesn't need to be provided by the user and instead the name field could just be id. I didn't want to deviate from the established pattern though with asking about it.

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.

First round of review

@nathanmusser
Copy link
Contributor Author

nathanmusser commented Oct 7, 2024

When implementing the traffic-loopback key, for my purposes I only really need a traffic-loopback source system device mac flag. When creating the key should I create the structure to represent the options available at the CLI that I don't plan to implement?

sw0-arista(config-if-Et3)#show cli commands | inc traffic-loopback
[no|default] traffic-loopback source ( ( system device ( SYSTEMPHY | mac ) ) | ( network device NETWORKPHY ) )

My current plan was to implement the schema as:

# ...
  ethernet_interfaces:
    type: list
    primary_key: name
    items:
      type: dict
      keys:
        # ...
        traffic_loopback_source:
          type: dict
          keys:
            system_device_mac:
              type: bool
              description: "Implement loopback in MAC layer."

To allow for the potential of the rest of the CLI structure being fleshed out later if needed.

@github-actions github-actions bot added the state: conflict PR with conflict label Oct 11, 2024
Copy link

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

@github-actions github-actions bot removed the state: conflict PR with conflict label Oct 14, 2024
Copy link

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link

sonarcloud bot commented Oct 14, 2024

@github-actions github-actions bot added the state: conflict PR with conflict label Oct 30, 2024
Copy link

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

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: conflict PR with conflict state: Documentation role Updated
Projects
None yet
3 participants