[multi-dut] Support for gen-mg to work with multi-dut where VM's connect to only a single host and supervisor card #2700

sanmalho-git · 2020-12-23T22:43:25Z

Description of PR

Summary:
Fixes # (issue)

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Approach

What is the motivation for this PR?

For SONiC Chassis support gen-mg has the following limitations:

for multi-dut it assumes each VM is connected to all the DUT's (similar to the dualtor topology)
assumes that all interfaces connected to a PortChannel are sequential.

It also needs the following enhancements:

In a SONiC Chassis we have a 'supervisor' card which has no ports and thus no BGP/loopback/IpInterfaces/ACL's etc. The minigraph should not include info about these.
We need to introduce 'T3' router type as 'CoreRouter'

How did you do it?

For VMs to connect to a single DUT:

In vm_topo_config, the fields like interface_indexes, intfs, ip_intf etc would be either empty or null. Add checks for these where they are being used in gen-mg (config_sonic_basedon_testbed.yml and the minigraph templates).

For not all PortChannel ports being sequential:

In config_sonic_based_on_testbed.yml, changed interface_to_vms structure to be compatable to allow using ansible's 'with_subelements' loop to get the intf_names

For SONiC Chassis support:

For 'supervisor' card, there are no BGP peers, interfaces etc. required.
- A 'supervisor' card is identified by having 'type' field in the inventory with value 'supervisor'.
- Add this check to skip generation of portions not valid for supervisor card
Added 'CoreRouter' as the DUT type for T3 VM's that connect to 'T2' DUT.

How did you verify/test it?

Ran gen-mg against all the testbeds defined in vtestbed.csv with latest github master code, and then with our code changes. Verified that there are no differences in the generated minigraph files.
Ran against topo_t2.yml proposed in PR Introduce basic T2 topology #2638 where we have 3 linecards with 24 VM's each (8 on 2 port LAG and 16 on single port) and 1 supervisor card.
- Validated generated minigraph files for the linecards are the same as those generated by running gen-mg against individual linecard testbed (with only 1 DUT).
- Validate the generated minigraph file for supervisor card is acceptable to sonic-cfggen and the generated config_db.json is valid for the supervisor card.

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

sanmalho-git · 2020-12-28T22:43:53Z

retest vs image

sanmalho-git · 2021-01-04T21:56:45Z

@wangxin @yxieca - can you please review

ansible/library/topo_facts.py

wangxin · 2021-01-22T09:53:31Z

ansible/config_sonic_basedon_testbed.yml

@@ -134,7 +134,8 @@

  - name: find all interface indexes mapping connecting to VM
    set_fact:
-      interface_to_vms: "{{ interface_to_vms|default({}) | combine({ item.key: item.value['interface_indexes'][dut_index|int] }) }}"
+      interface_to_vms: "{{ interface_to_vms|default([]) + [ {'name': item.key, 'ports': item.value['interface_indexes'][dut_index|int] }] }}"
+    when: item.value|length


The item value is vm config dict gathered by the topo_facts module. The length filter will return number of keys in the vm config dict. According to current implementation, I don't see a chance that the length could be 0. Is the original purpose to check if item.value['interface_indexes'] is empty?

wangxin · 2021-01-22T10:51:37Z

ansible/templates/minigraph_dpg.j2

 {%- for index in range(vms_number) %}
 {% if 'port-channel' in vm_topo_config['vm'][vms[index]]['ip_intf'][dut_index|int]|lower %}
-PortChannel{{ ((index+1) |string).zfill(4) }}{% if not loop.last %};{% endif %}
+{% set a_intf = 'PortChannel' + ((index+1) |string).zfill(4) %}


PortChannel is based on physical interfaces in vm_topo_config['vm'][vms[index]]['intfs'][dut_index|int].
If there is no physical interface between VM and DUT, there would be no PortChannel and DUT. Shall we make this part conditional too based on vm_topo_config['vm'][vms[index]]['intfs'][dut_index|int]|length like the change just below?

ansible/templates/minigraph_dpg.j2

saravanansv

similar to ansible/templates/minigraph_*.j2 should there be a ansible/templates/minigraph_supe.j2 that holds the template for all the supervisor related config ?

sanmalho-git · 2021-02-02T15:28:56Z

At the time of putting this PR, the configuration required for supervisor card was not finalized. For Nokia supervisor card, we have a bare minimal config. If we require a template, then we can remove the 'supervisor' related changes from this PR and a subsequent PR can handle the generation of minigraph for supervisor card.

saravanansv · 2021-02-02T16:08:48Z

At the time of putting this PR, the configuration required for supervisor card was not finalized. For Nokia supervisor card, we have a bare minimal config. If we require a template, then we can remove the 'supervisor' related changes from this PR and a subsequent PR can handle the generation of minigraph for supervisor card.

I'm okay with merging this change. Just wondering what other changes are needed for supervior's config

wangxin · 2021-02-05T13:27:50Z

@sanmalho-git Can you resolve the merge conflict?

…a single host; support for supervisor card. gen-mg supported multi-dut, but in a dualtor topology, each VM is assumed to be connected to all the DUT's. To support the VM to be connected to a single DUT: - In vm_topo_config, the fields like interface_indexes, intfs, ip_intf etc would be either empty or null. Add checks for these where they are being used in gen-mg gen-mg also assumes that all interfaces connected to a PortChannel are sequential. To fix this: - In config_sonic_based_on_testbed.yml, changed interface_to_vms structure to be compatable to allow using with_subelements to get the intf_names For 'supervisor' card, there are no BGP peers, interfaces etc. required. - A 'supervisor' card is identified by having 'type' field in the inventory with value 'supervisor'. - Add this check to skip generation of portions not valid for supervisor card

…l based on review comment

wangxin · 2021-02-07T01:32:24Z

/AzurePipelines run

azure-pipelines · 2021-02-07T01:32:33Z

Azure Pipelines successfully started running 1 pipeline(s).

sanmalho-git force-pushed the genmg branch from 54bd31a to 9593c1a Compare January 4, 2021 14:56

yxieca requested review from theasianpianist, a team and wangxin January 19, 2021 01:58

yxieca reviewed Jan 19, 2021

View reviewed changes

ansible/library/topo_facts.py Show resolved Hide resolved

yxieca reviewed Jan 19, 2021

View reviewed changes

ansible/library/topo_facts.py Show resolved Hide resolved

sanmalho-git force-pushed the genmg branch from 9593c1a to 2e307fb Compare January 19, 2021 15:12

lguohan added the chassis label Jan 20, 2021

lguohan added this to In progress in Chassis via automation Jan 20, 2021

wangxin reviewed Jan 22, 2021

View reviewed changes

sanmalho-git force-pushed the genmg branch from 2e307fb to 4e10b8f Compare January 22, 2021 15:47

saravanansv reviewed Feb 2, 2021

View reviewed changes

wangxin approved these changes Feb 3, 2021

View reviewed changes

Chassis automation moved this from In progress to Reviewer approved Feb 3, 2021

sanmalho-git added 4 commits February 5, 2021 09:48

Minor fix

e33501c

Added comments to topo_facts as per review

b665199

Remove a check that is not requied in config_sonic_basedon_testbed.ym…

933c5cc

…l based on review comment

sanmalho-git force-pushed the genmg branch from 4e10b8f to 933c5cc Compare February 5, 2021 14:48

wangxin merged commit 2b00e37 into sonic-net:master Feb 8, 2021

Chassis automation moved this from Reviewer approved to Done Feb 8, 2021

sanmalho-git deleted the genmg branch February 8, 2021 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[multi-dut] Support for gen-mg to work with multi-dut where VM's connect to only a single host and supervisor card #2700

[multi-dut] Support for gen-mg to work with multi-dut where VM's connect to only a single host and supervisor card #2700

sanmalho-git commented Dec 23, 2020

sanmalho-git commented Dec 28, 2020

sanmalho-git commented Jan 4, 2021

wangxin Jan 22, 2021

wangxin Jan 22, 2021

saravanansv left a comment

sanmalho-git commented Feb 2, 2021

saravanansv commented Feb 2, 2021

wangxin commented Feb 5, 2021

wangxin commented Feb 7, 2021

azure-pipelines bot commented Feb 7, 2021

[multi-dut] Support for gen-mg to work with multi-dut where VM's connect to only a single host and supervisor card #2700

[multi-dut] Support for gen-mg to work with multi-dut where VM's connect to only a single host and supervisor card #2700

Conversation

sanmalho-git commented Dec 23, 2020

Description of PR

Type of change

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

sanmalho-git commented Dec 28, 2020

sanmalho-git commented Jan 4, 2021

wangxin Jan 22, 2021

Choose a reason for hiding this comment

wangxin Jan 22, 2021

Choose a reason for hiding this comment

saravanansv left a comment

Choose a reason for hiding this comment

sanmalho-git commented Feb 2, 2021

saravanansv commented Feb 2, 2021

wangxin commented Feb 5, 2021

wangxin commented Feb 7, 2021

azure-pipelines bot commented Feb 7, 2021