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

Number of extra fabric settings plus a couple of bug fixes #275

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

mthurstocisco
Copy link
Collaborator

@mthurstocisco mthurstocisco commented Feb 7, 2025

Related Issue(s)

Related Collection Role

  • cisco.nac_dc_vxlan.validate
  • cisco.nac_dc_vxlan.dtc.create
  • cisco.nac_dc_vxlan.dtc.deploy
  • cisco.nac_dc_vxlan.dtc.remove
  • other

Related Data Model Element

  • vxlan.global
  • vxlan.topology
  • vxlan.underlay
  • vxlan.overlay_services
  • vxlan.overlay_extensions
  • vxlan.policy
  • defaults.vxlan
  • other

Proposed Changes

Test Notes

Cisco NDFC Version

Checklist

  • Latest commit is rebased from develop with merge conflicts resolved
  • New or updates to documentation has been made accordingly
  • Assigned the proper reviewers

@mthurstocisco mthurstocisco requested a review from a team as a code owner February 7, 2025 10:53
@mthurstocisco mthurstocisco added bug Something isn't working enhancement New feature or request labels Feb 7, 2025
@@ -1,4 +1,4 @@
{# Auto-generated NDFC DC VXLAN EVPN General config data structure for fabric {{ vxlan.global.name }} #}
BGP_AS: {{ global.bgp_asn }}
BGP_AS: "{{ global.bgp_asn }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's your data type in the data model for this now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Still using a string

ptp:
ptp_enable: false
enable_nxapi: true
enable_nxapi_http: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we default NXAPI HTTP to false and disabled? Even on the switch nowadays, http is disabled by default whereas https is enabled by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Happy to change to whatever, we were just mirroring the defaults in NDFC, should we diverge and make different decisions to the tool?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated to false

Comment on lines +63 to +77
- name: Config-Save block
block:
- name: Config-Save for Fabric {{ MD.vxlan.global.name }}
cisco.dcnm.dcnm_rest:
method: POST
path: "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/control/fabrics/{{ MD.vxlan.global.name }}/config-save"
when: MD_Extended.vxlan.topology.switches | length > 0
register: config_save
# TODO: Need to add logic to only save if changes are made

rescue:
- name: Config-Save for Fabric {{ MD.vxlan.global.name }} - Failed
ansible.builtin.debug:
msg: "{{ config_save.msg.DATA }}"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I know what this is and @mikewiebe was talking to me about this some, but want to understand this better in our standups.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, basic reason for save at this state is so that NDFC can do its easy fabric stuff (around both links and VPC config), happy to run through it though next time we chat

Comment on lines +31 to +52
# --------------------------------------------------------------------
# Manage Interface Trunk Configuration on NDFC
# --------------------------------------------------------------------

- name: Manage Interface Trunk
cisco.dcnm.dcnm_interface:
fabric: "{{ MD.vxlan.global.name }}"
state: replaced
config: "{{ interface_trunk }}"
when: MD_Extended.vxlan.topology.interfaces.modes.trunk.count > 0

# --------------------------------------------------------------------
# Manage Interface Access Routed Configuration on NDFC
# --------------------------------------------------------------------

- name: Manage Interface Access
cisco.dcnm.dcnm_interface:
fabric: "{{ MD.vxlan.global.name }}"
state: replaced
config: "{{ interface_access }}"
when: MD_Extended.vxlan.topology.interfaces.modes.access.count > 0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this net new for trunks and access interfaces?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

shouldn't be new, just moved around the ordering (basically if the member ports from a PO don't exist then you cant add them to the PO, again, all this for pre provisioning)

Comment on lines +8 to +28
{% if MD_Extended.vxlan.topology.leaf is defined and vpc_peer.peer1 in MD_Extended.vxlan.topology.leaf %}
{% if MD_Extended.vxlan.topology.leaf[vpc_peer.peer1].management_ipv4_address is not none %}
- peerOneId: {{ MD_Extended.vxlan.topology.leaf[vpc_peer.peer1].management_ipv4_address }}
{% elif MD_Extended.vxlan.topology.leaf[vpc_peer.peer1].management_ipv6_address is not none %}
- peerOneId: {{ MD_Extended.vxlan.topology.leaf[vpc_peer.peer1].management_ipv6_address }}
{% else %}
- peerOneId: {{ vpc_peer.peer1 }}
{% endif %}
{% else %}
- peerOneId: {{ vpc_peer.peer1 }}
{% endif %}
{% if MD_Extended.vxlan.topology.leaf is defined and vpc_peer.peer2 in MD_Extended.vxlan.topology.leaf %}
{% if MD_Extended.vxlan.topology.leaf[vpc_peer.peer2].management_ipv4_address is not none %}
peerTwoId: {{ MD_Extended.vxlan.topology.leaf[vpc_peer.peer2].management_ipv4_address }}
{% elif MD_Extended.vxlan.topology.leaf[vpc_peer.peer2].management_ipv6_address is not none %}
peerTwoId: {{ MD_Extended.vxlan.topology.leaf[vpc_peer.peer2].management_ipv6_address }}
{% else %}
peerTwoId: {{ vpc_peer.peer2 }}
{% endif %}
{% else %}
peerTwoId: {{ vpc_peer.peer2 }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, for this template, there's an action plugin that is setting up the index by role that we actually need to refactor and not do it that way as it limits things to only leaf roles now for some reason.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lets discuss more in person, think @peter8498 can speak more to this one

Comment on lines +37 to +40
peer1_cmds: |2-
{{ switches[peer1].freeform_config | default('') | indent(6, false) }}
peer2_cmds: |2-
{{ switches[peer2].freeform_config | default('') | indent(6, false) }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just want to make sure I understand this in next standup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just adding support for freeform under each peer

Comment on lines +46 to +48
{% if (vxlan.underlay.general.replication_mode | default(defaults.vxlan.underlay.general.replication_mode) ) == 'multicast' %}
BFD_PIM_ENABLE: {{ vxlan.underlay.bfd.pim | default(defaults.vxlan.underlay.bfd.pim) }}
{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a bunch of refactor in PR #268 for IPv6 and mcast; lets just make sure we don't create a conflict here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looks like we are setting BRD_PIM_ENABLE in very different circumstances from reading your PR, lets discuss on a call

Comment on lines 14 to 16
{% if global.bootstrap is defined and global.bootstrap.cdp_enable is defined %}
CDP_ENABLE: global.bootstrap.cdp_enable
{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this only apply to bootstrap?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

Yeah this is only to set CDP to on on the management interface at bootstrap, just tested on an already bootstrapped fabric, doesn't attempt to change any existing switch config, just a badly named key on the NDFC side

Comment on lines +38 to +40
fabric_vpc_qos: false
fabric_vpc_qos_policy_name: spine_qos_for_fabric_vpc_peering
enable_ipv6_nd_sync: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just general comment, don't forget updating defaults in schema repo.

@peter8498 peter8498 requested a review from mtarking February 24, 2025 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants