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,eos_designs): Add support for 'l2 mru' #3164

Merged
merged 9 commits into from Nov 13, 2023

Conversation

m-rhode
Copy link
Contributor

@m-rhode m-rhode commented Sep 25, 2023

Change Summary

add support for setting the "l2 mru" on interfaces and port-channels

Related Issue(s)

none

Component(s) name

arista.avd.eos_cli_config_gen

Proposed changes

I duplicated all "l2_mtu" assets and replaced "mtu" with mru"

How to test

did a config generation and deployment with the branch:

r2-lab2-de#show int eth14 | grep MRU
  IP MTU 1500 bytes (default), Ethernet MRU 10240 bytes, BW 10000000 kbit

ansible run:

ok: [r2-lab2-de] => 
  msg:
  - |-
    --- system:/running-config
    +++ session:/1481195311-session-config
    interface Ethernet14
    +   l2 mru 9000
r2-lab2-de#show int eth14 | grep MRU
  IP MTU 1500 bytes (default), Ethernet MRU 9000 bytes, BW 10000000 kbit

Checklist

User Checklist

ansible_collections/arista/avd/roles/eos_cli_config_gen](support-l2-mru) $ grep -r "l2_mtu\|l2_mru"

then eyeballed if every l2_mtu has a l2_mru equivalent (actually good that I did :) )

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)

@m-rhode m-rhode requested review from a team as code owners September 25, 2023 13:52
@github-actions github-actions bot added role: eos_cli_config_gen issue related to eos_cli_config_gen role state: Documentation role Updated role: eos_designs issue related to eos_designs role labels Sep 25, 2023
@m-rhode m-rhode changed the title Add support for 'l2 mru' Feat(eos_cli_config_gen): Add support for 'l2 mru' Sep 25, 2023
@carlbuchmann carlbuchmann marked this pull request as draft September 25, 2023 20:36
@carlbuchmann carlbuchmann changed the title Feat(eos_cli_config_gen): Add support for 'l2 mru' Feat(eos_cli_config_gen,eos_designs): Add support for 'l2 mru' Sep 25, 2023
@carlbuchmann carlbuchmann changed the title Feat(eos_cli_config_gen,eos_designs): Add support for 'l2 mru' Feat(eos_designs,eos_cli_config_gen): Add support for 'l2 mru' Sep 25, 2023
@carlbuchmann
Copy link
Member

@m-rhode thank you for this contribution, I converted it back to draft as you also need to include molecule unit tests for eos_designs and eos_cli_config_gen.
I see you are a first-time contributor to the project, and can assist on a zoom if required. Please contact me @ carl.buchmann@arista.com.

@carlbuchmann
Copy link
Member

carlbuchmann commented Sep 26, 2023

Also, you need to generate documentation. This is done by running pre-commit locally. Install pre-commit and then run:
pre-commit run shemas --all

@m-rhode
Copy link
Contributor Author

m-rhode commented Sep 26, 2023

@m-rhode thank you for this contribution, I converted it back to draft as you also need to include molecule unit tests for eos_designs and eos_cli_config_gen. I see you are a first-time contributor to the project, and can assist on a zoom if required. Please contact me @ carl.buchmann@arista.com.

Hello @carlbuchmann, thanks for the zoom offer, much appreciated!
Let's first try without. I assume it's basically reproducing the l2_mtu config under "ansible_collections/arista/avd/molecule" for the molecule stuff. And as this is not tested on real-life hardware I could simply use the same ports as they are used for the l2_mtu - I suppose?

Is there anything to do in "ansible_collections/arista/avd/roles/eos_designs"... as l2_mru` is not part of a design I suppose not?

Also, you need to generate documentation. This is done by running pre-commit locally. Install pre-commit and then run:
pre-commit run shemas --all

thanks for pointing this out...

@gmuloc
Copy link
Contributor

gmuloc commented Sep 26, 2023

Hello @m-rhode

Answering some of the questions here as @carlbuchmann is in another timezone

So in general we try to avoid introducing some new knobs in eos_cli_config_gen and eos_designs in one go to make things clearer. Usually, we just recommend one PR to add the low-level CLI in eos_cli_config_gen and then a new one on top to do the eos_designs bit. Thus I will structure a list of tasks based on this assumed separation and tick the steps you have followed already.

Make sure to install the requirements-dev.txt to be able to complete the process (I believe you have already but mentioning it just in case)

eos_cli_config_gen

This part of the change is aimed at adding some new knobs. The following tasks are required:

  • Adding the knob to the appropriate schema fragments file in the roles/eos_cli_config_gen/schema/schema_fragments directory (or create a new file if adding a top-level feature)
  • Editing the Jinja2 templates in roles/eos_cli_config_gen/templates/eos and roles/eos_cli_config_gen/templates/documentation (or adding new template if adding a top-level feature, in that case, you should also modify the roles/eos_cli_config_gen/templates/eos-device-documentation.j2 and ``roles/eos_cli_config_gen/templates/eos-intended-config.j2 to add these new templates, trying to respect the order in the EOS CLI)
    • NOTE: This one is partially done in your PR as you have modified only the ethernet-interface.j2 template for config and not the port-channel.j2 one where you have added the l2mru knob in the schema. Also for now you have not added anything to the documentation templates (it may or may not be necessary)
  • Run pre-commit run shemas --all to re-generate the eos_cli_config_gen schema with your modifications (you need to do this every time you change the schema, even if this is just a description changed). This is also used to add the new options to our documentation.
  • Add some molecule test in n the molecule/eos_cli_config_gen scenario exercising your new knob configuration
    • NOTE: In your case you would need to add to both molecule/eos_cli_config_gen/inventory/host_vars/ethernet-interfaces.yml and molecule/eos_cli_config_gen/inventory/host_vars/port-channel-interfaces.yml (re-using existing interfaces is fine as this is only for testing purposes)
  • Run the molecule test locally to generate the new expected configuration and documentation for these two hosts (we recommend running the full scenario using from ansible_collections/arista/avd/molecule folder the command make converge MOLECULE=eos_cli_config_gen
  • Run all pre-commit checks pre-commit run --all to make sure that all files you have added are correct and won't be rejected by CI (this command will take ~1min)
  • Review that everything added looks correct and push to Github!

eos_designs

Is there anything to do in "ansible_collections/arista/avd/roles/eos_designs"... as l2_mru` is not part of a design I suppose not?

I just would like to point out that if you don't need the l2_mru in eos_designs you should remove the changes you have made to the python code there. Otherwise, there is a bit more work to do indeed. This can be done only after the eos_cli_config_gen part has been completed (hence why we usually recommend 2 PRs)

The process is very similar (for simple changes like yours) except for the jinja2 template part where you need to edit some Python code

  • Adding the knob to the appropriate schema fragments file in the roles/eos_designs/schema/schema_fragments directory
  • Editing the relevant python_modules
    • NOTE: This has been done for you but you now need to add the knob to the eos_designs schema for adapters or it will never be possible to use it
  • Run pre-commit run shemas --all to re-generate the eos_designs schema with your modifications (you need to do this every time you change the schema, even if this is just a description changed). This is also used to add the new options to our documentation.
  • Add some molecule test in n the molecule/eos_designs_unit_tests scenario exercising your new knob configuration
    • NOTE: In your case you would need to add the l2mru knob to some adapters, one for port-channel and one for ethertnet-interfaces. It may be simpler in your case to add to the molecule/eos_designs_unit_tests/inventory/host_vars/connected_endpoints.yml file
  • Run the molecule test locally to generate the new expected configuration and documentation for this two hosts (we recommend running the full scenario using from ansible_collections/arista/avd/molecule folder the command make converge MOLECULE=eos_designs_unit_tests
  • Run all pre-commit checks pre-commit run --all to make sure that all files you have added are correct and won't be rejected by CI (this command will take ~1min)
  • Review that everything added looks correct and push to Github!

Let us know if you need further help

@gmuloc gmuloc mentioned this pull request Oct 23, 2023
14 tasks
eos-1(config-if-Et1)#mtu ?
  <68-65535>  Maximum transmission unit in bytes
eos-1(config-if-Et1)#l2 mtu ?
  <64-65535>  L2 MTU (bytes)
eos-1(config-if-Et1)#l2 mru ?
  <68-65535>  L2 MRU (bytes)
carlbuchmann

This comment was marked as outdated.

@carlbuchmann carlbuchmann self-requested a review November 2, 2023 02:01
@carlbuchmann carlbuchmann dismissed their stale review November 2, 2023 02:03

approved by mistake

@m-rhode
Copy link
Contributor Author

m-rhode commented Nov 2, 2023

Dear @carlbuchmann and @gmuloc ,
thanks a lot for your explanations, really appreciated. I was on PTO and today is my first day back. I'd have taken care of working further on this but as it seems I was to slow. Sorry for that and thanks for implementing this!

@github-actions github-actions bot added the state: conflict PR with conflict label Nov 11, 2023
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 Nov 13, 2023
Copy link

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

@ClausHolbechArista ClausHolbechArista marked this pull request as ready for review November 13, 2023 09:25
@ClausHolbechArista
Copy link
Contributor

@carlbuchmann @m-rhode I have adjusted the schema a bit for min/max. For l2_mtu the min is 68 on all the device types I could find. This was set to 64, so please let me know if you have a platform saying 64.

@github-actions github-actions bot added the state: CI Updated CI scenario have been updated in the PR label Nov 13, 2023
@ClausHolbechArista ClausHolbechArista changed the title Feat(eos_designs,eos_cli_config_gen): Add support for 'l2 mru' Feat(eos_cli_config_gen): Add support for 'l2 mru' Nov 13, 2023
@ClausHolbechArista ClausHolbechArista changed the title Feat(eos_cli_config_gen): Add support for 'l2 mru' Feat(eos_cli_config_gen,eos_designs): Add support for 'l2 mru' Nov 13, 2023
@m-rhode
Copy link
Contributor Author

m-rhode commented Nov 13, 2023

@ClausHolbechArista thanks for working on this. Min 68 is fine for us.

@ClausHolbechArista ClausHolbechArista requested a review from a team November 13, 2023 11:58
@ClausHolbechArista ClausHolbechArista added the one approval This PR has one approval and is only missing one more. label Nov 13, 2023
Copy link
Contributor

@gmuloc gmuloc left a comment

Choose a reason for hiding this comment

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

LGTM

@gmuloc gmuloc merged commit 927b77c into aristanetworks:devel Nov 13, 2023
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
one approval This PR has one approval and is only missing one more. rn: Feat(eos_cli_config_gen|eos_designs) role: eos_cli_config_gen issue related to eos_cli_config_gen role role: eos_designs issue related to eos_designs 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.

None yet

4 participants