Skip to content

evpn esi multihome model #301

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

aida-shumburo
Copy link
Contributor

No description provided.

'supported_by': 'community'
}
NETWORK_OS: sonic
RESOURCE: snmp
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
RESOURCE: snmp
RESOURCE: evpn_esi_multihome

COPYRIGHT: Copyright 2025 Dell Inc. or its subsidiaries. All Rights Reserved.
DOCUMENTATION: |
module: sonic_evpn_esi_multihome
version_added: 4.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version_added: 4.5
version_added: 3.1.0

DOCUMENTATION: |
module: sonic_evpn_esi_multihome
version_added: 4.5
short_description: Manage SNMP configuration on SONiC
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
short_description: Manage SNMP configuration on SONiC
short_description: Manage EVPN ESI multihoming configuration on SONiC

version_added: 4.5
short_description: Manage SNMP configuration on SONiC
description:
- This module provides configuration management of SNMP for devices running SONiC
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- This module provides configuration management of SNMP for devices running SONiC
- This module provides configuration management of EVPN ESI multihoming for devices running SONiC

options:
config:
description:
- evpn esi multihome configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- evpn esi multihome configuration
- EVPN ESI multihoming configuration

- evpn esi multihome configuration
type: dict
suboptions:
mac-holdtime:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mac-holdtime:
mac_holdtime:

attribute names should use underscores and not hyphens

suboptions:
mac-holdtime:
description:
- mac holdtime
Copy link
Contributor

@stalabi1 stalabi1 Apr 22, 2025

Choose a reason for hiding this comment

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

Suggested change
- mac holdtime
- MAC hold time in seconds, range 0-86400

type: int
neigh-holdtime:
description:
- neigh holdtime
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- neigh holdtime
- Neighbor hold time in seconds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

type: int
startup-delay:
description:
- startup delay
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- startup delay
- Startup delay in seconds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add df_election_time and es_activation_delay as they are supported in CLI. Also, please add ranges for all attributes of type int.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

# show running-configuration evpn-mh
#
# evpn esi-multihoming
# mac-holdtime 1080
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# mac-holdtime 1080

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

#
# evpn esi-multihoming
# mac-holdtime 1080
# neigh-holdtime 1080
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# neigh-holdtime 1080

Replaced state should replace the entire existing config dictionary with the new config dictionary specified in the playbook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

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

Please change the hyphens (-) to underscores (_) for the Ansible playbook option names in all applicable examples. (Please be careful to leave the hyphens "as is" in the "show running-configuration" excerpts. They just need to be changed for the corresponding Ansible options for the examples.)

Copy link
Contributor

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

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

Thank you for creating the foundation for this new resource module.

The content of the current proposed model files looks good.

Approved.

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

Successfully merging this pull request may close these issues.

3 participants