[DMA] add UMA and LMA min topo - #26742
Conversation
This file defines the topology and configuration properties for a single-node setup with 6 ports, including details for VMs and DUT.
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
🟡 Not ready to approve
topo_uma_single_node_6ports.yml lacks explicit VM device_type properties, causing minigraph generation to misclassify LMA/M1 neighbors based on name heuristics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR introduces two new Ansible topology variable files under ansible/vars/ to enable single-node, 6-port DMA testbed configurations for Upper and Lower Management Aggregator scenarios, including VM VLAN mappings, BGP peerings (with confederation), and interface addressing.
Changes:
- Added
topo_uma_single_node_6ports.ymldefining an UpperMgmtAggregator DUT with RWA/LMA/M1 neighbor VM roles and BGP/interface details. - Added
topo_lma_single_node_6ports.ymldefining a LowerMgmtAggregator DUT with UMA/M2/M3 neighbor VM roles, including explicitdevice_typeproperties for neighbor typing.
File summaries
| File | Description |
|---|---|
| ansible/vars/topo_uma_single_node_6ports.yml | New UMA single-node 6-port topology; needs explicit neighbor device_type properties to avoid incorrect minigraph typing. |
| ansible/vars/topo_lma_single_node_6ports.yml | New LMA single-node 6-port topology with BGP confederation and neighbor role/device typing. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
🟡 Not ready to approve
The new uma_*/lma_* topology names and the added EOS template filename don’t align with existing base_topo derivation/validation and template selection logic, so standard playbooks will reject or fail to render these topologies.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The new UMA topo has no corresponding EOS templates and the LMA templates won’t be selected for cEOS due to base_topo template naming, causing provisioning failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
ansible/roles/eos/templates/lma_single_node_6ports-leaf.j2:5
- The PR description says it only adds two new topology variable files, but it also introduces new EOS Jinja templates under ansible/roles/eos/templates/. Please update the PR description to include these additional deliverables so reviewers/operators understand the full scope of the change.
{# Converged (multi-VRF) topologies render the shared converged config; the
stock body below is used unchanged on non-converged topologies. #}
{% if topo_is_multi_vrf | default(false) | bool %}
{% include 'ceos_converged.j2' %}
{% else %}
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
🟡 Not ready to approve
topo_uma.yml currently lacks explicit device_type for the *LMA* VMs, which causes minigraph neighbor-type inference to misclassify them (and the PR description also doesn’t match the delivered filenames/scope).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (6)
ansible/vars/topo_uma.yml:79
- This VM is named
*LMA*but currently uses the genericleafproperties (nodevice_type), which makes minigraph device type fall back to name heuristics ('MA' in dev→MgmtSpineRouter). If this VM is intended to be a LowerMgmtAggregator neighbor, switch it to use the dedicatedlmaproperty set (withdevice_type: LowerMgmtAggregator).
properties:
- common
- leaf
ansible/vars/topo_uma.yml:102
- This VM is named
*LMA*but currently uses the genericleafproperties (nodevice_type), which makes minigraph device type fall back to name heuristics ('MA' in dev→MgmtSpineRouter). If this VM is intended to be a LowerMgmtAggregator neighbor, switch it to use the dedicatedlmaproperty set (withdevice_type: LowerMgmtAggregator).
properties:
- common
- leaf
ansible/vars/topo_uma.yml:125
- This VM is named
*LMA*but currently uses the genericleafproperties (nodevice_type), which makes minigraph device type fall back to name heuristics ('MA' in dev→MgmtSpineRouter). If this VM is intended to be a LowerMgmtAggregator neighbor, switch it to use the dedicatedlmaproperty set (withdevice_type: LowerMgmtAggregator).
properties:
- common
- leaf
ansible/vars/topo_uma.yml:148
- This VM is named
*LMA*but currently uses the genericleafproperties (nodevice_type), which makes minigraph device type fall back to name heuristics ('MA' in dev→MgmtSpineRouter). If this VM is intended to be a LowerMgmtAggregator neighbor, switch it to use the dedicatedlmaproperty set (withdevice_type: LowerMgmtAggregator).
properties:
- common
- leaf
ansible/vars/topo_uma.yml:1
- PR description references new topology var files
ansible/vars/topo_uma_single_node_6ports.ymlandansible/vars/topo_lma_single_node_6ports.yml, but the actual added files areansible/vars/topo_uma.ymlandansible/vars/topo_lma.yml(and this PR also adds multiple EOS templates). Please either update the PR description to match the delivered filenames/scope, or rename the files/templates accordingly sotoposelection remains clear.
topology:
ansible/vars/topo_uma.yml:51
VM02LMA..VM05LMAwill be inferred asMgmtSpineRouterin minigraph generation because their names contain the substringMAand no explicitdevice_typeis set via properties. This likely mislabels LowerMgmtAggregator neighbors (seeansible/templates/minigraph_png.j2heuristics) and can break tests/logic that rely on correct neighbor types. Consider adding a dedicated property (e.g.,lma) withdevice_type: LowerMgmtAggregatorand applying it to the LMA VMs.
This issue also appears in the following locations of the same file:
- line 77
- line 100
- line 123
- line 146
core:
swrole: core
leaf:
swrole: leaf
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Reviewed this together with #26639, #26723, #26724 and #26750, since they form one dependency chain. Confirmed correct
Issue:
|
Added new roles for 'lma' and 'm1' with their respective device types.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
🟡 Changes recommended
In topo_uma.yml, several VMs reference the generic leaf property set instead of the defined lma/m1 property sets, so the intended device_type values are never applied via props expansion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (5)
ansible/vars/topo_uma.yml:109
configuration_propertiesdefines anlmaproperty set (withdevice_type: LowerMgmtAggregator), but VM03LMA references the genericleafproperty set instead, so the intendeddevice_typenever gets applied viapropsexpansion.
properties:
- common
- lma
bgp:
asn: 64589
ansible/vars/topo_uma.yml:132
configuration_propertiesdefines anlmaproperty set (withdevice_type: LowerMgmtAggregator), but VM04LMA references the genericleafproperty set instead, so the intendeddevice_typenever gets applied viapropsexpansion.
properties:
- common
- lma
bgp:
asn: 64589
ansible/vars/topo_uma.yml:155
configuration_propertiesdefines anlmaproperty set (withdevice_type: LowerMgmtAggregator), but VM05LMA references the genericleafproperty set instead, so the intendeddevice_typenever gets applied viapropsexpansion.
properties:
- common
- lma
bgp:
asn: 64589
ansible/vars/topo_uma.yml:178
configuration_propertiesdefines anm1property set (withdevice_type: MgmtLeafRouter), but VM06M1 references the genericleafproperty set instead, so the intendeddevice_typenever gets applied viapropsexpansion.
properties:
- common
- m1
bgp:
peer_in_bgp_confed: true
ansible/vars/topo_uma.yml:85
configuration_propertiesdefines anlmaproperty set (withdevice_type: LowerMgmtAggregator), but VM02LMA references the genericleafproperty set instead, so the intendeddevice_typenever gets applied viapropsexpansion.
This issue also appears in the following locations of the same file:
- line 105
- line 128
- line 151
- line 174
properties:
- common
- lma
bgp:
asn: 64589
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
Only a minor maintainability nit was found (unused config property block) with no functional impact on topology rendering.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
ansible/vars/topo_lma.yml:58
configuration_propertiesdefines aleafproperty set that is never referenced by any node’spropertieslist in this topology file, so it has no effect and can confuse future edits (only entries listed underconfiguration.*.propertiesare merged intoprops). Consider removing this unused block (or start referencing it explicitly from nodes if it’s intended to be used).
device_type: UpperMgmtAggregator
leaf:
swrole: leaf
m2:
swrole: leaf
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Re-reviewed after the latest push. The Confirmed fixed
Blocker: no routes will be announced on lma/uma testbeds
pattern = re.compile(
r'^(t0-mclag|t0|t1|ptf|fullmesh|dualtor|t2|mgmttor|m0|mc0|mx|m1|c0|dpu|smartswitch-t1|lt2|ft2|lrh|urh)')
match = pattern.match(topo_name)
if not match:
return "unsupported"
else:
module.exit_json(
msg='Unsupported topology "{}" - skipping announcing routes'.format(topo_name))
That matters for the rest of the chain: the BGP tests being enabled in #26750 would come up with sessions established but no prefixes learned. There's a light-touch precedent right there in the same function if a dedicated route generator isn't wanted yet: # Temporarily map DRH topos to T2 routes
if topo_type in ['lrh', 'urh']:
topo_type = 't2'So either add Nit: dead
|
|
Cherry-pick PR to msft-202603: Azure/sonic-mgmt.msft#1346 |
This pull request introduces two new management network topologies—UMA and LMA—by adding their variable files and corresponding Jinja2 templates for both core and leaf devices. The templates support both converged (multi-VRF) and non-converged topologies, and implement detailed device and interface configuration logic, including BGP, interface, and management settings.
The most important changes are:
New Topology Variable Files:
topo_uma.ymlandtopo_lma.ymlwith detailed definitions for VMs, DUT loopbacks, configuration properties, and per-device interface and BGP settings. These files describe the network layout and parameters for the UMA and LMA topologies. [1] [2]Jinja2 Templates for Device Configuration:
lma-core.j2,lma-leaf.j2,uma-core.j2, anduma-leaf.j2templates. These templates generate EOS device configurations for core and leaf roles, handling both standard and converged (multi-VRF) topologies. [1] [2]Support for Converged Topologies:
ceos_converged.j2) when appropriate, ensuring flexibility and reusability. [1] [2]Device and Interface Configuration Logic:
BGP Neighbor Handling Enhancements:
allowas-inand other peer-specific settings to support advanced routing scenarios.These changes collectively enable automated, flexible generation of EOS configurations for new UMA and LMA management network topologies.
Please provide following information to help code review process a bit easier:
-->
Description of PR
Summary:
Fixes # (issue)
Type of change
Back port request
Tracking issue/work item for backport/cherry-pick request (GitHub issue or Microsoft ADO):
Failure type:
Tested branch
Test result
Approach
What is the motivation for this PR?
How did you do it?
How did you verify/test it?
Any platform specific information?
Supported testbed topology if it's a new test case?
Documentation