Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Draft
wants to merge 1 commit into
base: devel
Choose a base branch
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
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

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.

"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

@@ -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

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 ?

Copy link

sonarcloud bot commented Nov 4, 2024

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Node Type Keys specific to Media
3 participants