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.

Sign up for GitHub

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

Jump to bottom

Feat: New node type keys for m&e devices #4485

Draft
wants to merge 1 commit into
base: devel
Choose a base branch
from
Draft

Feat: New node type keys for m&e devices #4485

wants to merge 1 commit into from

Conversation

sugetha24
Copy link
Contributor

Change Summary

Introducing a new default Node type Keys for M&E devices

Related Issue(s)

Fixes #4484

Component(s) name

arista.avd.eos_designs

Proposed changes

node_type_keys:
  - key: ptp_leaf
    type: ptp_leaf
    default_overlay_routing_protocol: none
    connected_endpoints: true
    network_services:
      l2: true
      l3: true
    default_ptp_priority1: 10
  - key: media_spine
    type: media_spine
    default_overlay_routing_protocol: none
    connected_endpoints: true
    network_services:
      l2: true
      l3: true
    default_ptp_priority1: 20
  - key: media_leaf
    type: media_leaf
    default_overlay_routing_protocol: none
    connected_endpoints: true
    network_services:
      l2: true
      l3: true
    default_ptp_priority1: 30```

## How to test
Please test with this PR.
https://github.com/aristanetworks/avd/pull/3048

## Checklist

### Repository Checklist

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [x] My code has been rebased from devel before I start
- [x] I have read the [**CONTRIBUTING**](https://avd.sh/en/latest/docs/contribution/overview.html) document.
- [ ] My change requires a change to the documentation and documentation have been updated accordingly.
- [ ] I have updated [molecule CI](https://github.com/aristanetworks/avd/tree/devel/ansible_collections/arista/avd/molecule) testing accordingly. (check the box if not applicable)

@sugetha24 sugetha24 self-assigned this Sep 17, 2024
@github-actions GitHub Actions
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-4485
# Activate the virtual environment
source test-avd-pr-4485/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/sugetha24/ansible-avd.git@media_node_type_keys#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/sugetha24/ansible-avd.git#/ansible_collections/arista/avd/,media_node_type_keys --force
# Optional: Install AVD examples
cd test-avd-pr-4485
ansible-playbook arista.avd.install_examples

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 17, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

ClausHolbechArista
ClausHolbechArista reviewed Sep 18, 2024
View reviewed changes
Copy link
Contributor

@ClausHolbechArista ClausHolbechArista left a comment

Choose a reason for hiding this comment

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

Please also add these new types to the docs. There is a table of node types and a snip of yaml showing the default values.

python-avd/pyavd/_eos_designs/shared_utils/node_type_keys.py
"type": "ptp_leaf",
"connected_endpoints": True,
"network_services": {
"l2": True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need L2 on all device types? Do you have a design I can look at?

Copy link

Choose a reason for hiding this comment

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

Yes PTP distribution leafs can also have l2 connected endpoints . Slide 13 here is an example where PTP leaf is also a media_leaf which has connected endpoints which can be a mix of both L2 and/or L3. https://docs.google.com/presentation/d/1XyV6eMJpGsNNP9bxZaxJ25e2VBK96W2rOyHbwZgKanw/edit#slide=id.g1a2c972f82e_0_204

python-avd/pyavd/_eos_designs/shared_utils/node_type_keys.py
@@ -174,6 +173,39 @@
"l3": True,
},
},
{
"key": "ptp_leaf",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not be prefixed with media_ as well?

Copy link

@sarunac sarunac Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

would it be okay to leave the media_ so non-media PTP customers can consume it too ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarunac sarunac sarunac left review comments

@ClausHolbechArista ClausHolbechArista ClausHolbechArista left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees

@sugetha24 sugetha24

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New Node Type Keys specific to Media
3 participants
@sugetha24 @sarunac @ClausHolbechArista