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 match dscp and ecn support to class map type qos #4863

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

Conversation

Vibhu-gslab
Copy link
Contributor

@Vibhu-gslab Vibhu-gslab commented Jan 9, 2025

Change Summary

the current AVD data model does not contain the capability to match class-maps type qos to dscp and ecn.

other options like vlan, cos and ip are available

Related Issue(s)

Fixes #4756

Component(s) name

arista.avd.eos_cli_config_gen

Proposed changes

Added schema and testcases

How to test

molecule run

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)

@Vibhu-gslab Vibhu-gslab self-assigned this 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-4863
# Activate the virtual environment
source test-avd-pr-4863/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/Vibhu-gslab/avd.git@match_dscp_4756#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/Vibhu-gslab/avd.git#/ansible_collections/arista/avd/,match_dscp_4756 --force
# Optional: Install AVD examples
cd test-avd-pr-4863
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 type: code quality CI and development toolset state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated labels Jan 9, 2025
@laxmikantchintakindi laxmikantchintakindi marked this pull request as ready for review January 14, 2025 08:03
@laxmikantchintakindi laxmikantchintakindi requested review from a team as code owners January 14, 2025 08:03
@Vibhu-gslab Vibhu-gslab force-pushed the match_dscp_4756 branch 2 times, most recently from 8bb48db to 399df90 Compare January 15, 2025 09:53
Shivani-gslab
Shivani-gslab previously approved these changes Jan 15, 2025
Copy link
Contributor

@Shivani-gslab Shivani-gslab left a comment

Choose a reason for hiding this comment

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

LGTM!

@Vibhu-gslab Vibhu-gslab marked this pull request as draft January 17, 2025 08:15
@Vibhu-gslab Vibhu-gslab marked this pull request as ready for review January 17, 2025 11:23
Copy link
Contributor

@Shivani-gslab Shivani-gslab left a comment

Choose a reason for hiding this comment

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

LGTM

@Vibhu-gslab Vibhu-gslab force-pushed the match_dscp_4756 branch 2 times, most recently from e5a44f8 to cde98c1 Compare January 22, 2025 15:55
Comment on lines 11892 to 12038
| DSCP_TEST_1 | dscp | af11 |
| DSCP_TEST_2 | dscp | 2-4, 6 |
| DSCP_TEST_3 | dscp | cs0 |
| DSCP_TEST_4 | dscp | ef |
| DSCP_TEST_5 | ecn | ce |
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 have expected to see dscp + ecn on some of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Simplest is to put them on the same line, but it may not look perfect as we accept DSCP ranges with whitespaces which makes it a bit harder (see line 2) to separate visually DSCP from ECN (although still clear as ECN is always alphabetic)

#### QOS Class Maps Summary

| Name | Field | Value |
| ---- | ----- | ----- |
| DSCP_TEST_1 | dscp ecn | af11 ect-ce |
| DSCP_TEST_2 | dscp ecn | 2-4, 6 non-ect |
| DSCP_TEST_3 | dscp | cs0 |
| DSCP_TEST_4 | dscp ecn | ef ce |
| DSCP_TEST_5 | ecn | ce |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I have kept "+", will change if we decide otherwise.

Comment on lines +85 to +82
- non-ect (matches 00).
- ect (matches 01 an 10).
- ce (matches 11).
- ect-ce (matches 01, 10 and 11).
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the EOS help, which seems easier to follow:

smv030(config-s-claus-cmap-qos-claus)#match dscp af11 ecn ?
  ce       ECN codepoint CE
  ect      ECN codepoint ECT(0) or ECT(1)
  ect-ce   ECN codepoint ECT(0), ECT(1) or CE
  non-ect  ECN codepoint Not-ECT

Copy link
Contributor

@alexeygorbunov alexeygorbunov Jan 22, 2025

Choose a reason for hiding this comment

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

I feel like explicitly describing actual binary values associated with each usecase is easier to read than default abstracted EoS terms (like ECT(0) which is actually a decimal value of 1 (binary 01) or ECT(1) which is 2 (10) ).
If we don't want to provide this additional info - we can probably just skip this detailed description as it would just be repeating what EoS is already exposing.

@@ -23,6 +23,12 @@
{% elif class_map.ip.access_group is arista.avd.defined %}
{% set row.type = 'acl' %}
{% set row.value = class_map.ip.access_group %}
{% elif class_map.dscp is arista.avd.defined %}
Copy link
Contributor

Choose a reason for hiding this comment

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

All this would be simpler implemented by just checking for the var and print out the line directly. You can then do elif on all the first ones and then nest dscp and ecn under an else block and check those with a regular if inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@Vibhu-gslab Vibhu-gslab marked this pull request as draft January 23, 2025 07:40
@Vibhu-gslab Vibhu-gslab marked this pull request as ready for review January 23, 2025 09:20
Comment on lines 1 to 6
{
"name": "avd",
"lockfileVersion": 3,
"requires": true,
"packages": {}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

NPM? Is this file relevant to this PR?

Copy link
Contributor Author

@Vibhu-gslab Vibhu-gslab Jan 24, 2025

Choose a reason for hiding this comment

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

Nope, removed it.
Thanks, I missed it somehow.

@@ -15,19 +15,22 @@
{% for class_map in class_maps.qos | arista.avd.natural_sort('name') %}
{% set row = namespace() %}
{% if class_map.cos is arista.avd.defined %}
{% set row.type = 'cos' %}
{% set row.value = class_map.cos %}
| {{ class_map.name }} | cos | {{ class_map.cos }} |
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this row namespace is not use anymore and can be removed

{% set row = namespace() %}

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 type: code quality CI and development toolset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add "match dscp" support to class map type qos in eos_cli_config_gen
7 participants